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(-)
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
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@lists.xenproject.org/msg87439.html
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
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(-)
diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index 3e36202..d21bba7 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -224,7 +224,7 @@ err:
return page;
}
-bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
+bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec *npfec)
{
int rc;
bool violation;
@@ -248,23 +248,23 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
violation = false;
break;
case XENMEM_access_rw:
- violation = npfec.insn_fetch;
+ violation = npfec->insn_fetch;
break;
case XENMEM_access_wx:
- violation = npfec.read_access;
+ violation = npfec->read_access;
break;
case XENMEM_access_rx:
case XENMEM_access_rx2rw:
- violation = npfec.write_access;
+ violation = npfec->write_access;
break;
case XENMEM_access_x:
- violation = npfec.read_access || npfec.write_access;
+ violation = npfec->read_access || npfec->write_access;
break;
case XENMEM_access_w:
- violation = npfec.read_access || npfec.insn_fetch;
+ violation = npfec->read_access || npfec->insn_fetch;
break;
case XENMEM_access_r:
- violation = npfec.write_access || npfec.insn_fetch;
+ violation = npfec->write_access || npfec->insn_fetch;
break;
default:
case XENMEM_access_n:
@@ -277,7 +277,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
return true;
/* First, handle rx2rw and n2rwx conversion automatically. */
- if ( npfec.write_access && xma == XENMEM_access_rx2rw )
+ if ( npfec->write_access && xma == XENMEM_access_rx2rw )
{
rc = p2m_set_mem_access(v->domain, gaddr_to_gfn(gpa), 1,
0, ~0, XENMEM_access_rw, 0);
@@ -324,19 +324,19 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
/* Send request to mem access subscriber */
req->u.mem_access.gfn = gpa >> PAGE_SHIFT;
req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
- if ( npfec.gla_valid )
+ if ( npfec->gla_valid )
{
req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
req->u.mem_access.gla = gla;
- if ( npfec.kind == npfec_kind_with_gla )
+ if ( npfec->kind == npfec_kind_with_gla )
req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
- else if ( npfec.kind == npfec_kind_in_gpt )
+ else if ( npfec->kind == npfec_kind_in_gpt )
req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
}
- req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0;
- req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0;
- req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0;
+ req->u.mem_access.flags |= npfec->read_access ? MEM_ACCESS_R : 0;
+ req->u.mem_access.flags |= npfec->write_access ? MEM_ACCESS_W : 0;
+ req->u.mem_access.flags |= npfec->insn_fetch ? MEM_ACCESS_X : 0;
if ( monitor_traps(v, (xma != XENMEM_access_n2rwx), req) < 0 )
domain_crash(v->domain);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 219ab3c..4ad1618 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1957,7 +1957,7 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
.kind = xabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
};
- p2m_mem_access_check(gpa, gva, npfec);
+ p2m_mem_access_check(gpa, gva, &npfec);
/*
* The only way to get here right now is because of mem_access,
* thus reinjecting the exception to the guest is never required.
diff --git a/xen/include/asm-arm/mem_access.h b/xen/include/asm-arm/mem_access.h
index 35ed0ad..be43e18 100644
--- a/xen/include/asm-arm/mem_access.h
+++ b/xen/include/asm-arm/mem_access.h
@@ -35,7 +35,7 @@ static inline bool p2m_mem_access_sanity_check(struct domain *d)
* Send mem event based on the access. Boolean return value indicates if trap
* needs to be injected into guest.
*/
-bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec);
+bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec *npfec);
struct page_info*
p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
--
2.7.4
On Thu, Nov 25, 2021 at 5:49 PM Oleksandr Tyshchenko <olekstysh@gmail.com> wrote: > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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@lists.xenproject.org/msg87439.html > [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469 > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
On 25.11.2021 23:49, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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@lists.xenproject.org/msg87439.html > [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469 > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > 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. Jan
On 26/11/2021 07:46, Jan Beulich wrote: > On 25.11.2021 23:49, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> 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@lists.xenproject.org/msg87439.html >> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469 >> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> 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
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@epam.com> >>> >>> 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@lists.xenproject.org/msg87439.html >>> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469 >>> >>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> 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
On 26.11.21 09:46, Jan Beulich wrote: Hi Jan > On 25.11.2021 23:49, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> 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@lists.xenproject.org/msg87439.html >> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88469 >> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> 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. Thank you for the pointer and suggestion. Actually, after the realization that note is harmless and doesn't matter in our case, we could indeed tolerate it. It is up to the maintainers to decide. I will be ok either way. > > Jan > -- Regards, Oleksandr Tyshchenko
© 2016 - 2024 Red Hat, Inc.