| Message ID | 20210420070853.8918-10-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm64: Get rid of READ/WRITE_SYSREG32 | expand |
Hi Michal, On 20/04/2021 08:08, 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. > We should also use register_t type when reading sysregs > which can correspond to uint64_t or uint32_t. > Even though many AArch64 sysregs have upper 32bit reserved > it does not mean that they can't be widen in the future. As a general comment, all your commit message contains information about the goal (which is great). But they don't go much in details about the actual changes. I have tried to point out where I think those details would be helpful. > > As there are no other places in the code using READ/WRITE_SYSREG32, > remove the helper macros. > > Signed-off-by: Michal Orzel <michal.orzel@arm.com> > --- > xen/arch/arm/vcpreg.c | 16 ++++++++++++++++ > xen/include/asm-arm/arm64/sysregs.h | 5 ----- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > index c7f516ee0a..6cb7f3108c 100644 > --- a/xen/arch/arm/vcpreg.c > +++ b/xen/arch/arm/vcpreg.c > @@ -48,6 +48,7 @@ > */ > > /* The name is passed from the upper macro to workaround macro expansion. */ > +#ifdef CONFIG_ARM_32 > #define TVM_REG(sz, func, reg...) \ > static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > { \ > @@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > \ > return true; \ > } > +#else /* CONFIG_ARM_64 */ > +#define TVM_REG(sz, func, reg...) \ > +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ > +{ \ > + struct vcpu *v = current; \ > + bool cache_enabled = vcpu_has_cache_enabled(v); \ > + \ > + GUEST_BUG_ON(read); \ > + WRITE_SYSREG(*r, reg); \ > + \ > + p2m_toggle_cache(v, cache_enabled); \ > + \ > + return true; \ > +} > +#endif It would be nice if this change can be explained in the commit message. However, I think we can avoid the duplication by aliasing TVM_REG32() to TVM_REG64() on Arm64. > > #define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg) > #define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg) > diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h > index 077fd95fb7..83a1157ac4 100644 > --- a/xen/include/asm-arm/arm64/sysregs.h > +++ b/xen/include/asm-arm/arm64/sysregs.h > @@ -86,11 +86,6 @@ > #endif > > /* 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)); \ > Cheers,
Hi Jilien, On 21.04.2021 21:16, Julien Grall wrote: > Hi Michal, > > On 20/04/2021 08:08, 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. >> We should also use register_t type when reading sysregs >> which can correspond to uint64_t or uint32_t. >> Even though many AArch64 sysregs have upper 32bit reserved >> it does not mean that they can't be widen in the future. > > As a general comment, all your commit message contains information about the goal (which is great). But they don't go much in details about the actual changes. I have tried to point out where I think those details would be helpful. > >> >> As there are no other places in the code using READ/WRITE_SYSREG32, >> remove the helper macros. >> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >> --- >> xen/arch/arm/vcpreg.c | 16 ++++++++++++++++ >> xen/include/asm-arm/arm64/sysregs.h | 5 ----- >> 2 files changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >> index c7f516ee0a..6cb7f3108c 100644 >> --- a/xen/arch/arm/vcpreg.c >> +++ b/xen/arch/arm/vcpreg.c >> @@ -48,6 +48,7 @@ >> */ >> /* The name is passed from the upper macro to workaround macro expansion. */ >> +#ifdef CONFIG_ARM_32 >> #define TVM_REG(sz, func, reg...) \ >> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >> { \ >> @@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >> \ >> return true; \ >> } >> +#else /* CONFIG_ARM_64 */ >> +#define TVM_REG(sz, func, reg...) \ >> +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >> +{ \ >> + struct vcpu *v = current; \ >> + bool cache_enabled = vcpu_has_cache_enabled(v); \ >> + \ >> + GUEST_BUG_ON(read); \ >> + WRITE_SYSREG(*r, reg); \ >> + \ >> + p2m_toggle_cache(v, cache_enabled); \ >> + \ >> + return true; \ >> +} >> +#endif > > It would be nice if this change can be explained in the commit message. However, I think we can avoid the duplication by aliasing TVM_REG32() to TVM_REG64() on Arm64. > Unfortunately we cannot. This is the only working solution for now. If we do the alias we will get many errors due to incompatbile pointer type in vreg_emulate_*. Without a proper change in this functions we can't do that. I will modify it once I start working on vreg_emulate topic but it requires more investigation. For now I'd suggest to keep it as it is + explain the change in the commit. I will push v2 soon. >> #define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg) >> #define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg) >> diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h >> index 077fd95fb7..83a1157ac4 100644 >> --- a/xen/include/asm-arm/arm64/sysregs.h >> +++ b/xen/include/asm-arm/arm64/sysregs.h >> @@ -86,11 +86,6 @@ >> #endif >> /* 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)); \ >> > > Cheers, > Cheers, Michal
On 27/04/2021 08:16, Michal Orzel wrote: > Hi Jilien, Hi, > On 21.04.2021 21:16, Julien Grall wrote: >> Hi Michal, >> >> On 20/04/2021 08:08, 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. >>> We should also use register_t type when reading sysregs >>> which can correspond to uint64_t or uint32_t. >>> Even though many AArch64 sysregs have upper 32bit reserved >>> it does not mean that they can't be widen in the future. >> >> As a general comment, all your commit message contains information about the goal (which is great). But they don't go much in details about the actual changes. I have tried to point out where I think those details would be helpful. >> >>> >>> As there are no other places in the code using READ/WRITE_SYSREG32, >>> remove the helper macros. >>> >>> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >>> --- >>> xen/arch/arm/vcpreg.c | 16 ++++++++++++++++ >>> xen/include/asm-arm/arm64/sysregs.h | 5 ----- >>> 2 files changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c >>> index c7f516ee0a..6cb7f3108c 100644 >>> --- a/xen/arch/arm/vcpreg.c >>> +++ b/xen/arch/arm/vcpreg.c >>> @@ -48,6 +48,7 @@ >>> */ >>> /* The name is passed from the upper macro to workaround macro expansion. */ >>> +#ifdef CONFIG_ARM_32 >>> #define TVM_REG(sz, func, reg...) \ >>> static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >>> { \ >>> @@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >>> \ >>> return true; \ >>> } >>> +#else /* CONFIG_ARM_64 */ >>> +#define TVM_REG(sz, func, reg...) \ >>> +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ >>> +{ \ >>> + struct vcpu *v = current; \ >>> + bool cache_enabled = vcpu_has_cache_enabled(v); \ >>> + \ >>> + GUEST_BUG_ON(read); \ >>> + WRITE_SYSREG(*r, reg); \ >>> + \ >>> + p2m_toggle_cache(v, cache_enabled); \ >>> + \ >>> + return true; \ >>> +} >>> +#endif >> >> It would be nice if this change can be explained in the commit message. However, I think we can avoid the duplication by aliasing TVM_REG32() to TVM_REG64() on Arm64. >> > Unfortunately we cannot. This is the only working solution for now. > If we do the alias we will get many errors due to incompatbile pointer type in vreg_emulate_*. > Without a proper change in this functions we can't do that. Right, so the prototype needs to stay the same. How about provide a wrapper WRITE_SYSREG_SZ(sz, val, sysreg) internal to vcpreg.c? On 32-bit it would expand to WRITE_SYSREG##sz(val, sysreg) and on 64-bit, it would expand to WRITE_SYSREG(). > I will modify it once I start working on vreg_emulate topic but it requires more investigation. While it would be great to get rid of {READ, WRITE}_SYSREG32, I don't want to duplicate the code (even temporarily) just for the sake for removing the helpers. If the new proposal I suggested above doesn't work, then I would strongly prefer to keep the existing code until vreg_emulate_* is reworked. Cheers,
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c index c7f516ee0a..6cb7f3108c 100644 --- a/xen/arch/arm/vcpreg.c +++ b/xen/arch/arm/vcpreg.c @@ -48,6 +48,7 @@ */ /* The name is passed from the upper macro to workaround macro expansion. */ +#ifdef CONFIG_ARM_32 #define TVM_REG(sz, func, reg...) \ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ { \ @@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ \ return true; \ } +#else /* CONFIG_ARM_64 */ +#define TVM_REG(sz, func, reg...) \ +static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \ +{ \ + struct vcpu *v = current; \ + bool cache_enabled = vcpu_has_cache_enabled(v); \ + \ + GUEST_BUG_ON(read); \ + WRITE_SYSREG(*r, reg); \ + \ + p2m_toggle_cache(v, cache_enabled); \ + \ + return true; \ +} +#endif #define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg) #define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg) diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h index 077fd95fb7..83a1157ac4 100644 --- a/xen/include/asm-arm/arm64/sysregs.h +++ b/xen/include/asm-arm/arm64/sysregs.h @@ -86,11 +86,6 @@ #endif /* 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. We should also use register_t type when reading sysregs which can correspond to uint64_t or uint32_t. Even though many AArch64 sysregs have upper 32bit reserved it does not mean that they can't be widen in the future. As there are no other places in the code using READ/WRITE_SYSREG32, remove the helper macros. Signed-off-by: Michal Orzel <michal.orzel@arm.com> --- xen/arch/arm/vcpreg.c | 16 ++++++++++++++++ xen/include/asm-arm/arm64/sysregs.h | 5 ----- 2 files changed, 16 insertions(+), 5 deletions(-)