Re: [RFC PATCH 2/8] xen/arm: add sve_vl_bits field to domain

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>>> index 8ea3843ea8e8..27f38729302b 100644
>>>> --- a/xen/arch/arm/domain.c
>>>> +++ b/xen/arch/arm/domain.c
>>>> @@ -13,6 +13,7 @@
>>>>  #include <xen/wait.h>
>>>>    #include <asm/alternative.h>
>>>> +#include <asm/arm64/sve.h>
>>>>  #include <asm/cpuerrata.h>
>>>>  #include <asm/cpufeature.h>
>>>>  #include <asm/current.h>
>>>> @@ -183,6 +184,11 @@ static void ctxt_switch_to(struct vcpu *n)
>>>>        WRITE_SYSREG(n->arch.cptr_el2, CPTR_EL2);
>>>>  +#ifdef CONFIG_ARM64_SVE
>>>> +    if ( is_sve_domain(n->domain) )
>>>> +        WRITE_SYSREG(n->arch.zcr_el2, ZCR_EL2);
>>>> +#endif
>>> 
>>> I would actually expect that is_sve_domain() returns false when the SVE is 
>>> not enabled. So can we simply remove the #ifdef?
>> I was tricked by it too, the arm32 build will fail because it doesn’t know 
>> ZCR_EL2
> 
> Ok. In which case, I think we should move the call to sve_restore_state().

Ok for me, I will move the zcr_el2 introduction together with the context 
switch code introduced by the patch
later.

> 
>>> 
>>>> +
>>>>      /* VFP */
>>>>      vfp_restore_state(n);
>>>>  @@ -551,6 +557,11 @@ int arch_vcpu_create(struct vcpu *v)
>>>>      v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
>>>>        v->arch.cptr_el2 = get_default_cptr_flags();
>>>> +    if ( is_sve_domain(v->domain) )
>>>> +    {
>>>> +        v->arch.cptr_el2 &= ~HCPTR_CP(8);
>>>> +        v->arch.zcr_el2 = vl_to_zcr(v->domain->arch.sve_vl_bits);
>>>> +    }
>>>>        v->arch.hcr_el2 = get_default_hcr_flags();
>>>>  @@ -595,6 +606,7 @@ int arch_sanitise_domain_config(struct 
>>>> xen_domctl_createdomain *config)
>>>>      unsigned int max_vcpus;
>>>>      unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | 
>>>> XEN_DOMCTL_CDF_hap);
>>>>      unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
>>>> XEN_DOMCTL_CDF_vpmu);
>>>> +    unsigned int sve_vl_bits = config->arch.sve_vl_bits;
>>>>        if ( (config->flags & ~flags_optional) != flags_required )
>>>>      {
>>>> @@ -603,6 +615,36 @@ int arch_sanitise_domain_config(struct 
>>>> xen_domctl_createdomain *config)
>>>>          return -EINVAL;
>>>>      }
>>>>  +    /* Check feature flags */
>>>> +    if ( sve_vl_bits > 0 ) {
>>>> +        unsigned int zcr_max_bits;
>>>> +
>>>> +        if ( !cpu_has_sve )
>>>> +        {
>>>> +            dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
>>>> +            return -EINVAL;
>>>> +        }
>>>> +        else if ( !is_vl_valid(sve_vl_bits) )
>>>> +        {
>>>> +            dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
>>>> +                    sve_vl_bits);
>>>> +            return -EINVAL;
>>>> +        }
>>>> +        /*
>>>> +         * get_sys_vl_len() is the common safe value among all cpus, so 
>>>> if the
>>>> +         * value specified by the user is above that value, use the safe 
>>>> value
>>>> +         * instead.
>>>> +         */
>>>> +        zcr_max_bits = get_sys_vl_len();
>>>> +        if ( sve_vl_bits > zcr_max_bits )
>>>> +        {
>>>> +            config->arch.sve_vl_bits = zcr_max_bits;
>>>> +            dprintk(XENLOG_INFO,
>>>> +                    "SVE vector length lowered to %u, safe value among 
>>>> CPUs\n",
>>>> +                    zcr_max_bits);
>>>> +        }
>>> 
>>> I don't think Xen should "downgrade" the value. Instead, this should be the 
>>> decision from the tools. So we want to find a different way to reproduce 
>>> the value (Andrew may have some ideas here as he was looking at it).
>> Can you explain this in more details?
> 
> I would extend XEN_SYSCTL_physinfo to return the maximum SVE vecto length 
> supported by the Hardware.
> 
> Libxl would then read the value and compare to what the user requested. This 
> would then be up to the toolstack to decide what to do.

Sounds good, looking into struct xen_sysctl_physinfo, seems that I might be the 
first user of the arch_capabilities field
(as well as introducing a new one for the VL value), where can I put the define 
for the arch_capabilities flag?
Is it ok inside sysctl.h something along these lines: 

#define XEN_SYSCTL_PHYSCAP_ARM_SVE (1u << 0)

or

#define XEN_SYSCTL_PHYSCAP_ARM_SVE (1u)

And, is it ok to put the VL value in the struct xen_sysctl_physinfo even if 
it’s just for arm64?


> 
>> By the tools you mean xl?
> 
> I think libxl should do strict checking. If we also want to reduce what the 
> user requested, then this part should be in xl.
> 
>> This would be ok for DomUs, but how to do it for Dom0?
> Then we should fail to create dom0 and say the vector-length requested is not 
> supported.

Fine for me

> 
> Cheers,
> 
> -- 
> Julien Grall