| Message ID | 20221031151326.22634-4-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 (v8R) | expand |
Hi Ayan, On 31/10/2022 16:13, Ayan Kumar Halder wrote: > > > In some situations (eg GICR_TYPER), the hypervior may need to emulate > 64bit registers in aarch32 mode. In such situations, the hypervisor may > need to read/modify the lower or upper 32 bits of the 64 bit register. > > In aarch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit > registers. > > The correct approach is to typecast 'mask' based on the size of register access > (ie uint32_t or uint64_t) instead of using 'unsigned long' as it will not > generate the correct mask for the upper 32 bits of a 64 bit register. > Also, 'val' needs to be typecasted so that it can correctly update the upper/ > lower 32 bits of a 64 bit register. > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > > Changes from :- > v1 - 1. Remove vreg_reg_extract(), vreg_reg_update(), vreg_reg_setbits() and > vreg_reg_clearbits(). Moved the implementation to vreg_reg##sz##_*. > 'mask' and 'val' is now using uint##sz##_t. > > xen/arch/arm/include/asm/vreg.h | 88 ++++++++------------------------- > 1 file changed, 20 insertions(+), 68 deletions(-) > > diff --git a/xen/arch/arm/include/asm/vreg.h b/xen/arch/arm/include/asm/vreg.h > index f26a70d024..122ea79b65 100644 > --- a/xen/arch/arm/include/asm/vreg.h > +++ b/xen/arch/arm/include/asm/vreg.h > @@ -89,107 +89,59 @@ static inline bool vreg_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr > * The check on the size supported by the register has to be done by > * the caller of vreg_regN_*. > * > - * vreg_reg_* should never be called directly. Instead use the vreg_regN_* > - * according to size of the emulated register > - * > * Note that the alignment fault will always be taken in the guest > * (see B3.12.7 DDI0406.b). > */ > -static inline register_t vreg_reg_extract(unsigned long reg, > - unsigned int offset, > - enum dabt_size size) > -{ > - reg >>= 8 * offset; > - reg &= VREG_REG_MASK(size); > - > - return reg; > -} > - > -static inline void vreg_reg_update(unsigned long *reg, register_t val, > - unsigned int offset, > - enum dabt_size size) > -{ > - unsigned long mask = VREG_REG_MASK(size); > - int shift = offset * 8; > - > - *reg &= ~(mask << shift); > - *reg |= ((unsigned long)val & mask) << shift; > -} > - > -static inline void vreg_reg_setbits(unsigned long *reg, register_t bits, > - unsigned int offset, > - enum dabt_size size) > -{ > - unsigned long mask = VREG_REG_MASK(size); > - int shift = offset * 8; > - > - *reg |= ((unsigned long)bits & mask) << shift; > -} > - > -static inline void vreg_reg_clearbits(unsigned long *reg, register_t bits, > - unsigned int offset, > - enum dabt_size size) > -{ > - unsigned long mask = VREG_REG_MASK(size); > - int shift = offset * 8; > - > - *reg &= ~(((unsigned long)bits & mask) << shift); > -} > > /* N-bit register helpers */ > #define VREG_REG_HELPERS(sz, offmask) \ > static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg, \ > const mmio_info_t *info)\ > { \ > - return vreg_reg_extract(reg, info->gpa & (offmask), \ > - info->dabt.size); \ > + unsigned int offset = info->gpa & (offmask); \ In all the other helpers you are also defining the variables to store shift and mask, no matter the number of uses. I know that this is a left over from the removed helpers, but since you are modifying the file you could improve consistency and define them here as well. > + \ > + reg >>= 8 * offset; \ > + reg &= VREG_REG_MASK(info->dabt.size); \ > + \ > + return reg; \ > } \ > \ > static inline void vreg_reg##sz##_update(uint##sz##_t *reg, \ > register_t val, \ > const mmio_info_t *info) \ > { \ > - unsigned long tmp = *reg; \ > - \ > - vreg_reg_update(&tmp, val, info->gpa & (offmask), \ > - info->dabt.size); \ > + unsigned int offset = info->gpa & (offmask); \ > + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ > + int shift = offset * 8; \ > \ > - *reg = tmp; \ > + *reg &= ~(mask << shift); \ > + *reg |= ((uint##sz##_t)val & mask) << shift; \ > } \ > \ > static inline void vreg_reg##sz##_setbits(uint##sz##_t *reg, \ > register_t bits, \ > const mmio_info_t *info) \ > { \ > - unsigned long tmp = *reg; \ > - \ > - vreg_reg_setbits(&tmp, bits, info->gpa & (offmask), \ > - info->dabt.size); \ > + unsigned int offset = info->gpa & (offmask); \ > + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ > + int shift = offset * 8; \ > \ > - *reg = tmp; \ > + *reg |= ((uint##sz##_t)bits & mask) << shift; \ > } \ > \ > static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg, \ > register_t bits, \ > const mmio_info_t *info) \ > { \ > - unsigned long tmp = *reg; \ > + unsigned int offset = info->gpa & (offmask); \ > + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ > + int shift = offset * 8; \ > \ > - vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask), \ > - info->dabt.size); \ > - \ > - *reg = tmp; \ > + *reg &= ~(((uint##sz##_t)bits & mask) << shift); \ > } > > -/* > - * 64 bits registers are only supported on platform with 64-bit long. > - * This is also allow us to optimize the 32 bit case by using > - * unsigned long rather than uint64_t > - */ > -#if BITS_PER_LONG == 64 > -VREG_REG_HELPERS(64, 0x7); > -#endif > VREG_REG_HELPERS(32, 0x3); > +VREG_REG_HELPERS(64, 0x7); No need for the movement. 64 should stay as it was before 32 and you should only remove the guards. > > #undef VREG_REG_HELPERS > > -- > 2.17.1 > > Apart from that, the change looks good. ~Michal
Hi, On 31/10/2022 15:13, Ayan Kumar Halder wrote: > In some situations (eg GICR_TYPER), the hypervior may need to emulate Typo: s/eg/e.g./ > 64bit registers in aarch32 mode. In such situations, the hypervisor may > need to read/modify the lower or upper 32 bits of the 64 bit register. > > In aarch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit > registers. > > The correct approach is to typecast 'mask' based on the size of register access > (ie uint32_t or uint64_t) instead of using 'unsigned long' as it will not > generate the correct mask for the upper 32 bits of a 64 bit register. > Also, 'val' needs to be typecasted so that it can correctly update the upper/ 'val' doesn't exist everywhere. But rather than explaining variable by variable what needs to be done, I would suggest something like the following: "While we could replace 'unsigned long' by 'uint64_t', it is not entirely clear whether a 32-bit compiler would not allocate register for the upper 32-bit. Therefore fold vreg_reg_* helper in the size specific one and use the appropriate type based on the size requested." This would also explain why we didn't do a straight replacement of "unsigned long" to "uint64_t". [...] > /* N-bit register helpers */ > #define VREG_REG_HELPERS(sz, offmask) \ > static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg, \ > const mmio_info_t *info)\ > { \ > - return vreg_reg_extract(reg, info->gpa & (offmask), \ > - info->dabt.size); \ > + unsigned int offset = info->gpa & (offmask); \ > + \ > + reg >>= 8 * offset; \ > + reg &= VREG_REG_MASK(info->dabt.size); \ > + \ > + return reg; \ > } \ > \ > static inline void vreg_reg##sz##_update(uint##sz##_t *reg, \ > register_t val, \ > const mmio_info_t *info) \ > { \ > - unsigned long tmp = *reg; \ > - \ > - vreg_reg_update(&tmp, val, info->gpa & (offmask), \ > - info->dabt.size); \ > + unsigned int offset = info->gpa & (offmask); \ > + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ > + int shift = offset * 8; \ As you fold it, please switch to "unsigned int". > \ > - *reg = tmp; \ > + *reg &= ~(mask << shift); \ > + *reg |= ((uint##sz##_t)val & mask) << shift; \ > } \ > \ > static inline void vreg_reg##sz##_setbits(uint##sz##_t *reg, \ > register_t bits, \ > const mmio_info_t *info) \ > { \ > - unsigned long tmp = *reg; \ > - \ > - vreg_reg_setbits(&tmp, bits, info->gpa & (offmask), \ > - info->dabt.size); \ > + unsigned int offset = info->gpa & (offmask); \ > + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ > + int shift = offset * 8; \ Same here. > \ > - *reg = tmp; \ > + *reg |= ((uint##sz##_t)bits & mask) << shift; \ > } \ > \ > static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg, \ > register_t bits, \ > const mmio_info_t *info) \ > { \ > - unsigned long tmp = *reg; \ > + unsigned int offset = info->gpa & (offmask); \ > + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ > + int shift = offset * 8; \ Same here. > \ > - vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask), \ > - info->dabt.size); \ > - \ > - *reg = tmp; \ > + *reg &= ~(((uint##sz##_t)bits & mask) << shift); \ > } > > -/* > - * 64 bits registers are only supported on platform with 64-bit long. > - * This is also allow us to optimize the 32 bit case by using > - * unsigned long rather than uint64_t > - */ > -#if BITS_PER_LONG == 64 > -VREG_REG_HELPERS(64, 0x7); > -#endif > VREG_REG_HELPERS(32, 0x3); > +VREG_REG_HELPERS(64, 0x7); > > #undef VREG_REG_HELPERS > Cheers,
On 02/11/2022 08:52, Michal Orzel wrote: >> /* N-bit register helpers */ >> #define VREG_REG_HELPERS(sz, offmask) \ >> static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg, \ >> const mmio_info_t *info)\ >> { \ >> - return vreg_reg_extract(reg, info->gpa & (offmask), \ >> - info->dabt.size); \ >> + unsigned int offset = info->gpa & (offmask); \ > In all the other helpers you are also defining the variables to store shift and mask, > no matter the number of uses. I know that this is a left over from the removed helpers, > but since you are modifying the file you could improve consistency and define them > here as well. Nack. We don't define extra local variable just for consistency. They are added in some places below to reduce the number of operation in one line. This is not necessary here. Cheers,
diff --git a/xen/arch/arm/include/asm/vreg.h b/xen/arch/arm/include/asm/vreg.h index f26a70d024..122ea79b65 100644 --- a/xen/arch/arm/include/asm/vreg.h +++ b/xen/arch/arm/include/asm/vreg.h @@ -89,107 +89,59 @@ static inline bool vreg_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr * The check on the size supported by the register has to be done by * the caller of vreg_regN_*. * - * vreg_reg_* should never be called directly. Instead use the vreg_regN_* - * according to size of the emulated register - * * Note that the alignment fault will always be taken in the guest * (see B3.12.7 DDI0406.b). */ -static inline register_t vreg_reg_extract(unsigned long reg, - unsigned int offset, - enum dabt_size size) -{ - reg >>= 8 * offset; - reg &= VREG_REG_MASK(size); - - return reg; -} - -static inline void vreg_reg_update(unsigned long *reg, register_t val, - unsigned int offset, - enum dabt_size size) -{ - unsigned long mask = VREG_REG_MASK(size); - int shift = offset * 8; - - *reg &= ~(mask << shift); - *reg |= ((unsigned long)val & mask) << shift; -} - -static inline void vreg_reg_setbits(unsigned long *reg, register_t bits, - unsigned int offset, - enum dabt_size size) -{ - unsigned long mask = VREG_REG_MASK(size); - int shift = offset * 8; - - *reg |= ((unsigned long)bits & mask) << shift; -} - -static inline void vreg_reg_clearbits(unsigned long *reg, register_t bits, - unsigned int offset, - enum dabt_size size) -{ - unsigned long mask = VREG_REG_MASK(size); - int shift = offset * 8; - - *reg &= ~(((unsigned long)bits & mask) << shift); -} /* N-bit register helpers */ #define VREG_REG_HELPERS(sz, offmask) \ static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg, \ const mmio_info_t *info)\ { \ - return vreg_reg_extract(reg, info->gpa & (offmask), \ - info->dabt.size); \ + unsigned int offset = info->gpa & (offmask); \ + \ + reg >>= 8 * offset; \ + reg &= VREG_REG_MASK(info->dabt.size); \ + \ + return reg; \ } \ \ static inline void vreg_reg##sz##_update(uint##sz##_t *reg, \ register_t val, \ const mmio_info_t *info) \ { \ - unsigned long tmp = *reg; \ - \ - vreg_reg_update(&tmp, val, info->gpa & (offmask), \ - info->dabt.size); \ + unsigned int offset = info->gpa & (offmask); \ + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ + int shift = offset * 8; \ \ - *reg = tmp; \ + *reg &= ~(mask << shift); \ + *reg |= ((uint##sz##_t)val & mask) << shift; \ } \ \ static inline void vreg_reg##sz##_setbits(uint##sz##_t *reg, \ register_t bits, \ const mmio_info_t *info) \ { \ - unsigned long tmp = *reg; \ - \ - vreg_reg_setbits(&tmp, bits, info->gpa & (offmask), \ - info->dabt.size); \ + unsigned int offset = info->gpa & (offmask); \ + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ + int shift = offset * 8; \ \ - *reg = tmp; \ + *reg |= ((uint##sz##_t)bits & mask) << shift; \ } \ \ static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg, \ register_t bits, \ const mmio_info_t *info) \ { \ - unsigned long tmp = *reg; \ + unsigned int offset = info->gpa & (offmask); \ + uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \ + int shift = offset * 8; \ \ - vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask), \ - info->dabt.size); \ - \ - *reg = tmp; \ + *reg &= ~(((uint##sz##_t)bits & mask) << shift); \ } -/* - * 64 bits registers are only supported on platform with 64-bit long. - * This is also allow us to optimize the 32 bit case by using - * unsigned long rather than uint64_t - */ -#if BITS_PER_LONG == 64 -VREG_REG_HELPERS(64, 0x7); -#endif VREG_REG_HELPERS(32, 0x3); +VREG_REG_HELPERS(64, 0x7); #undef VREG_REG_HELPERS
In some situations (eg GICR_TYPER), the hypervior may need to emulate 64bit registers in aarch32 mode. In such situations, the hypervisor may need to read/modify the lower or upper 32 bits of the 64 bit register. In aarch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit registers. The correct approach is to typecast 'mask' based on the size of register access (ie uint32_t or uint64_t) instead of using 'unsigned long' as it will not generate the correct mask for the upper 32 bits of a 64 bit register. Also, 'val' needs to be typecasted so that it can correctly update the upper/ lower 32 bits of a 64 bit register. Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> --- Changes from :- v1 - 1. Remove vreg_reg_extract(), vreg_reg_update(), vreg_reg_setbits() and vreg_reg_clearbits(). Moved the implementation to vreg_reg##sz##_*. 'mask' and 'val' is now using uint##sz##_t. xen/arch/arm/include/asm/vreg.h | 88 ++++++++------------------------- 1 file changed, 20 insertions(+), 68 deletions(-)