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

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi Luca,

> On 20 Apr 2023, at 10:46, Luca Fancellu <Luca.Fancellu@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