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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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.

> 
>> 
>>> +    else
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index eeb4662f0eee..3f30ef5c37b6 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -26,6 +26,7 @@
>>> #include <asm/platform.h>
>>> #include <asm/psci.h>
>>> #include <asm/setup.h>
>>> +#include <asm/arm64/sve.h>
>>> #include <asm/cpufeature.h>
>>> #include <asm/domain_build.h>
>>> #include <xen/event.h>
>>> @@ -61,6 +62,21 @@ custom_param("dom0_mem", parse_dom0_mem);
>>> 
>>> int __init parse_arch_dom0_param(const char *s, const char *e)
>>> {
>>> +    long long val;
>>> +
>>> +    if ( !parse_signed_integer("sve", s, e, &val) )
>>> +    {
>>> +#ifdef CONFIG_ARM64_SVE
>>> +        if ( (val >= INT_MIN) && (val <= INT_MAX) )
>>> +            opt_dom0_sve = val;
>>> +        else
>>> +            printk(XENLOG_INFO "'sve=%lld' value out of range!\n", val);
>>> +#else
>>> +        no_config_param("ARM64_SVE", "sve", s, e);
>>> +#endif
>> 
>> Correct me if my understanding is wrong but here you just ignore the sve
>> parameter if SVE is not supported by Xen ?
>> 
>> I am a bit wondering if we should not just refuse it here as the user might
>> wrongly think that his parameter had some effect.
>> 
>> Or is it a usual way to handle this case ?
> 
> Jan suggested this approach, as it should be the preferred way to handle the 
> case,
> looking into the x86 code it seems so.
> 

This is somehow going around the global discussion: is it really ok to ignore 
sve 
param if it is not supported. Let's have this discussion on the other thread 
instead.

Cheers
Bertrand