Slightly change the meaning of the command line
gnttab_max_{maptrack_,}frames: do not use them as upper bounds for the
passed values at domain creation, instead just use them as defaults
in the absence of any provided value.

It's not very useful for the options to be used both as defaults and
as capping values for domain creation inputs.  The defaults passed on
the command line are used by dom0 which has a very different grant
requirements than a regular domU.  dom0 usually needs a bigger
maptrack array, while domU usually require a bigger number of grant
frames.

The relaxation in the logic for the maximum size of the grant and
maptrack table sizes doesn't change the fact that domain creation
hypercall can cause resource exhausting, so disaggregated setups
should take it into account.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Expand commit message about disaggregated setups implication.
 - Reword documentation in xen-command-line for the affected options.
---
 docs/misc/xen-command-line.pandoc | 16 +++++++++-------
 xen/common/grant_table.c          |  9 +++------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index f68deaa6a9..e0b89b7d33 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1232,11 +1232,11 @@ The usage of gnttab v2 is not security supported on ARM platforms.
 
 > Can be modified at runtime
 
-Specify the maximum number of frames which any domain may use as part
-of its grant table. This value is an upper boundary of the per-domain
-value settable via Xen tools.
+Specify the default upper bound on the number of frames which any domain may
+use as part of its grant table unless a different value is specified at domain
+creation.
 
-Dom0 is using this value for sizing its grant table.
+Note this value is the effective upper bound for dom0.
 
 ### gnttab_max_maptrack_frames
 > `= <integer>`
@@ -1245,9 +1245,11 @@ Dom0 is using this value for sizing its grant table.
 
 > Can be modified at runtime
 
-Specify the maximum number of frames to use as part of a domains
-maptrack array. This value is an upper boundary of the per-domain
-value settable via Xen tools.
+Specify the default upper bound on the number of frames which any domain may
+use as part of its maptrack array unless a different value is specified at
+domain creation.
+
+Note this value is the effective upper bound for dom0.
 
 ### global-pages
     = <boolean>
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index b896f9af0e..627bf4026c 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1956,18 +1956,15 @@ int grant_table_init(struct domain *d, int max_grant_frames,
         return -EINVAL;
     }
 
-    /* Default to maximum value if no value was specified */
+    /* Apply defaults if no value was specified */
     if ( max_grant_frames < 0 )
         max_grant_frames = opt_max_grant_frames;
     if ( max_maptrack_frames < 0 )
         max_maptrack_frames = opt_max_maptrack_frames;
 
-    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
-         max_grant_frames > opt_max_grant_frames ||
-         max_maptrack_frames > opt_max_maptrack_frames )
+    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES )
     {
-        dprintk(XENLOG_INFO, "Bad grant table sizes: grant %u, maptrack %u\n",
-                max_grant_frames, max_maptrack_frames);
+        dprintk(XENLOG_INFO, "Bad grant table size %u\n", max_grant_frames);
         return -EINVAL;
     }
 
-- 
2.39.0

On 14.03.2023 15:45, Roger Pau Monne wrote:
> Slightly change the meaning of the command line
> gnttab_max_{maptrack_,}frames: do not use them as upper bounds for the
> passed values at domain creation, instead just use them as defaults
> in the absence of any provided value.
> 
> It's not very useful for the options to be used both as defaults and
> as capping values for domain creation inputs.  The defaults passed on
> the command line are used by dom0 which has a very different grant
> requirements than a regular domU.  dom0 usually needs a bigger
> maptrack array, while domU usually require a bigger number of grant
> frames.
> 
> The relaxation in the logic for the maximum size of the grant and
> maptrack table sizes doesn't change the fact that domain creation
> hypercall can cause resource exhausting, so disaggregated setups
> should take it into account.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit perhaps with yet one more wording change (which I'd be happy to
make while committing, provided you agree):

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1232,11 +1232,11 @@ The usage of gnttab v2 is not security supported on ARM platforms.
>  
>  > Can be modified at runtime
>  
> -Specify the maximum number of frames which any domain may use as part
> -of its grant table. This value is an upper boundary of the per-domain
> -value settable via Xen tools.
> +Specify the default upper bound on the number of frames which any domain may
> +use as part of its grant table unless a different value is specified at domain
> +creation.
>  
> -Dom0 is using this value for sizing its grant table.
> +Note this value is the effective upper bound for dom0.

DomU-s created during Xen boot are equally taking this as their effective
value, aiui. So I'd be inclined to amend this: "... for dom0, and for
any domU created in the course of Xen booting".

> @@ -1245,9 +1245,11 @@ Dom0 is using this value for sizing its grant table.
>  
>  > Can be modified at runtime
>  
> -Specify the maximum number of frames to use as part of a domains
> -maptrack array. This value is an upper boundary of the per-domain
> -value settable via Xen tools.
> +Specify the default upper bound on the number of frames which any domain may
> +use as part of its maptrack array unless a different value is specified at
> +domain creation.
> +
> +Note this value is the effective upper bound for dom0.

Same here then of course (which actually is bad, because those DomU-s
shouldn't need this big a maptrack table, but that's a separate topic).

Jan

On Tue, Mar 14, 2023 at 03:59:22PM +0100, Jan Beulich wrote:
> On 14.03.2023 15:45, Roger Pau Monne wrote:
> > Slightly change the meaning of the command line
> > gnttab_max_{maptrack_,}frames: do not use them as upper bounds for the
> > passed values at domain creation, instead just use them as defaults
> > in the absence of any provided value.
> > 
> > It's not very useful for the options to be used both as defaults and
> > as capping values for domain creation inputs.  The defaults passed on
> > the command line are used by dom0 which has a very different grant
> > requirements than a regular domU.  dom0 usually needs a bigger
> > maptrack array, while domU usually require a bigger number of grant
> > frames.
> > 
> > The relaxation in the logic for the maximum size of the grant and
> > maptrack table sizes doesn't change the fact that domain creation
> > hypercall can cause resource exhausting, so disaggregated setups
> > should take it into account.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit perhaps with yet one more wording change (which I'd be happy to
> make while committing, provided you agree):
> 
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -1232,11 +1232,11 @@ The usage of gnttab v2 is not security supported on ARM platforms.
> >  
> >  > Can be modified at runtime
> >  
> > -Specify the maximum number of frames which any domain may use as part
> > -of its grant table. This value is an upper boundary of the per-domain
> > -value settable via Xen tools.
> > +Specify the default upper bound on the number of frames which any domain may
> > +use as part of its grant table unless a different value is specified at domain
> > +creation.
> >  
> > -Dom0 is using this value for sizing its grant table.
> > +Note this value is the effective upper bound for dom0.
> 
> DomU-s created during Xen boot are equally taking this as their effective
> value, aiui. So I'd be inclined to amend this: "... for dom0, and for
> any domU created in the course of Xen booting".

Not really for domUs, as there's a way to pass a different value for
both options in the dt, see create_domUs().

Thanks, Roger.

On 14/03/2023 3:42 pm, Roger Pau Monné wrote:
> On Tue, Mar 14, 2023 at 03:59:22PM +0100, Jan Beulich wrote:
>> On 14.03.2023 15:45, Roger Pau Monne wrote:
>>> Slightly change the meaning of the command line
>>> gnttab_max_{maptrack_,}frames: do not use them as upper bounds for the
>>> passed values at domain creation, instead just use them as defaults
>>> in the absence of any provided value.
>>>
>>> It's not very useful for the options to be used both as defaults and
>>> as capping values for domain creation inputs.  The defaults passed on
>>> the command line are used by dom0 which has a very different grant
>>> requirements than a regular domU.  dom0 usually needs a bigger
>>> maptrack array, while domU usually require a bigger number of grant
>>> frames.
>>>
>>> The relaxation in the logic for the maximum size of the grant and
>>> maptrack table sizes doesn't change the fact that domain creation
>>> hypercall can cause resource exhausting, so disaggregated setups
>>> should take it into account.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit perhaps with yet one more wording change (which I'd be happy to
>> make while committing, provided you agree):
>>
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -1232,11 +1232,11 @@ The usage of gnttab v2 is not security supported on ARM platforms.
>>>  
>>>  > Can be modified at runtime
>>>  
>>> -Specify the maximum number of frames which any domain may use as part
>>> -of its grant table. This value is an upper boundary of the per-domain
>>> -value settable via Xen tools.
>>> +Specify the default upper bound on the number of frames which any domain may
>>> +use as part of its grant table unless a different value is specified at domain
>>> +creation.
>>>  
>>> -Dom0 is using this value for sizing its grant table.
>>> +Note this value is the effective upper bound for dom0.
>> DomU-s created during Xen boot are equally taking this as their effective
>> value, aiui. So I'd be inclined to amend this: "... for dom0, and for
>> any domU created in the course of Xen booting".
> Not really for domUs, as there's a way to pass a different value for
> both options in the dt, see create_domUs().

Correct.  On the ARM side, this is configurable in the for all dom0less
VMs in the device tree.

I've committed the patch as is, seeing as it's fixing a real bug we
currently have.


But, I'd like to point out that there are still some issues which want
fixing.

The /* Apply defaults if no value was specified */ section is pointless
complication.  All callers pass a real number of frames, except for the
dom0 construction paths which pass in -1.

The logic gets smaller and easier to follow if each of the dom0's
dom_cfg's default to the appropriate opt_* value.  Userspace which
really asks for -1 gets a large domain that actually honours the
uint32_t ABI presented.

With that, the writeable hypfs nodes become useless, and can be dropped,
and the opt_* variables become __initdata.


Next, we need to do something about the fact that the number of maptrack
frames has no relationship to the number of entries.  I don't know what,
but the status quo needs changing.

Next we need to confirm that running guests with no maptrack is safe and
security supportable option.  XSM_SILO + 0 maptrack blocks most of the
grant related XSAs we've had.

And in some copious free time, we still need to get to a point where we
can construct a VM without a grant table at all (but this still looks
like a lot of work).

~Andrew

On Wed, Mar 15, 2023 at 06:40:57PM +0000, Andrew Cooper wrote:
> On 14/03/2023 3:42 pm, Roger Pau Monné wrote:
> > On Tue, Mar 14, 2023 at 03:59:22PM +0100, Jan Beulich wrote:
> >> On 14.03.2023 15:45, Roger Pau Monne wrote:
> >>> Slightly change the meaning of the command line
> >>> gnttab_max_{maptrack_,}frames: do not use them as upper bounds for the
> >>> passed values at domain creation, instead just use them as defaults
> >>> in the absence of any provided value.
> >>>
> >>> It's not very useful for the options to be used both as defaults and
> >>> as capping values for domain creation inputs.  The defaults passed on
> >>> the command line are used by dom0 which has a very different grant
> >>> requirements than a regular domU.  dom0 usually needs a bigger
> >>> maptrack array, while domU usually require a bigger number of grant
> >>> frames.
> >>>
> >>> The relaxation in the logic for the maximum size of the grant and
> >>> maptrack table sizes doesn't change the fact that domain creation
> >>> hypercall can cause resource exhausting, so disaggregated setups
> >>> should take it into account.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> albeit perhaps with yet one more wording change (which I'd be happy to
> >> make while committing, provided you agree):
> >>
> >>> --- a/docs/misc/xen-command-line.pandoc
> >>> +++ b/docs/misc/xen-command-line.pandoc
> >>> @@ -1232,11 +1232,11 @@ The usage of gnttab v2 is not security supported on ARM platforms.
> >>>  
> >>>  > Can be modified at runtime
> >>>  
> >>> -Specify the maximum number of frames which any domain may use as part
> >>> -of its grant table. This value is an upper boundary of the per-domain
> >>> -value settable via Xen tools.
> >>> +Specify the default upper bound on the number of frames which any domain may
> >>> +use as part of its grant table unless a different value is specified at domain
> >>> +creation.
> >>>  
> >>> -Dom0 is using this value for sizing its grant table.
> >>> +Note this value is the effective upper bound for dom0.
> >> DomU-s created during Xen boot are equally taking this as their effective
> >> value, aiui. So I'd be inclined to amend this: "... for dom0, and for
> >> any domU created in the course of Xen booting".
> > Not really for domUs, as there's a way to pass a different value for
> > both options in the dt, see create_domUs().
> 
> Correct.  On the ARM side, this is configurable in the for all dom0less
> VMs in the device tree.
> 
> I've committed the patch as is, seeing as it's fixing a real bug we
> currently have.
> 
> 
> But, I'd like to point out that there are still some issues which want
> fixing.
> 
> The /* Apply defaults if no value was specified */ section is pointless
> complication.  All callers pass a real number of frames, except for the
> dom0 construction paths which pass in -1.

I'm afraid that's not accurate, xl still passes -1 if no value has been
provided in the guest config file.  And the python bindings
(pyxc_domain_create()) do seem to also hardcode -1.

Which is not to say it can't be done, but we would need to move the
default from being a command line option to a toolstack option (an
additional patch).

> The logic gets smaller and easier to follow if each of the dom0's
> dom_cfg's default to the appropriate opt_* value.  Userspace which
> really asks for -1 gets a large domain that actually honours the
> uint32_t ABI presented.
> 
> With that, the writeable hypfs nodes become useless, and can be dropped,
> and the opt_* variables become __initdata.
> 
> 
> Next, we need to do something about the fact that the number of maptrack
> frames has no relationship to the number of entries.  I don't know what,
> but the status quo needs changing.
> 
> Next we need to confirm that running guests with no maptrack is safe and
> security supportable option.  XSM_SILO + 0 maptrack blocks most of the
> grant related XSAs we've had.

I've given this a quick try and it seemed to at least boot fine, but
haven't done any in depth audit.

> And in some copious free time, we still need to get to a point where we
> can construct a VM without a grant table at all (but this still looks
> like a lot of work).

Yes, that's likely more work.  I did an attempt in the past by
allowing to set grant table version = 0 (patch on the list somewhere).

Thanks, Roger.