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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

> 
>> +    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.