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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Luca,

> On 20 Apr 2023, at 09:43, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
> 
> 
>>>>> +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.
>> 
> 
> Hi Bertrand,
> 
> These are the changes I’m doing to this patch to address your comment, are 
> you ok with them?
> 
> diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
> index f0eab18dc384..1fef466ba0aa 100644
> --- a/xen/arch/arm/arm64/sve.c
> +++ b/xen/arch/arm/arm64/sve.c
> @@ -91,35 +91,35 @@ int sve_context_init(struct vcpu *v)
>     if ( !ctx )
>         return -ENOMEM;
> 
> -    v->arch.vfp.sve_context = ctx;
> +    /* Point to the end of Z0-Z31 memory, just before FFR memory */
> +    v->arch.vfp.sve_zreg_ctx_end = ctx +
> +        (sve_zreg_ctx_size(sve_vl_bits) / sizeof(uint64_t));
> 
>     return 0;
> }
> 
> void sve_context_free(struct vcpu *v)
> {
> -    XFREE(v->arch.vfp.sve_context);
> +    unsigned int sve_vl_bits = sve_decode_vl(v->domain->arch.sve_vl);
> +
> +    /* Point back to the beginning of Z0-Z31 + FFR memory */
> +    v->arch.vfp.sve_zreg_ctx_end = v->arch.vfp.sve_zreg_ctx_end -
> +        (sve_zreg_ctx_size(sve_vl_bits) / sizeof(uint64_t));

Please use a local variable here instead.

For the rest all good yes, it makes the save/restore code simpler :-)

Thanks
Bertrand

> +
> +    XFREE(v->arch.vfp.sve_zreg_ctx_end);
> }
> 
> 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));
> -
>     v->arch.zcr_el1 = READ_SYSREG(ZCR_EL1);
> 
> -    sve_save_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
> +    sve_save_ctx(v->arch.vfp.sve_zreg_ctx_end, v->arch.vfp.fpregs, 1);
> }
> 
> void sve_restore_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));
> -
>     WRITE_SYSREG(v->arch.zcr_el1, ZCR_EL1);
>     WRITE_SYSREG(v->arch.zcr_el2, ZCR_EL2);
> 
> -    sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
> +    sve_load_ctx(v->arch.vfp.sve_zreg_ctx_end, v->arch.vfp.fpregs, 1);
> }
> diff --git a/xen/arch/arm/include/asm/arm64/vfp.h 
> b/xen/arch/arm/include/asm/arm64/vfp.h
> index 8af714cb8ecc..4aa371e85d26 100644
> --- a/xen/arch/arm/include/asm/arm64/vfp.h
> +++ b/xen/arch/arm/include/asm/arm64/vfp.h
> @@ -13,10 +13,12 @@ struct vfp_state
>      */
>     uint64_t fpregs[64] __vfp_aligned;
>     /*
> -     * When SVE is enabled for the guest, sve_context contains memory to
> -     * save/restore Z0-Z31 registers and FFR.
> +     * When SVE is enabled for the guest, sve_zreg_ctx_end points to memory
> +     * where Z0-Z31 registers and FFR can be saved/restored, it points at the
> +     * end of the Z0-Z31 space and at the beginning of the FFR space, it's 
> done
> +     * like that to ease the save/restore assembly operations.
>      */
> -    uint64_t *sve_context;
> +    uint64_t *sve_zreg_ctx_end;
>     register_t fpcr;
>     register_t fpexc32_el2;
>     register_t fpsr;
> 
>