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

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 20/04/2023 13:43, Luca Fancellu wrote:
> > On 20 Apr 2023, at 13:29, Julien Grall <julien@xxxxxxx> wrote:
> >
> > 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:
> Hi Julien,
>
> > 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?
> Yes I am checking the value in arch_sanitise_domain_config, the value checked 
> is from arch_domain and it is the vector length divided by 128, so an encoded 
> value.
I think this is where the disconnect is coming from. 
arch_sanitise_domain_config() doesn't use struct arch_domain because the 
domain itself is not yet allocated. Instead, it is using 
xen_arch_domainconfig.
I care less about the increase of xen_arch_domainconfig because this is 
a one off structure.
> Bertrand was puzzled by the fact that I also put a check in sve_sanitize_vl_param to see if the vector length 
> passed by the user is mod 128, his point is that we should check a value only in one place and not in two, and it 
> is a valid point but in this case we can’t put all the check into arch_sanitise_domain_config because we 
> don’t have the “full” value from arch_domain, and we can’t put all the checks in 
> sve_sanitize_vl_param because it will leave out from the check domains created by hyper calls.
>
> So to have only one point where the checks are done (mod 128 and <= HW 
> supported VL), we might need to have a full resolution VL value in struct 
> xen_arch_domainconfig (16 bit).
>
> But if we want to save space for the future, we could leave the code as it is 
> and rely on the fact that the tools (or Xen) when creating the domain 
> configuration, will check that the SVE VL parameter is mod 128.
> In this last case what is in struct xen_arch_domainconfig is implicitly mod 128 
> and only the upper boundary of the max supported VL will be checked by Xen 
> inside arch_sanitise_domain_config.
Before answering to the approach, can you explain why you ask the user 
to pass a value that is a multiple of 128 rather than the already 
"divided" value? Is it more natural for the user?
Cheers,

--
Julien Grall