| Message ID | 20210709124035.5520-1-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros | expand |
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)); \ >
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,
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
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)); \
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(-)