Re: [PATCH v2] xen/grants: repurpose command line max options

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> 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.