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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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