Re: [RFC PATCH v1 03/12] Arm: GICv3: Enable vreg_reg64_* macros for AArch32

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

Title: The code you are modifying below is not GICv3 specific. I would 
suggest the following title:
xen/arm: vreg: Support vreg_reg64_* helpers on Aarch32

On 21/10/2022 16:31, 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, 64 bit is represented by unsigned long long. Thus, we need
> to change the prototype accordingly.
>
> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> ---
>   xen/arch/arm/include/asm/vreg.h | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/vreg.h b/xen/arch/arm/include/asm/vreg.h
> index f26a70d024..ac6e702c5c 100644
> --- a/xen/arch/arm/include/asm/vreg.h
> +++ b/xen/arch/arm/include/asm/vreg.h
> @@ -95,7 +95,7 @@ static inline bool vreg_emulate_sysreg(struct cpu_user_regs 
> *regs, union hsr hsr
>    * 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,
> +static inline register_t vreg_reg_extract(unsigned long long reg,
>                                             unsigned int offset,
>                                             enum dabt_size size)
>   {
> @@ -105,7 +105,7 @@ static inline register_t vreg_reg_extract(unsigned long reg,
>       return reg;
>   }
>   
> -static inline void vreg_reg_update(unsigned long *reg, register_t val,
> +static inline void vreg_reg_update(unsigned long long *reg, register_t val,
>                                      unsigned int offset,
>                                      enum dabt_size size)
>   {
> @@ -116,7 +116,7 @@ static inline void vreg_reg_update(unsigned long *reg, 
> register_t val,
>       *reg |= ((unsigned long)val & mask) << shift;
>   }
>   
> -static inline void vreg_reg_setbits(unsigned long *reg, register_t bits,
> +static inline void vreg_reg_setbits(unsigned long long *reg, register_t bits,
>                                       unsigned int offset,
>                                       enum dabt_size size)
>   {
> @@ -126,7 +126,7 @@ static inline void vreg_reg_setbits(unsigned long *reg, 
> register_t bits,
>       *reg |= ((unsigned long)bits & mask) << shift;
>   }
>   
> -static inline void vreg_reg_clearbits(unsigned long *reg, register_t bits,
> +static inline void vreg_reg_clearbits(unsigned long long *reg, register_t bits,
>                                         unsigned int offset,
>                                         enum dabt_size size)
>   {
> @@ -149,7 +149,7 @@ static inline void vreg_reg##sz##_update(uint##sz##_t *reg, 
>             \
>                                            register_t val,                \
>                                            const mmio_info_t *info)       \
>   {                                                                       \
> -    unsigned long tmp = *reg;                                           \
> +    unsigned long long tmp = *reg;                                      \
>                                                                           \
>       vreg_reg_update(&tmp, val, info->gpa & (offmask),                   \
>                       info->dabt.size);                                   \
> @@ -161,7 +161,7 @@ static inline void vreg_reg##sz##_setbits(uint##sz##_t 
> *reg,            \
>                                             register_t bits,              \
>                                             const mmio_info_t *info)      \
>   {                                                                       \
> -    unsigned long tmp = *reg;                                           \
> +    unsigned long long tmp = *reg;                                      \
>                                                                           \
>       vreg_reg_setbits(&tmp, bits, info->gpa & (offmask),                 \
>                        info->dabt.size);                                  \
> @@ -173,7 +173,7 @@ static inline void vreg_reg##sz##_clearbits(uint##sz##_t 
> *reg,          \
>                                               register_t bits,            \
>                                               const mmio_info_t *info)    \
>   {                                                                       \
> -    unsigned long tmp = *reg;                                           \
> +    unsigned long long tmp = *reg;                                      \
>                                                                           \
>       vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask),               \
>                          info->dabt.size);                                \
> @@ -181,15 +181,8 @@ static inline void vreg_reg##sz##_clearbits(uint##sz##_t 
> *reg,          \
>       *reg = tmp;                                                         \
>   }
>   
> -/*
> - * 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
> - */
The comment above explain why we never use uint64_t in the helpers 
above. IIRC, the compiler would end up to use 2 registers on AArch32 
even for the vreg_reg32_* helpers. I wanted to avoid that and would like 
like to today. Can you check the code generated?
For other options, I would consider to either:
  1) Fold vreg_reg_* in the macros.
  2) Write a separate vreg_reg64_*

My preference would be 1).

If we are planning to keep the code with "unsigned long long", then I 
think this should be addressed in the commit message.
> -#if BITS_PER_LONG == 64
> -VREG_REG_HELPERS(64, 0x7);
> -#endif
>   VREG_REG_HELPERS(32, 0x3);
> +VREG_REG_HELPERS(64, 0x7);
Regardless what I wrote above, the code movement seems to be unwarranted.
>   
>   #undef VREG_REG_HELPERS
>   
Cheers,

--
Julien Grall