Re: [PATCH] IOMMU/x86: work around bogus gcc12 warning in hvm_gsi_eoi()

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

On Fri, Jun 10, 2022 at 09:29:44AM +0200, Jan Beulich wrote:
> On 10.06.2022 09:20, Roger Pau Monné wrote:
> > On Fri, May 27, 2022 at 12:37:19PM +0200, Jan Beulich wrote:
> >> As per [1] the expansion of the pirq_dpci() macro causes a -Waddress
> >> controlled warning (enabled implicitly in our builds, if not by default)
> >> tying the middle part of the involved conditional expression to the
> >> surrounding boolean context. Work around this by introducing a local
> >> inline function in the affected source file.
> >>
> >> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967
> >> ---
> >> This is intended to replace an earlier patch by Andrew [2], open-coding
> >> and then simplifying the macro in the one problematic place.
> >>
> >> Note that, with pirq_dpci() presently used solely in the one file being
> >> changed here, we could in principle also remove the #define and use just
> >> an inline(?) function in this file. But then the macro would need
> >> reinstating as soon as a use elsewhere would become necessary.
> 
> Did you read this before ...
> 
> >> As to the inline - I think it's warranted here, but it goes against our
> >> general policy of using inline only in header files. Hence I'd be okay
> >> to drop it to avoid controversy.
> >>
> >> [2] https://lists.xen.org/archives/html/xen-devel/2021-10/msg01635.html
> >>
> >> --- a/xen/drivers/passthrough/x86/hvm.c
> >> +++ b/xen/drivers/passthrough/x86/hvm.c
> >> @@ -25,6 +25,18 @@
> >>  #include <asm/hvm/support.h>
> >>  #include <asm/io_apic.h>
> >>  
> >> +/*
> >> + * Gcc12 takes issue with pirq_dpci() being used in boolean context (see 
> >> gcc
> >> + * bug 102967). While we can't replace the macro definition in the header 
> >> by an
> >> + * inline function, we can do so here.
> >> + */
> >> +static inline struct hvm_pirq_dpci *_pirq_dpci(struct pirq *pirq)
> >> +{
> >> +    return pirq_dpci(pirq);
> >> +}
> >> +#undef pirq_dpci
> >> +#define pirq_dpci(pirq) _pirq_dpci(pirq)
> > 
> > That's fairly ugly.  Seeing as pirq_dpci is only used in hvm.c, would
> > it make sense to just convert the macro to be a static inline in that
> > file? (and remove pirq_dpci() from irq.h).
> 
> ... saying so? IOW I'm not entirely opposed, but I'm a little afraid we might
> be setting us up for later trouble. 

Sorry, started replying yesterday but had to leave and left the reply
open.  Then when I came back this morning I just read the code and not
the commit message.

Hm, so ideally we would also then move dpci_pirq() to hvm.c in order
to match the move of pirq_dpci(), but that's not possible due to that
helper having other callers outside of hvm.c.

We could always export the function from hvm.c if we gained outside
callers.  In any case, I don't want to block you further on this, so:

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

Thanks, Roger.