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

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

[Andrew Cooper]

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 )

Do so.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>

Request for 4.16, as being very low risk.

This is a build problem with a soon-to-be-released compiler, but the issue it
highlights is real and the fix is a clear improvement in code quality.  There
is no difference in the compiled binary as a result of this change.

  $ diff -u dis-before dis-after
  --- dis-before       2021-10-27 21:00:07.512530321 +0100
  +++ dis-after        2021-10-27 21:00:25.996752544 +0100
  @@ -1,5 +1,5 @@

  -xen-syms-before:     file format elf64-x86-64
  +xen-syms-after:     file format elf64-x86-64

   Disassembly of section .text:

If this does not get taken at this point, it will need backporting after the
release, when GCC 12 is released.
---
 xen/drivers/passthrough/x86/hvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index 351daafdc9bf..22bf84639f22 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -902,7 +902,7 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int gsi)
     struct pirq *pirq = pirq_info(d, gsi);
 
     /* Check if GSI is actually mapped. */
-    if ( !pirq_dpci(pirq) )
+    if ( !pirq )
         return;
 
     hvm_gsi_deassert(d, gsi);
-- 
2.11.0

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

[Roger Pau Monné]

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?

IOW we could change how pirq_dpci is implemented and then that
relation might no longer be true. What we care in hvm_gsi_eoi is not
only having a valid pirq, but also having a valid dpci struct which
will only be the case if the PIRQ is bound to an HVM domain, and that
is what the check tries to represent.

I know this is not how pirq_dpci is currently implemented, but I don't
see why it couldn't change in the future.

Thanks, Roger.

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

[Andrew Cooper]

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.

~Andrew

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Andrew Cooper]

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

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

[Roger Pau Monné]

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@citrix.com>

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.

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

[Jan Beulich]

On 27.10.2021 22:07, 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) )

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.

Jan

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

[Ian Jackson]

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.  Please vote on
it if you can't quickly get consensus.

I have added the for-4.16 tag since this seems like a bugfix that
should probably go into 4.16 ?  Unless I have misunderstood.

Thanks,
Ian.
(with RM hat on)

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

[Jan Beulich]

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).

Jan

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

[Andrew Cooper]

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, and
its use in this case does not do what the source code comment says
either.  A GSI is mapped when a pirq object exists, not a dpci object.


If your answer is "well actually, we didn't mean to say 'if a GSI is
mapped' in the comment, and here's a different predicate which actually
inspects the state of a dpci object for validity", then fine -  that
will shut the compiler up because you're no longer checking for the
NULLness of a pointer to a sub-object of a non-NULL pointer, but that's
a bugfix which needs backporting several releases too.

The current logic is not correct, and does not become correct by trying
pass blame to the compiler.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967 is the GCC bug, but
the result of it was them persuading me that the diagnostic was
legitimate, even if currently expressed badly.  They've agreed to fix
how it is expressed, but I doubt you'll persuade them that the trigger
for the diagnostic in the first place was wrong.

~Andrew

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

[Jan Beulich]

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?

> and
> its use in this case does not do what the source code comment says
> either.  A GSI is mapped when a pirq object exists, not a dpci object.

As per my earlier reply on the thread, I view the check as a guard
for the subsequent call to hvm_pirq_eoi(), where _this_ pointer
(and not pirq) is assumed to be non-NULL (while pirq gets explicitly
checked).

> If your answer is "well actually, we didn't mean to say 'if a GSI is
> mapped' in the comment, and here's a different predicate which actually
> inspects the state of a dpci object for validity", then fine -  that
> will shut the compiler up because you're no longer checking for the
> NULLness of a pointer to a sub-object of a non-NULL pointer, but that's
> a bugfix which needs backporting several releases too.
> 
> The current logic is not correct, and does not become correct by trying
> pass blame to the compiler.

I have yet to understand in which way you deem the current logic to not
be correct. I'm sorry for being dense.

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967 is the GCC bug, but
> the result of it was them persuading me that the diagnostic was
> legitimate, even if currently expressed badly.  They've agreed to fix
> how it is expressed, but I doubt you'll persuade them that the trigger
> for the diagnostic in the first place was wrong.

Well, thanks for the pointer in any event. I've commented there as well.

Jan

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

[Roger Pau Monné]

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.

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

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH for-4.16] x86/passthrough: Fix hvm_gsi_eoi() build with GCC 12"):
> 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.

Anyway, I don't think there are any functional implications.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

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

[Jan Beulich]

On 04.11.2021 16:24, Roger Pau Monné wrote:
> It might be appropriate to switch pirq_dpci to:
> 
> #define pirq_dpci(d, pirq) \
>     ((is_hvm_domain(d) && (pirq)) ? &(pirq)->arch.hvm.dpci : NULL)

I don't see how this would help suppress the warning.

> Or to an inline helper.

I expect it's a macro because an inline function would cause header
dependency issues. Plus both that or any other attempt to leverage
some means to circumvent gcc recognizing the pattern is only going
to help until they further enhance their recognition.

Jan

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

[Roger Pau Monné]

On Thu, Nov 04, 2021 at 01:17:53PM +0100, Jan Beulich wrote:
> On 04.11.2021 11:48, Andrew Cooper wrote:
> > If your answer is "well actually, we didn't mean to say 'if a GSI is
> > mapped' in the comment, and here's a different predicate which actually
> > inspects the state of a dpci object for validity", then fine -  that
> > will shut the compiler up because you're no longer checking for the
> > NULLness of a pointer to a sub-object of a non-NULL pointer, but that's
> > a bugfix which needs backporting several releases too.
> > 
> > The current logic is not correct, and does not become correct by trying
> > pass blame to the compiler.
> 
> I have yet to understand in which way you deem the current logic to not
> be correct. I'm sorry for being dense.
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967 is the GCC bug, but
> > the result of it was them persuading me that the diagnostic was
> > legitimate, even if currently expressed badly.  They've agreed to fix
> > how it is expressed, but I doubt you'll persuade them that the trigger
> > for the diagnostic in the first place was wrong.
> 
> Well, thanks for the pointer in any event. I've commented there as well.

Did we get any resolution out of this?

It would be good IMO if we could build out of the box with GCC 12
instead of having to backport fixes later on.

Regards, Roger.

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

[Jan Beulich]

On 18.11.2021 09:33, Roger Pau Monné wrote:
> On Thu, Nov 04, 2021 at 01:17:53PM +0100, Jan Beulich wrote:
>> On 04.11.2021 11:48, Andrew Cooper wrote:
>>> If your answer is "well actually, we didn't mean to say 'if a GSI is
>>> mapped' in the comment, and here's a different predicate which actually
>>> inspects the state of a dpci object for validity", then fine -  that
>>> will shut the compiler up because you're no longer checking for the
>>> NULLness of a pointer to a sub-object of a non-NULL pointer, but that's
>>> a bugfix which needs backporting several releases too.
>>>
>>> The current logic is not correct, and does not become correct by trying
>>> pass blame to the compiler.
>>
>> I have yet to understand in which way you deem the current logic to not
>> be correct. I'm sorry for being dense.
>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967 is the GCC bug, but
>>> the result of it was them persuading me that the diagnostic was
>>> legitimate, even if currently expressed badly.  They've agreed to fix
>>> how it is expressed, but I doubt you'll persuade them that the trigger
>>> for the diagnostic in the first place was wrong.
>>
>> Well, thanks for the pointer in any event. I've commented there as well.
> 
> Did we get any resolution out of this?

I don't think we did. I'm still struggling to understand Andrew's way
of thinking.

> It would be good IMO if we could build out of the box with GCC 12
> instead of having to backport fixes later on.

I guess gcc12 is too far from getting released that there could be any
guarantee for no further issues to get exposed by that point. It has
also been common for us to backport fixes and workarounds when new
compiler versions appear.

I could agree to being proactive if the change to make to our code was
uncontroversial.

Jan

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

[Roger Pau Monné]

On Thu, Nov 18, 2021 at 09:51:52AM +0100, Jan Beulich wrote:
> On 18.11.2021 09:33, Roger Pau Monné wrote:
> > On Thu, Nov 04, 2021 at 01:17:53PM +0100, Jan Beulich wrote:
> >> On 04.11.2021 11:48, Andrew Cooper wrote:
> >>> If your answer is "well actually, we didn't mean to say 'if a GSI is
> >>> mapped' in the comment, and here's a different predicate which actually
> >>> inspects the state of a dpci object for validity", then fine -  that
> >>> will shut the compiler up because you're no longer checking for the
> >>> NULLness of a pointer to a sub-object of a non-NULL pointer, but that's
> >>> a bugfix which needs backporting several releases too.
> >>>
> >>> The current logic is not correct, and does not become correct by trying
> >>> pass blame to the compiler.
> >>
> >> I have yet to understand in which way you deem the current logic to not
> >> be correct. I'm sorry for being dense.
> >>
> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967 is the GCC bug, but
> >>> the result of it was them persuading me that the diagnostic was
> >>> legitimate, even if currently expressed badly.  They've agreed to fix
> >>> how it is expressed, but I doubt you'll persuade them that the trigger
> >>> for the diagnostic in the first place was wrong.
> >>
> >> Well, thanks for the pointer in any event. I've commented there as well.
> > 
> > Did we get any resolution out of this?
> 
> I don't think we did. I'm still struggling to understand Andrew's way
> of thinking.

What about the GCC bug report?

Ultimately we need GCC people to make the check less restrictive or we
need a way to rework the code in a way that doesn't trigger it, either
Andrew's proposal or something else.

> > It would be good IMO if we could build out of the box with GCC 12
> > instead of having to backport fixes later on.
> 
> I guess gcc12 is too far from getting released that there could be any
> guarantee for no further issues to get exposed by that point. It has
> also been common for us to backport fixes and workarounds when new
> compiler versions appear.
> 
> I could agree to being proactive if the change to make to our code was
> uncontroversial.

OK, but unless GCC changes their mind we are likely to have this
conversation again in the future, so we might be just delaying the
inevitable.

Thanks, Roger.

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

[Jan Beulich]

On 18.11.2021 10:34, Roger Pau Monné wrote:
> On Thu, Nov 18, 2021 at 09:51:52AM +0100, Jan Beulich wrote:
>> On 18.11.2021 09:33, Roger Pau Monné wrote:
>>> On Thu, Nov 04, 2021 at 01:17:53PM +0100, Jan Beulich wrote:
>>>> On 04.11.2021 11:48, Andrew Cooper wrote:
>>>>> If your answer is "well actually, we didn't mean to say 'if a GSI is
>>>>> mapped' in the comment, and here's a different predicate which actually
>>>>> inspects the state of a dpci object for validity", then fine -  that
>>>>> will shut the compiler up because you're no longer checking for the
>>>>> NULLness of a pointer to a sub-object of a non-NULL pointer, but that's
>>>>> a bugfix which needs backporting several releases too.
>>>>>
>>>>> The current logic is not correct, and does not become correct by trying
>>>>> pass blame to the compiler.
>>>>
>>>> I have yet to understand in which way you deem the current logic to not
>>>> be correct. I'm sorry for being dense.
>>>>
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967 is the GCC bug, but
>>>>> the result of it was them persuading me that the diagnostic was
>>>>> legitimate, even if currently expressed badly.  They've agreed to fix
>>>>> how it is expressed, but I doubt you'll persuade them that the trigger
>>>>> for the diagnostic in the first place was wrong.
>>>>
>>>> Well, thanks for the pointer in any event. I've commented there as well.
>>>
>>> Did we get any resolution out of this?
>>
>> I don't think we did. I'm still struggling to understand Andrew's way
>> of thinking.
> 
> What about the GCC bug report?
> 
> Ultimately we need GCC people to make the check less restrictive or we
> need a way to rework the code in a way that doesn't trigger it, either
> Andrew's proposal or something else.
> 
>>> It would be good IMO if we could build out of the box with GCC 12
>>> instead of having to backport fixes later on.
>>
>> I guess gcc12 is too far from getting released that there could be any
>> guarantee for no further issues to get exposed by that point. It has
>> also been common for us to backport fixes and workarounds when new
>> compiler versions appear.
>>
>> I could agree to being proactive if the change to make to our code was
>> uncontroversial.
> 
> OK, but unless GCC changes their mind we are likely to have this
> conversation again in the future, so we might be just delaying the
> inevitable.

Yes, but we apparently need time to settle on how to work around the
issue. I was actually surprised Andrew was able to talk you into
going the chosen route despite your open-coding concerns, which I
share. In the course of the discussion at least one alternative
approach was already outlined, iirc.

Jan