Hi Luca,
> On 20 Apr 2023, at 10:46, Luca Fancellu <luca.fance...@arm.com> 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?
Sorry i missed that answer.
Yes i agree, maybe we could factorize the checks in one function and use it in
several places ?
>
> 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:
>
> struct arch_domain {
> enum domain_type type; /* 0 4 */
> uint8_t sve_vl; /* 4 1 */
>
> /* XXX 3 bytes hole, try to pack */
>
> struct p2m_domain p2m; /* 8 328 */
> /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
> struct hvm_domain hvm; /* 336 312 */
> /* --- cacheline 10 boundary (640 bytes) was 8 bytes ago --- */
> struct paging_domain paging; /* 648 32 */
> struct vmmio vmmio; /* 680 32 */
> /* --- cacheline 11 boundary (704 bytes) was 8 bytes ago --- */
> unsigned int rel_priv; /* 712 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct {
> uint64_t offset; /* 720 8 */
> s_time_t nanoseconds; /* 728 8 */
> } virt_timer_base; /* 720 16 */
> struct vgic_dist vgic; /* 736 200 */
>
> /* XXX last struct has 2 bytes of padding */
>
> /* --- cacheline 14 boundary (896 bytes) was 40 bytes ago --- */
> struct vuart vuart; /* 936 32 */
> /* --- cacheline 15 boundary (960 bytes) was 8 bytes ago --- */
> unsigned int evtchn_irq; /* 968 4 */
> struct {
> uint8_t privileged_call_enabled:1; /* 972: 0 1 */
> } monitor; /* 972 1 */
>
> /* XXX 3 bytes hole, try to pack */
>
> struct vpl011 vpl011; /* 976 72 */
>
> /* size: 1152, cachelines: 18, members: 13 */
> /* sum members: 1038, holes: 3, sum holes: 10 */
> /* padding: 104 */
> /* paddings: 1, sum paddings: 2 */
> } __attribute__((__aligned__(128)));
That would work but it is a bit odd to save a 16bit value just so
you could save invalid values and give an error.
Cheers
Bertrand
