Re: [PATCH v5 02/12] xen/arm: add SVE vector length field to the domain

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 13/04/2023 15:05, Luca Fancellu wrote:
> > On 13 Apr 2023, at 14:30, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 13/04/2023 14:24, Luca Fancellu wrote:
> > > Hi Julien,
> > Hi Luca,
> >
> > > > >   @@ -594,6 +597,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 = sve_decode_vl(config->arch.sve_vl);
> > > > >         if ( (config->flags & ~flags_optional) != flags_required )
> > > > >       {
> > > > > @@ -602,6 +606,26 @@ 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 = get_sys_vl_len();
> > > > > +
> > > > > +        if ( !zcr_max_bits )
> > > > > +        {
> > > > > +            dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
> > > > > +            return -EINVAL;
> > > > > +        }
> > > > > +
> > > > > +        if ( sve_vl_bits > zcr_max_bits )
> > > > > +        {
> > > > > +            dprintk(XENLOG_INFO,
> > > > > +                    "Requested SVE vector length (%u) > supported length 
> > > > > (%u)\n",
> > > > > +                    sve_vl_bits, zcr_max_bits);
> > > > > +            return -EINVAL;
> > > > > +        }
> > > > Is SVE supported for 32-bit guest? If not, then you should had a check here to 
> > > > prevent the creation of the domain if sve_vl_bits is set.
> > > No SVE is not supported for 32 bit guests, here I think we will get “SVE is 
> > > unsupported on this machine” because get_sys_vl_len() will return 0.
> >  From my understanding, get_sys_vl_len() will return the len supported by the 
> > hosts. So if you run a 32-bit guest on top of a 64-bit hosts, then I believe 
> > get_sys_vl_len() will be non-zero.
> Yes you are right, I realise that I need the domain type information and I 
> can’t have it in arch_sanitise_domain_config, however they might have sense 
> there, and I can do a check
> like this afterwards:
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c1f0d1d78431..ce1235c25769 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3694,6 +3694,12 @@ static int __init construct_domain(struct domain *d, 
> struct kernel_info *kinfo)
>           return -EINVAL;
>       }
>   
> +    if ( d->arch.sve_vl && (kinfo->type == DOMAIN_32BIT) )
> +    {
> +        printk("SVE is not available for 32-bit domain\n");
> +        return -EINVAL;
> +    }
> +
>       if ( is_64bit_domain(d) )
>           vcpu_switch_to_aarch64_mode(v);
>
> Would it be ok for you?
construct_domain() is only going to be used for domains created by Xen. 
You would need the same check for the ones created by the toolstack.
Do you need to know the SVE length when the domain is created? If not, 
then I would suggest to create a new domctl that would be called after 
we switch the domain to 32-bit.
Cheers,

--
Julien Grall