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

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

[Michal Orzel]

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@amd.com>
---
 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;
 
         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;
+
         /*
          * The variable max_init_domid is initialized with zero, so here it's
          * very important to use the pre-increment operator to call
-- 
2.25.1

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

[Luca Fancellu]

> On 14 Dec 2022, at 13:08, Michal Orzel <michal.orzel@amd.com> 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@amd.com>

The patch looks good to me.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>


> ---
> 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;
> 
>         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;
> +
>         /*
>          * The variable max_init_domid is initialized with zero, so here it's
>          * very important to use the pre-increment operator to call
> -- 
> 2.25.1
> 
> 

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

[Stefano Stabellini]

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@amd.com>
> ---
>  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


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

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

[Michal Orzel]

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@amd.com>
>> ---
>>  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

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

[Stefano Stabellini]

On Fri, 16 Dec 2022, Michal Orzel wrote:
> 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@amd.com>
> >> ---
> >>  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;
> }

Yes, looks good