Re: [PATCH] x86/passthrough: Fix hvm_gsi_eoi() build with GCC 12

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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