Re: [PATCH v8 13/15] xen: make grant resource limits per domain

["Jan Beulich" <JBeulich@suse.com>]

>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
> On 20/09/17 12:34, Jan Beulich wrote:
>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>> --- a/xen/common/compat/grant_table.c
>>> +++ b/xen/common/compat/grant_table.c
>>> @@ -157,21 +157,14 @@ int compat_grant_table_op(unsigned int cmd,
>>>                  unsigned int max_frame_list_size_in_page =
>>>                      (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
>>>                      sizeof(*nat.setup->frame_list.p);
>>> -                if ( max_frame_list_size_in_page < max_grant_frames )
>> 
>> The latest here, but perhaps even earlier I think max_grant_frames
>> should become static, so one can be reasonably certain that all other
>> references are gone.
> 
> Patch 14 removes the last references, so I can't do it earlier.

That's unfortunate, but okay.

>>> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>>>  
>>>  static long
>>>  gnttab_setup_table(
>>> -    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>> +    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>>> +    unsigned int limit_max)
>>>  {
>>>      struct vcpu *curr = current;
>>>      struct gnttab_setup_table op;
>>>      struct domain *d = NULL;
>>>      struct grant_table *gt;
>>>      unsigned int i;
>>> +    long ret = 0;
>> 
>> Wouldn't int suffice here?
> 
> I just followed the return type of the function. I think this way is
> cleaner, but in case you like int better I can change it.

I sort of expected this reply, but that's not in line with what you
did in gnttab_get_status_frames() then. I think we will want to
eventually change all function return types to int where the wider
type isn't needed.

>>> @@ -3442,6 +3469,8 @@ grant_table_create(
>>>      /* Simple stuff. */
>>>      percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>>      spin_lock_init(&t->maptrack_lock);
>>> +    t->max_grant_frames = max_grant_frames;
>>> +    t->max_maptrack_frames = max_maptrack_frames;
>> 
>> This together with ...
>> 
>>> @@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>>  
>>>      /* Set limits. */
>>>      if ( !gt->active )
>>> +    {
>>> +        gt->max_grant_frames = grant_frames;
>>> +        gt->max_maptrack_frames = maptrack_frames;
>>>          ret = grant_table_init(gt);
>>> +    }
>> 
>> .. this raises the question of whether it is legal to decrease the
>> limits. There may be code depending on it only ever growing.
> 
> Before grant_table_init() has been called there is no problem
> decreasing the limits, as nothing depending on them has been setup
> yet.

Oh, right, I didn't pay attention to this being a one-time action.

>> Additionally to take the input values without applying some
>> upper cap - while we have XSA-77, we still shouldn't introduce
>> new issues making disaggregation more unsafe. Perhaps the
>> global limits could serve as a cap here?
> 
> I thought about a cap and TBH I'm not sure which would be sane to
> apply. The global limits seem wrong, especially looking at patch 14:
> those limits will be for dom0 only then. And dom0 won't need many
> grant frames in the normal case...

I've been thinking about this Dom0 aspect too over lunch. What
about allowing the hardware domain to set its limit (only upwards
of course) in setup_table(), without any upper bound enforced?
This would free up the globals to be used as system wide limits
again.

> So I could make up a cap in form of either a configurable constant
> (CONFIG_* or boot parameter?) or as a fraction of domain memory. Any
> preferences here?

A config constant as well as a fraction of domain memory might lock
out special purpose guests. Which would leave command line options
- as per above perhaps the ones we already have.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel