Re: [PATCH v5 07/12] xen: enable Dom0 to use SVE feature

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 20/04/2023 09:46, Luca Fancellu wrote:
> > > > > > +int __init sve_sanitize_vl_param(int val, unsigned int *out)
> > > > > > +{
> > > > > > +    /*
> > > > > > +     * Negative SVE parameter value means to use the maximum supported
> > > > > > +     * vector length, otherwise if a positive value is provided, check if the
> > > > > > +     * vector length is a multiple of 128 and not bigger than the maximum value
> > > > > > +     * 2048
> > > > > > +     */
> > > > > > +    if ( val < 0 )
> > > > > > +        *out = get_sys_vl_len();
> > > > > > +    else if ( ((val % SVE_VL_MULTIPLE_VAL) == 0) && (val <= SVE_VL_MAX_BITS) )
> > > > > > +        *out = val;
> > > > > Shouldn't you also check if it is not greater than the maximum vector length ?
> > > > I don’t understand, I am checking that the value is below or equal to 
> > > > SVE_VL_MAX_BITS,
> > > > If you mean if it should be checked also against the maximum supported length 
> > > > by the platform,
> > > > I think this is not the right place, the check is already in 
> > > > arch_sanitise_domain_config(), introduced
> > > > in patch #2
> > > If this is not the right place to check it then why checking the rest here ?
> > >
> > >  From a user or a developer point of view I would expect the validity of the 
> > > input to be checked only
> > > in one place.
> > > If here is not the place for that it is ok but then i would check everything in 
> > > arch_sanitise_domain_config
> > > (multiple, min and supported) instead of doing it partly in 2 places.
> > Ok, given the way we encoded the value in xen_domctl_createdomain structure, we 
> > have that the value takes
> > very little space, but a small issue is that when we encode it, we are dividing 
> > it by 128, which is fine for user params
> > that are multiple of 128, but it’s less fine if the user passes “129”.
> >
> > To overcome this issue we are checking the value when it is not already 
> > encoded. Now, thinking about it, the check
> > "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value 
> > is above, then in arch_sanitise_domain_config
> > we will hit the top limit of the platform maximum VL.
> >
> > 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 )
> >     {
> >         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
> >                 config->flags);
> >         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;
> >         }
> >     }
> >    [...]
> >
> > Now, I understand your point, we could check everything in 
> > sve_sanitize_vl_param(), but it would leave a problem
> > for domains created by hypercalls if I am not wrong.
> >
> > What do you think?
> I thought about that and another possibility is to store “sve_vl” as uint16_t 
> inside struct xen_arch_domainconfig, and
> check it inside arch_sanitise_domain_config() for it to be mod 128 and less 
> than the max supported VL, this will
> allow to have all the checks in one place, taking a bit more space, anyway we 
> would take the space from the implicit
> padding as this is the current status:
Sorry, I am having trouble to follow the discussion. If you are checking 
the value in arch_sanitise_domain_config(), then why do you need to take 
more space in arch_domain?
Cheers,

--
Julien Grall