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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 19 Apr 2023, at 08:21, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
> 
> Hi Luca,
> 
>> On 19 Apr 2023, at 09:15, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>> 
>> Hi Bertrand,
>> 
>>>> 
>>>> diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
>>>> index 5485648850a0..ad5db62e1805 100644
>>>> --- a/xen/arch/arm/arm64/sve.c
>>>> +++ b/xen/arch/arm/arm64/sve.c
>>>> @@ -9,6 +9,9 @@
>>>> #include <xen/sizes.h>
>>>> #include <asm/arm64/sve.h>
>>>> 
>>>> +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */
>>>> +int __initdata opt_dom0_sve;
>>>> +
>>>> extern unsigned int sve_get_hw_vl(void);
>>>> extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr);
>>>> extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs,
>>>> @@ -118,3 +121,21 @@ void sve_restore_state(struct vcpu *v)
>>>> 
>>>>  sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
>>>> }
>>>> +
>>>> +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?