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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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

Yes, looks good