Hi Luca,
> On 20 Apr 2023, at 09:43, Luca Fancellu <[email protected]> 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;
>
>
