> On 25 May 2023, at 10:09, Julien Grall <jul...@xen.org> 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
