Re: [PATCH v5 05/12] arm/sve: save/restore SVE context switch

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Luca,

> On 19 Apr 2023, at 09:09, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
>> On 18 Apr 2023, at 13:40, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Luca,
>> 
>>> 
>>> diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
>>> index 78f7482619da..5485648850a0 100644
>>> --- a/xen/arch/arm/arm64/sve.c
>>> +++ b/xen/arch/arm/arm64/sve.c
>>> @@ -5,14 +5,29 @@
>>> * Copyright (C) 2022 ARM Ltd.
>>> */
>>> 
>>> -#include <xen/types.h>
>>> -#include <asm/cpufeature.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/sizes.h>
>>> #include <asm/arm64/sve.h>
>>> -#include <asm/arm64/sysregs.h>
>>> -#include <asm/processor.h>
>>> -#include <asm/system.h>
>>> 
>>> extern unsigned int sve_get_hw_vl(void);
>>> +extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr);
>>> +extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs,
>>> +                         int restore_ffr);
>>> +
>>> +static inline unsigned int sve_zreg_ctx_size(unsigned int vl)
>>> +{
>>> +    /*
>>> +     * Z0-31 registers size in bytes is computed from VL that is in bits, 
>>> so VL
>>> +     * in bytes is VL/8.
>>> +     */
>>> +    return (vl / 8U) * 32U;
>>> +}
>>> +
>>> +static inline unsigned int sve_ffrreg_ctx_size(unsigned int vl)
>>> +{
>>> +    /* FFR register size is VL/8, which is in bytes (VL/8)/8 */
>>> +    return (vl / 64U);
>>> +}
>>> 
>>> register_t compute_max_zcr(void)
>>> {
>>> @@ -60,3 +75,46 @@ unsigned int get_sys_vl_len(void)
>>>   return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
>>>           SVE_VL_MULTIPLE_VAL;
>>> }
>>> +
>>> +int sve_context_init(struct vcpu *v)
>>> +{
>>> +    unsigned int sve_vl_bits = sve_decode_vl(v->domain->arch.sve_vl);
>>> +    uint64_t *ctx = _xzalloc(sve_zreg_ctx_size(sve_vl_bits) +
>>> +                             sve_ffrreg_ctx_size(sve_vl_bits),
>>> +                             L1_CACHE_BYTES);
>>> +
>>> +    if ( !ctx )
>>> +        return -ENOMEM;
>>> +
>>> +    v->arch.vfp.sve_context = ctx;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +void sve_context_free(struct vcpu *v)
>>> +{
>>> +    xfree(v->arch.vfp.sve_context);
>>> +}
>>> +
>>> +void sve_save_state(struct vcpu *v)
>>> +{
>>> +    unsigned int sve_vl_bits = sve_decode_vl(v->domain->arch.sve_vl);
>>> +    uint64_t *sve_ctx_zreg_end = v->arch.vfp.sve_context +
>>> +            (sve_zreg_ctx_size(sve_vl_bits) / sizeof(uint64_t));
>> 
>> You do quite some computation here for something which does not change
>> during the life of the VM.
>> Could we save the context_end in the vcpu instead and just do this
>> computation on init and free only ?
> 
> Yes sure, would you be ok to have it in struct vfp_state?

Yes definitely i would store it instead of the current sve_context.

> 
>> 
>>> 
>>> #endif /* _ARM_ARM64_SVE_H */
>>> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h 
>>> b/xen/arch/arm/include/asm/arm64/sysregs.h
>>> index 4cabb9eb4d5e..3fdeb9d8cdef 100644
>>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>>> @@ -88,6 +88,9 @@
>>> #ifndef ID_AA64ISAR2_EL1
>>> #define ID_AA64ISAR2_EL1            S3_0_C0_C6_2
>>> #endif
>>> +#ifndef ZCR_EL1
>>> +#define ZCR_EL1                     S3_0_C1_C2_0
>>> +#endif
>>> 
>> 
>> What about ZCR_EL2 ?
> 
> It was introduced in patch #1 because we use it in register_t 
> compute_max_zcr(void)
> 

Sorry i missed that.

Cheers
Bertrand

> 
>