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 <ayank...@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
