Hi Luca,
> On 19 Apr 2023, at 09:15, Luca Fancellu <[email protected]> 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
