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

[Andrew Cooper <amc96@xxxxxxxx>]

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.

~Andrew