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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 25 May 2023, at 10:09, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 23/05/2023 08:43, Luca Fancellu wrote:
>> +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;
>> +
>> +    /*
>> +     * Point to the end of Z0-Z31 memory, just before FFR memory, to be 
>> kept in
>> +     * sync with sve_context_free()
> 
> Nit: Missing a full stop.

I’ll fix

> 
>> +     */
>> +    v->arch.vfp.sve_zreg_ctx_end = ctx +
>> +        (sve_zreg_ctx_size(sve_vl_bits) / sizeof(uint64_t));
>> +
>> +    v->arch.zcr_el2 = vl_to_zcr(sve_vl_bits);
>> +
>> +    return 0;
>> +}
>> +
>> +void sve_context_free(struct vcpu *v)
>> +{
>> +    unsigned int sve_vl_bits;
>> +
>> +    if ( v->arch.vfp.sve_zreg_ctx_end )
>> +        return;
>> +
>> +    sve_vl_bits = sve_decode_vl(v->domain->arch.sve_vl);
>> +
>> +    /*
>> +    * Point to the end of Z0-Z31 memory, just before FFR memory, to be kept
>> +    * in sync with sve_context_init()
>> +    */
> 
> The spacing looks a bit odd in this comment. Did you miss an extra space?
> 
> Also, I notice this comment is the exact same as on top as 
> sve_context_init(). I think this is a bit misleading because the logic is 
> different. I would suggest the following:
> 
> "Currently points to the end of Z0-Z31 memory which is not the start of the 
> buffer. To be kept in sync with the sve_context_init()."
> 
> Lastly, nit: Missing a full stop.

Ok I’ll change it

> 
>> +    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);
>> +}
>> +
> 
> [...]
> 
>> diff --git a/xen/arch/arm/include/asm/arm64/vfp.h 
>> b/xen/arch/arm/include/asm/arm64/vfp.h
>> index e6e8c363bc16..4aa371e85d26 100644
>> --- a/xen/arch/arm/include/asm/arm64/vfp.h
>> +++ b/xen/arch/arm/include/asm/arm64/vfp.h
>> @@ -6,7 +6,19 @@
>>    struct vfp_state
>>  {
>> +    /*
>> +     * When SVE is enabled for the guest, fpregs memory will be used to
>> +     * save/restore P0-P15 registers, otherwise it will be used for the 
>> V0-V31
>> +     * registers.
>> +     */
>>      uint64_t fpregs[64] __vfp_aligned;
>> +    /*
>> +     * 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_zreg_ctx_end;
> 
> Sorry I only noticed now. But shouldn't this be protected with #ifdef 
> CONFIG_SVE? Same...
> 
>>      register_t fpcr;
>>      register_t fpexc32_el2;
>>      register_t fpsr;
>> diff --git a/xen/arch/arm/include/asm/domain.h 
>> b/xen/arch/arm/include/asm/domain.h
>> index 331da0f3bcc3..814652d92568 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -195,6 +195,8 @@ struct arch_vcpu
>>      register_t tpidrro_el0;
>>        /* HYP configuration */
>> +    register_t zcr_el1;
>> +    register_t zcr_el2;
> 
> ... here.

Sure I can protect them. It was done on purpose before to avoid ifdefs but I 
think saving space
is better here and also there won’t be any use of them when the config is off.


> 
>>      register_t cptr_el2;
>>      register_t hcr_el2;
>>      register_t mdcr_el2;
> 
> Cheers,
> 
> -- 
> Julien Grall