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