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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

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)));