On 28/10/2021 14:26, Roger Pau Monné wrote:
> On Thu, Oct 28, 2021 at 01:15:13PM +0100, Andrew Cooper wrote:
>> On 28/10/2021 08:31, Roger Pau Monné wrote:
>>> On Wed, Oct 27, 2021 at 09:07:13PM +0100, Andrew Cooper wrote:
>>>> GCC master (nearly version 12) complains:
>>>>
>>>> hvm.c: In function 'hvm_gsi_eoi':
>>>> hvm.c:905:10: error: the comparison will always evaluate as 'true' for
>>>> the
>>>> address of 'dpci' will never be NULL [-Werror=address]
>>>> 905 | if ( !pirq_dpci(pirq) )
>>>> | ^
>>>> In file included from /local/xen.git/xen/include/xen/irq.h:73,
>>>> from /local/xen.git/xen/include/xen/pci.h:13,
>>>> from /local/xen.git/xen/include/asm/hvm/io.h:22,
>>>> from /local/xen.git/xen/include/asm/hvm/domain.h:27,
>>>> from /local/xen.git/xen/include/asm/domain.h:7,
>>>> from /local/xen.git/xen/include/xen/domain.h:8,
>>>> from /local/xen.git/xen/include/xen/sched.h:11,
>>>> from /local/xen.git/xen/include/xen/event.h:12,
>>>> from hvm.c:20:
>>>> /local/xen.git/xen/include/asm/irq.h:140:34: note: 'dpci' declared here
>>>> 140 | struct hvm_pirq_dpci dpci;
>>>> | ^~~~
>>>>
>>>> The location marker is unhelpfully positioned and upstream may get around
>>>> to
>>>> fixing it. The complaint is intended to be:
>>>>
>>>> if ( !((pirq) ? &(pirq)->arch.hvm.dpci : NULL) )
>>>> ^~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> which is a hint that the code is should be simplified to just:
>>>>
>>>> if ( !pirq )
>>> I likely need more coffee, but doesn't this exploit how the macro
>>> (pirq_dpci) is currently coded?
>> The way pirq_dpci() is currently coded, this is nonsense, which GCC is
>> now highlighting.
>>
>> It would be a very different thing if the logic said:
>>
>> struct pirq *pirq = pirq_info(d, gsi);
>> struct hvm_pirq_dpci *dpci = pirq_dpci(pirq);
>>
>> /* Check if GSI is actually mapped. */
>> if ( !dpci )
>> return;
>>
>> but it doesn't. Getting a non-null pirq pointer from pirq_info(d, gsi)
>> does identify that the GSI is mapped, and the dpci nested structure is
>> not used in this function. I would expect this property to remain true
>> even if we alter the data layout.
> I think we have further assertions that this will be true:
>
> 1. d can only be an HVM domain given the callers of the function.
> 2. The pirq struct is obtained from the list of pirqs owned by d.
>
> In which case it's assured that the pirq will always have a dpci. I
> think it might be better if the check was replaced with:
>
> if ( !is_hvm_domain(d) || !pirq )
> {
> ASSERT(is_hvm_domain(d));
> return;
> }
>
> Here Xen cares that pirq is not NULL and that d is an HVM domain
> because hvm_gsi_deassert will assume so.
We're several calls deep in an hvm-named codepath, called exclusively
from logic in arch/x86/hvm/
is_hvm_domain() is far from free, and while I'm in favour of having
suitable sanity checks in appropriate places, I don't even think:
{
struct pirq *pirq = pirq_info(d, gsi);
ASSERT(is_hvm_domain(d));
if ( !pirq )
return;
...
would be appropriate in this case.
~Andrew
