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

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 14/04/2023 12:07, Luca Fancellu wrote:
> > On 13 Apr 2023, at 20:52, Julien Grall <julien@xxxxxxx> wrote:
> >
> > 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.
> Hi Julien,
>
> Yes that’s true, we would like to prevent who is using hyper calls to have guests with 
> SVE but 32 bits, I think that with this check it’s possible to avoid them:
>
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 0de89b42c448..b7189e8dbbb5 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -43,6 +43,9 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct 
> domain *d,
>           case 32:
>               if ( !cpu_has_el1_32 )
>                   return -EINVAL;
> +            /* SVE is not supported for 32 bit domain */
> +            if ( is_sve_domain(d) )
> +                return -EOPNOTSUPP;
>               return switch_mode(d, DOMAIN_32BIT);
>           case 64:
>               return switch_mode(d, DOMAIN_64BIT);
>
> It’s a bit late in the guest creation, but we don’t have the domain type 
> information before, this check together with the check above in construct_domain would
> be enough.
>
> What do you think?
I would be OK with this approach for now. In the longer term, we 
probably want to consider to set the mode when the domain is created 
because this can't change at runtime.
Cheers,

--
Julien Grall