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

[Julien Grall <julien@xxxxxxx>]

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.
> > Can we move this somewhere else to avoid adding extra padding? Also shouldn't 
> > this be protected with #ifdef CONFIG_ARM_64 to make clear this is not supported 
> > on Xen 32-bit?
> Yes, I’ll move it and protect with CONFIG_ARM_64, is it ok for you if I move it 
> after:
>
> /* Monitor options */
> struct {
>      uint8_t privileged_call_enabled : 1;
> } monitor;
Please check the padding with "pahole". If possible, it would be better 
to re-use an existing one.
Cheers,

--
Julien Grall