Re: [PATCH 7/9] arm/time,vtimer: Get rid of READ/WRITE_SYSREG32

[Julien Grall <julien@xxxxxxx>]

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.
>
> Modify type of vtimer structure's member: ctl to register_t.
>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>   xen/arch/arm/time.c          | 28 ++++++++++++++--------------
>   xen/arch/arm/vtimer.c        | 10 +++++-----
>   xen/include/asm-arm/domain.h |  2 +-
>   3 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index b0021c2c69..e6c96eb90c 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -145,7 +145,7 @@ void __init preinit_xen_time(void)
>           preinit_acpi_xen_time();
>   
>       if ( !cpu_khz )
> -        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
> +        cpu_khz = (unsigned long)(READ_SYSREG(CNTFRQ_EL0) / 1000);
From the Arm Arm (DDI 0486 F.c), only the lower 32-bit represents the 
clock frequency. The rest are RES0 (IOW can have a different meaning in 
the future). So I think you want to mask it.
>   
>   static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
> @@ -347,7 +347,7 @@ bool vtimer_emulate(struct cpu_user_regs *regs, union hsr 
> hsr)
>   }
>   
>   static void vtimer_update_irq(struct vcpu *v, struct vtimer *vtimer,
> -                              uint32_t vtimer_ctl)
> +                              register_t vtimer_ctl)
>   {
>       bool level;
>   
> @@ -389,7 +389,7 @@ void vtimer_update_irqs(struct vcpu *v)
>        * but this requires reworking the arch timer to implement this.
>        */
>       vtimer_update_irq(v, &v->arch.virt_timer,
> -                      READ_SYSREG32(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
> +                      READ_SYSREG(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
AFAICT, CNTx_CTL_MASK is an unsigned int. This will not only mask IMASK 
but also the bits 32-63 for Armv8.
So I think you want to switch CTNx_CTL_MASK to return an unsigned long 
and explain it in the commit message.
Cheers,

--
Julien Grall