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

[Julien Grall <julien@xxxxxxx>]

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.

> +     */
> +    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.

> +    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.

>       register_t cptr_el2;
>       register_t hcr_el2;
>       register_t mdcr_el2;
Cheers,

--
Julien Grall