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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 29, 2021 at 07:06:43PM +0100, Andrew Cooper wrote:
> 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.

Fine:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I think I would prefer if you could slightly adjust the commit message
to mention that is already exclusively called from HVM only paths,
and that as such the HVM related dpirq structures will always be
present.

Thanks, Roger.