Re: [PATCH] xen/arm: Allow to set grant table related limits for dom0less domUs

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 15/12/2022 22:38, Stefano Stabellini wrote:
> 
> 
> On Wed, 14 Dec 2022, Michal Orzel wrote:
>> At the moment, for dom0less domUs, we do not have a way to specify
>> per domain grant table related limits (unlike when using xl), namely
>> max version, max number of grant frames, max number of maptrack frames.
>> This means that such domains always use the values specified by the Xen
>> command line parameters or their default values if unspecified.
>>
>> In order to have more control over dom0less domUs, introduce the
>> following device-tree properties that can be set under domUs nodes:
>>  - max_grant_version to set the maximum grant table version the domain
>>    is allowed to use,
>>  - max_grant_frames to set the maximum number of grant frames the domain
>>    is allowed to have,
>>  - max_maptrack_frames to set the maximum number of grant maptrack frames
>>    the domain is allowed to have.
>>
>> Update documentation accordingly.
>>
>> Note that the sanity checks regarding the passed values are already
>> there in grant_table_init() resulting in panic in case of errors,
>> therefore no need to repeat them in create_domUs().
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>  docs/misc/arm/device-tree/booting.txt | 21 +++++++++++++++++++++
>>  xen/arch/arm/domain_build.c           | 11 ++++++++++-
>>  2 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/misc/arm/device-tree/booting.txt 
>> b/docs/misc/arm/device-tree/booting.txt
>> index 87eaa3e25491..3879340b5e0a 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -223,6 +223,27 @@ with the following properties:
>>      the default size of domain P2M pool, i.e. 1MB per guest vCPU plus 4KB
>>      per MB of guest RAM plus 512KB for guest extended regions.
>>
>> +- max_grant_version
>> +
>> +    Optional. A 32-bit integer specifying the maximum grant table version
>> +    the domain is allowed to use (valid values are 1 or 2). If this property
>> +    is missing, the value specified by Xen command line parameter 
>> gnttab=max-ver
>> +    (or its default value if unspecified, i.e. 1) is used.
>> +
>> +- max_grant_frames
>> +
>> +    Optional. A 32-bit integer specifying the maximum number of grant frames
>> +    the domain is allowed to have. If this property is missing, the value
>> +    specified by Xen command line parameter gnttab_max_frames (or its 
>> default
>> +    value if unspecified, i.e. 64) is used.
>> +
>> +- max_maptrack_frames
>> +
>> +    Optional. A 32-bit integer specifying the maximum number of grant 
>> maptrack
>> +    frames the domain is allowed to have. If this property is missing, the
>> +    value specified by Xen command line parameter gnttab_max_maptrack_frames
>> +    (or its default value if unspecified, i.e. 1024) is used.
>> +
>>  Under the "xen,domain" compatible node, one or more sub-nodes are present
>>  for the DomU kernel and ramdisk.
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index bef5e905a73c..29b2f3e1faa2 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3871,7 +3871,7 @@ void __init create_domUs(void)
>>              .max_maptrack_frames = -1,
>>              .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
>>          };
>> -        unsigned int flags = 0U;
>> +        unsigned int flags = 0U, val;
> 
> val should be uint32_t
ok

> 
> 
>>          if ( !dt_device_is_compatible(node, "xen,domain") )
>>              continue;
>> @@ -3940,6 +3940,15 @@ void __init create_domUs(void)
>>              d_cfg.cpupool_id = pool_id;
>>          }
>>
>> +        if ( dt_property_read_u32(node, "max_grant_version", &val) )
>> +            d_cfg.grant_opts = XEN_DOMCTL_GRANT_version(val);
>> +
>> +        if ( dt_property_read_u32(node, "max_grant_frames", &val) )
>> +            d_cfg.max_grant_frames = val;
>> +
>> +        if ( dt_property_read_u32(node, "max_maptrack_frames", &val) )
>> +            d_cfg.max_maptrack_frames = val;
> 
> We need to be careful here because max_grant_frames and
> max_maptrack_frames are defined as int32_t (signed):
> 
>     int32_t max_grant_frames;
>     int32_t max_maptrack_frames;
> 
> I think we should have a check to make sure we don't cause an overflow.
> For instance:
> 
> if ( val >= INT32_MAX ) {
>     error;
> }
> d_cfg.max_grant_frames = val;

Is following ok for you?

if ( dt_property_read_u32(node, "max_grant_frames", &val) )
{
    if ( val > INT32_MAX )
        panic("max_grant_frames (%"PRIu32") overflow\n", val);
    d_cfg.max_grant_frames = val;
}

if ( dt_property_read_u32(node, "max_maptrack_frames", &val) )
{
    if ( val > INT32_MAX )
        panic("max_maptrack_frames (%"PRIu32") overflow\n", val);
    d_cfg.max_maptrack_frames = val;
}

~Michal