Re: [RFC?] xen/arm: memaccess: Pass struct npfec by reference in p2m_mem_access_check

[Oleksandr <olekstysh@xxxxxxxxx>]

On 26.11.21 13:39, Andrew Cooper wrote:


Hi Andrew

> On 26/11/2021 07:46, Jan Beulich wrote:
> > On 25.11.2021 23:49, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > >
> > > Today I noticed a "note" when building Xen on Arm64 with
> > > aarch64-poky-linux-gcc (GCC) 9.3.0. It turned out that Andrew Cooper
> > > had alredy reported it before [1]:
> > >
> > > mem_access.c: In function 'p2m_mem_access_check':
> > > mem_access.c:227:6: note: parameter passing for argument of type
> > > 'const struct npfec' changed in GCC 9.1
> > >    227 | bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla,
> > >                                    const struct npfec npfec)
> > >
> > >  From the explanation I understand that nothing bad actually is going
> > > to happen in our case, it is harmless and shown to only draw our
> > > attention that the ABI changed due to bug (with passing bit-fields
> > > by value) fixed in GCC 9.1. This information doesn't mean much for us
> > > as Xen is an embedded project with no external linkage. But, of course,
> > > it would be better to eliminate the note. You can also find related
> > > information about the bug at [2].
> > >
> > > So make the note go away by passing bit-fields by reference.
> > >
> > > [1] https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg87439.html
> > > [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469
> > >
> > > Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > ---
> > > Compile-tested only.
> > > ---
> > >   xen/arch/arm/mem_access.c        | 28 ++++++++++++++--------------
> > >   xen/arch/arm/traps.c             |  2 +-
> > >   xen/include/asm-arm/mem_access.h |  2 +-
> > >   3 files changed, 16 insertions(+), 16 deletions(-)
> > It's all Arm code, so I'm not the one to judge, but I'd like to recommend
> > to live with the note or convince distros to backport the gcc side fix.
> > This definitely was a compiler flaw; see
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91710.
> I too would recommend just living with the note.  The code change
> proposed is a backwards step in terms of runtime complexity - you're now
> passing around a pointer to 7 bits of information, which the compiler
> cannot pull into a local because of C's aliasing rules.  At a guess, the
> very best an optimising compiler could do is turn it into only two
> dereferences of the pointer.
Thank you for the analysis. I don't think, we want to make things worse 
(less optimal) than they are currently.

> ~Andrew
--
Regards,

Oleksandr Tyshchenko