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

[Luca Fancellu <Luca.Fancellu@arm.com>]

>>>> +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));
+
+    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;