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.
