xen/arch/arm/vcpreg.c | 12 +++++++++++- xen/include/asm-arm/arm64/sysregs.h | 4 ---- 2 files changed, 11 insertions(+), 5 deletions(-)
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
The last place in code making use of READ/WRITE_SYSREG32
on arm64 is in TVM_REG macro defining functions vreg_emulate_<register>.
Implement a macro WRITE_SYSREG_SZ which expands as follows:
-on arm64: WRITE_SYSREG
-on arm32: WRITE_SYSREG{32/64}
As there are no other places in the code using these helpers
on arm64 - remove them.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v1:
-implement WRITE_SYSREG_SZ instead of duplicating the TVM_REG
---
xen/arch/arm/vcpreg.c | 12 +++++++++++-
xen/include/asm-arm/arm64/sysregs.h | 4 ----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index f0cdcc8a54..10c4846954 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -47,6 +47,16 @@
*
*/
+#ifdef CONFIG_ARM_64
+#define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG(val, sysreg)
+#else
+/*
+ * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes
+ * on the below macro to be defined like that as well.
+ */
+#define WRITE_SYSREG_SZ(sz, val, sysreg...) WRITE_SYSREG##sz(val, sysreg)
+#endif
+
/* The name is passed from the upper macro to workaround macro expansion. */
#define TVM_REG(sz, func, reg...) \
static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
@@ -55,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
bool cache_enabled = vcpu_has_cache_enabled(v); \
\
GUEST_BUG_ON(read); \
- WRITE_SYSREG##sz(*r, reg); \
+ WRITE_SYSREG_SZ(sz, *r, reg); \
\
p2m_toggle_cache(v, cache_enabled); \
\
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 077fd95fb7..795901e1ba 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -87,10 +87,6 @@
/* Access to system registers */
-#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name))
-
-#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name)
-
#define WRITE_SYSREG64(v, name) do { \
uint64_t _r = v; \
asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
--
2.29.0
Hi Michal, > On 9 Jul 2021, at 13:40, Michal Orzel <Michal.Orzel@arm.com> wrote: > > AArch64 system registers are 64bit whereas AArch32 ones > are 32bit or 64bit. MSR/MRS are expecting 64bit values thus > we should get rid of helpers READ/WRITE_SYSREG32 > in favour of using READ/WRITE_SYSREG. > > The last place in code making use of READ/WRITE_SYSREG32 > on arm64 is in TVM_REG macro defining functions vreg_emulate_<register>. > Implement a macro WRITE_SYSREG_SZ which expands as follows: > -on arm64: WRITE_SYSREG > -on arm32: WRITE_SYSREG{32/64} > > As there are no other places in the code using these helpers > on arm64 - remove them. > > Signed-off-by: Michal Orzel <michal.orzel@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > Changes since v1: > -implement WRITE_SYSREG_SZ instead of duplicating the TVM_REG > --- > xen/arch/arm/vcpreg.c | 12 +++++++++++- > xen/include/asm-arm/arm64/sysregs.h | 4 ---- > 2 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > index f0cdcc8a54..10c4846954 100644 > --- a/xen/arch/arm/vcpreg.c > +++ b/xen/arch/arm/vcpreg.c > @@ -47,6 +47,16 @@ > * > */ > > +#ifdef CONFIG_ARM_64 > +#define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG(val, sysreg) > +#else > +/* > + * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes > + * on the below macro to be defined like that as well. > + */ > +#define WRITE_SYSREG_SZ(sz, val, sysreg...) WRITE_SYSREG##sz(val, sysreg) > +#endif > + > /* The name is passed from the upper macro to workaround macro expansion. */ > #define TVM_REG(sz, func, reg...) \ > static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > @@ -55,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > bool cache_enabled = vcpu_has_cache_enabled(v); \ > \ > GUEST_BUG_ON(read); \ > - WRITE_SYSREG##sz(*r, reg); \ > + WRITE_SYSREG_SZ(sz, *r, reg); \ > \ > p2m_toggle_cache(v, cache_enabled); \ > \ > diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h > index 077fd95fb7..795901e1ba 100644 > --- a/xen/include/asm-arm/arm64/sysregs.h > +++ b/xen/include/asm-arm/arm64/sysregs.h > @@ -87,10 +87,6 @@ > > /* Access to system registers */ > > -#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name)) > - > -#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name) > - > #define WRITE_SYSREG64(v, name) do { \ > uint64_t _r = v; \ > asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ > -- > 2.29.0 > >
Hi Michal, On 09/07/2021 13:40, Michal Orzel wrote: > AArch64 system registers are 64bit whereas AArch32 ones > are 32bit or 64bit. MSR/MRS are expecting 64bit values thus > we should get rid of helpers READ/WRITE_SYSREG32 > in favour of using READ/WRITE_SYSREG. > > The last place in code making use of READ/WRITE_SYSREG32 > on arm64 is in TVM_REG macro defining functions vreg_emulate_<register>. > Implement a macro WRITE_SYSREG_SZ which expands as follows: > -on arm64: WRITE_SYSREG > -on arm32: WRITE_SYSREG{32/64} > > As there are no other places in the code using these helpers > on arm64 - remove them. > > Signed-off-by: Michal Orzel <michal.orzel@arm.com> > --- > Changes since v1: > -implement WRITE_SYSREG_SZ instead of duplicating the TVM_REG > --- > xen/arch/arm/vcpreg.c | 12 +++++++++++- > xen/include/asm-arm/arm64/sysregs.h | 4 ---- > 2 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > index f0cdcc8a54..10c4846954 100644 > --- a/xen/arch/arm/vcpreg.c > +++ b/xen/arch/arm/vcpreg.c > @@ -47,6 +47,16 @@ > * > */ > > +#ifdef CONFIG_ARM_64 > +#define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG(val, sysreg) I think you want to cast to (uint##sz##_t) to avoid any surprise. I think... > +#else > +/* > + * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes > + * on the below macro to be defined like that as well. > + */ > +#define WRITE_SYSREG_SZ(sz, val, sysreg...) WRITE_SYSREG##sz(val, sysreg) > +#endif > + > /* The name is passed from the upper macro to workaround macro expansion. */ > #define TVM_REG(sz, func, reg...) \ > static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > @@ -55,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > bool cache_enabled = vcpu_has_cache_enabled(v); \ > \ > GUEST_BUG_ON(read); \ > - WRITE_SYSREG##sz(*r, reg); \ > + WRITE_SYSREG_SZ(sz, *r, reg); \ > \ > p2m_toggle_cache(v, cache_enabled); \ > \ > diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h > index 077fd95fb7..795901e1ba 100644 > --- a/xen/include/asm-arm/arm64/sysregs.h > +++ b/xen/include/asm-arm/arm64/sysregs.h > @@ -87,10 +87,6 @@ > > /* Access to system registers */ > > -#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name)) > - > -#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name) ... this was intended behavior but I used the wrong cast here. > - > #define WRITE_SYSREG64(v, name) do { \ > uint64_t _r = v; \ > asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ > -- Julien Grall
Hi Julien, On 09.07.2021 17:21, Julien Grall wrote: > Hi Michal, > > On 09/07/2021 13:40, Michal Orzel wrote: >> AArch64 system registers are 64bit whereas AArch32 ones >> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus >> we should get rid of helpers READ/WRITE_SYSREG32 >> in favour of using READ/WRITE_SYSREG. >> >> The last place in code making use of READ/WRITE_SYSREG32 >> on arm64 is in TVM_REG macro defining functions vreg_emulate_<register>. >> Implement a macro WRITE_SYSREG_SZ which expands as follows: >> -on arm64: WRITE_SYSREG >> -on arm32: WRITE_SYSREG{32/64} >> >> As there are no other places in the code using these helpers >> on arm64 - remove them. >> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >> --- >> Changes since v1: >> -implement WRITE_SYSREG_SZ instead of duplicating the TVM_REG >> --- >> xen/arch/arm/vcpreg.c | 12 +++++++++++- >> xen/include/asm-arm/arm64/sysregs.h | 4 ---- >> 2 files changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >> index f0cdcc8a54..10c4846954 100644 >> --- a/xen/arch/arm/vcpreg.c >> +++ b/xen/arch/arm/vcpreg.c >> @@ -47,6 +47,16 @@ >> * >> */ >> +#ifdef CONFIG_ARM_64 >> +#define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG(val, sysreg) > > I think you want to cast to (uint##sz##_t) to avoid any surprise. I think... > But the val will always be of type uint##sz##_t because it is passed from ... >> +#else >> +/* >> + * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes >> + * on the below macro to be defined like that as well. >> + */ >> +#define WRITE_SYSREG_SZ(sz, val, sysreg...) WRITE_SYSREG##sz(val, sysreg) >> +#endif >> + >> /* The name is passed from the upper macro to workaround macro expansion. */ >> #define TVM_REG(sz, func, reg...) \ >> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >> @@ -55,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ ... here(*r argument). So I do not think that such casting makes sense e.g uint##sz##_t foo; WRITE_SYSREG((uint##sz##_t)foo, bar); Also whatever type we will pass to WRITE_SYSREG, it will be casted to uint64_t ... >> bool cache_enabled = vcpu_has_cache_enabled(v); \ >> \ >> GUEST_BUG_ON(read); \ >> - WRITE_SYSREG##sz(*r, reg); \ >> + WRITE_SYSREG_SZ(sz, *r, reg); \ >> \ >> p2m_toggle_cache(v, cache_enabled); \ >> \ >> diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h >> index 077fd95fb7..795901e1ba 100644 >> --- a/xen/include/asm-arm/arm64/sysregs.h >> +++ b/xen/include/asm-arm/arm64/sysregs.h >> @@ -87,10 +87,6 @@ >> /* Access to system registers */ >> -#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name)) >> - >> -#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name) > > ... this was intended behavior but I used the wrong cast here. > >> - >> #define WRITE_SYSREG64(v, name) do { \ >> uint64_t _r = v; \ ...here >> asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ >> > Cheers, Michal
On 12/07/2021 07:26, Michal Orzel wrote: > Hi Julien, Hi Michal, > On 09.07.2021 17:21, Julien Grall wrote: >> Hi Michal, >> >> On 09/07/2021 13:40, Michal Orzel wrote: >>> AArch64 system registers are 64bit whereas AArch32 ones >>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus >>> we should get rid of helpers READ/WRITE_SYSREG32 >>> in favour of using READ/WRITE_SYSREG. >>> >>> The last place in code making use of READ/WRITE_SYSREG32 >>> on arm64 is in TVM_REG macro defining functions vreg_emulate_<register>. >>> Implement a macro WRITE_SYSREG_SZ which expands as follows: >>> -on arm64: WRITE_SYSREG >>> -on arm32: WRITE_SYSREG{32/64} >>> >>> As there are no other places in the code using these helpers >>> on arm64 - remove them. >>> >>> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >>> --- >>> Changes since v1: >>> -implement WRITE_SYSREG_SZ instead of duplicating the TVM_REG >>> --- >>> xen/arch/arm/vcpreg.c | 12 +++++++++++- >>> xen/include/asm-arm/arm64/sysregs.h | 4 ---- >>> 2 files changed, 11 insertions(+), 5 deletions(-) >>> >>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >>> index f0cdcc8a54..10c4846954 100644 >>> --- a/xen/arch/arm/vcpreg.c >>> +++ b/xen/arch/arm/vcpreg.c >>> @@ -47,6 +47,16 @@ >>> * >>> */ >>> +#ifdef CONFIG_ARM_64 >>> +#define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG(val, sysreg) >> >> I think you want to cast to (uint##sz##_t) to avoid any surprise. I think... >> > But the val will always be of type uint##sz##_t because it is passed from ... >>> +#else >>> +/* >>> + * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes >>> + * on the below macro to be defined like that as well. >>> + */ >>> +#define WRITE_SYSREG_SZ(sz, val, sysreg...) WRITE_SYSREG##sz(val, sysreg) >>> +#endif >>> + >>> /* The name is passed from the upper macro to workaround macro expansion. */ >>> #define TVM_REG(sz, func, reg...) \ >>> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >>> @@ -55,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > ... here(*r argument). > So I do not think that such casting makes sense e.g > uint##sz##_t foo; > WRITE_SYSREG((uint##sz##_t)foo, bar); I agree that this doesn't make sense for the *current* use. However, when writing code, we need to think how one could use it in the future... When I read the name, I would expect that if I call it with sz == 32, then bottom 32-bit value to be written and the top 32-bit zeroed. But this is not the case... You are relying on the developper and reviewer to notice that only 32-bit value should be passed when sz == 32. I am not particularly keen on relying on this when a simple cast could prevent any future misuse. Alternatively, I would be happy to consider checking the type of the value at build time. Cheers, -- Julien Grall
On 12.07.2021 10:33, Julien Grall wrote: > > > On 12/07/2021 07:26, Michal Orzel wrote: >> Hi Julien, > > Hi Michal, > >> On 09.07.2021 17:21, Julien Grall wrote: >>> Hi Michal, >>> >>> On 09/07/2021 13:40, Michal Orzel wrote: >>>> AArch64 system registers are 64bit whereas AArch32 ones >>>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus >>>> we should get rid of helpers READ/WRITE_SYSREG32 >>>> in favour of using READ/WRITE_SYSREG. >>>> >>>> The last place in code making use of READ/WRITE_SYSREG32 >>>> on arm64 is in TVM_REG macro defining functions vreg_emulate_<register>. >>>> Implement a macro WRITE_SYSREG_SZ which expands as follows: >>>> -on arm64: WRITE_SYSREG >>>> -on arm32: WRITE_SYSREG{32/64} >>>> >>>> As there are no other places in the code using these helpers >>>> on arm64 - remove them. >>>> >>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >>>> --- >>>> Changes since v1: >>>> -implement WRITE_SYSREG_SZ instead of duplicating the TVM_REG >>>> --- >>>> xen/arch/arm/vcpreg.c | 12 +++++++++++- >>>> xen/include/asm-arm/arm64/sysregs.h | 4 ---- >>>> 2 files changed, 11 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >>>> index f0cdcc8a54..10c4846954 100644 >>>> --- a/xen/arch/arm/vcpreg.c >>>> +++ b/xen/arch/arm/vcpreg.c >>>> @@ -47,6 +47,16 @@ >>>> * >>>> */ >>>> +#ifdef CONFIG_ARM_64 >>>> +#define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG(val, sysreg) >>> >>> I think you want to cast to (uint##sz##_t) to avoid any surprise. I think... >>> >> But the val will always be of type uint##sz##_t because it is passed from ... >>>> +#else >>>> +/* >>>> + * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes >>>> + * on the below macro to be defined like that as well. >>>> + */ >>>> +#define WRITE_SYSREG_SZ(sz, val, sysreg...) WRITE_SYSREG##sz(val, sysreg) >>>> +#endif >>>> + >>>> /* The name is passed from the upper macro to workaround macro expansion. */ >>>> #define TVM_REG(sz, func, reg...) \ >>>> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >>>> @@ -55,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >> ... here(*r argument). >> So I do not think that such casting makes sense e.g >> uint##sz##_t foo; >> WRITE_SYSREG((uint##sz##_t)foo, bar); > > I agree that this doesn't make sense for the *current* use. However, when writing code, we need to think how one could use it in the future... > > When I read the name, I would expect that if I call it with sz == 32, then bottom 32-bit value to be written and the top 32-bit zeroed. But this is not the case... > > You are relying on the developper and reviewer to notice that only 32-bit value should be passed when sz == 32. > > I am not particularly keen on relying on this when a simple cast could prevent any future misuse. Alternatively, I would be happy to consider checking the type of the value at build time. > > Cheers, > Ok, you are right that it does not cost much to add it, so let's stick with: #ifdef CONFIG_ARM_64 #define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG((uint##sz##_t)val, sysreg) #else I will push v3 with this change. Cheers, Michal
© 2016 - 2024 Red Hat, Inc.