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

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

On Thu, Nov 04, 2021 at 01:17:53PM +0100, Jan Beulich wrote:
> On 04.11.2021 11:48, Andrew Cooper wrote:
> > On 04/11/2021 08:07, Jan Beulich wrote:
> >> On 03.11.2021 17:13, Ian Jackson wrote:
> >>> Jan Beulich writes ("Re: [PATCH] x86/passthrough: Fix hvm_gsi_eoi() build 
> >>> with GCC 12"):
> >>>> On 27.10.2021 22:07, Andrew Cooper wrote:
> >>>>>   if ( !((pirq) ? &(pirq)->arch.hvm.dpci : NULL) )
> >>>> I disagree with the compiler's analysis: While &(pirq)->arch.hvm.dpci
> >>>> indeed can't be NULL, that's not the operand of !. The operand of !
> >>>> can very well be NULL, when pirq is.
> >>>>
> >>>>> which is a hint that the code is should be simplified to just:
> >>>>>
> >>>>>   if ( !pirq )
> >>>>>
> >>>>> Do so.
> >>>> And I further agree with Roger's original reply (despite you
> >>>> apparently having managed to convince him): You shouldn't be open-
> >>>> coding pirq_dpci(). Your observation that the construct's result
> >>>> isn't otherwise used in the function is only one half of it. The
> >>>> other half is that hvm_pirq_eoi() gets called from here, and that
> >>>> one does require the result to be non-NULL.
> >>> Can you (collectively) please come to some agreement here ?
> >>> I think this is mostly a question of taste or style.
> >> Personally I don't think open-coding of constructs is merely a style /
> >> taste issue. The supposed construct changing and the open-coded
> >> instance then being forgotten (likely not even noticed) can lead to
> >> actual bugs. I guess the issue should at least be raised as one against
> >> gcc12, such that the compiler folks can provide their view on whether
> >> the warning is actually appropriate in that case (and if so, what their
> >> take is on a general approach towards silencing such warnings when
> >> they're identified as false positives).
> > 
> > This isn't opencoding anything.
> > 
> > The compiler has pointed out that the logic, as currently expressed, is
> > junk and doesn't do what you think it does.
> > 
> > And based on the, IMO dubious, argument that both you and Roger have
> > used to try and defend the current code, I agree with the compiler.
> > 
> > pirq_dpci() does not have the property that you seem expect of it,
> 
> Which property is that, exactly?

I honestly don't think we expect any property of pirq_dpci, it just
tells whether a pirq has a dpci associated with it or not. As I said
on my previous replies I think GCC is not correct in doing the check
after expanding the macro.

The relation between a pirq and it's dpci is an implementation detail
that could change at any point, and hence the complain by GCC would no
longer be true. That's exactly why we use a macro to get the dpci out
of a pirq, because how that's obtained is opaque to the caller.

So while it's true that a NULL pirq will always result in a NULL dpci,
a non-NULL pirq should not be assumed to result in a non-NULL dpci,
which is inferred by GCC by expanding the macro.

In this specific case I think the change is fine for two reasons:

 * The pirq is obtained from the domain struct.
 * The domain is known to be HVM because the only caller of
   hvm_gsi_eoi is hvm_dpci_eoi which in
   turn is only called by the vIO-APIC and the vPIC emulation code.

I dislike of GCC doing those checks to expanded macros. In this case I
think the change is fine due to my reasoning above.

It might be appropriate to switch pirq_dpci to:

#define pirq_dpci(d, pirq) \
    ((is_hvm_domain(d) && (pirq)) ? &(pirq)->arch.hvm.dpci : NULL)

Or to an inline helper.

Regards, Roger.