Re: [Xen-devel] [PATCH 09/11] evtchn: implement EVTCHNOP_set_limit

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.09.13 at 12:00, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> On 16/09/13 08:07, Jan Beulich wrote:
>>>>> On 13.09.13 at 18:56, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
>>> +static long evtchn_set_limit(const struct evtchn_set_limit *set_limit)
>>> +{
>>> +    struct domain *d;
>>> +    unsigned max_port = set_limit->max_port;
>>> +    long ret;
>>> +
>>> +    if ( max_port > EVTCHN_MAX_PORT_UNLIMITED )
>>> +        return -EINVAL;
>>> +
>>> +    d = rcu_lock_domain_by_id(set_limit->domid);
>>> +    if ( !d )
>>> +        return -ESRCH;
>>> +
>>> +    ret = xsm_evtchn_set_limit(XSM_DM_PRIV, d);
>>> +    if ( ret )
>>> +        goto out;
>>> +
>>> +    spin_lock(&d->event_lock);
>>> +
>>> +    d->max_evtchn_port = max_port;
>> 
>> So you allow this to be set even if the L2 ABI is in use. Does this
>> make sense? Is this consistent?
> 
> I think it would be confusing if guests could subvert the limit by using
> a different ABI, even if it doesn't really make much difference from a
> resource usage point of view.

Somehow I'm getting the impression we're no understanding one
another. My questions were:
- What's the point of permitting use of this function for a guest
  using the 2-level ABI?
- If you consider it valid to be used by a 2-level guest, will the
  result be consistent (will the guest see the limit enforced, and is
  there no implicit assumption somewhere that 2-level guests can
  always use the so far statically limited number of event channels)?

>>> @@ -1189,6 +1229,11 @@ void evtchn_check_pollers(struct domain *d, unsigned 
>>> port)
>>>  
>>>  int evtchn_init(struct domain *d)
>>>  {
>>> +    if ( is_control_domain(d) )
>>> +        d->max_evtchn_port = EVTCHN_MAX_PORT_UNLIMITED;
>>> +    else
>>> +        d->max_evtchn_port = EVTCHN_MAX_PORT_DEFAULT;
>>> +
>>>      /* Default to N-level ABI. */
>>>      evtchn_2l_init(d);
>> 
>> Similarly here - you set limits that are not consistent with the default
>> L2 ABI.
> 
> I'm not sure why you think they are inconsistent, the limits set here
> are such that there is no regression in the number of usable event
> channels.  A guest is still limited by the maximum supported by any ABI.
> i.e., the limit is min(d->max_evtchn_port, d->max_evtchns-1).

I asked because the limit for a 2-level Dom0 is now wrong. But you
ought to read this in the context of the questions above, i.e. if all's
consistent (and the max() you point out is indeed consistently
enforced), then there is no issue.

>>> --- a/xen/include/public/event_channel.h
>>> +++ b/xen/include/public/event_channel.h
>>> @@ -308,6 +308,9 @@ struct evtchn_set_limit {
>>>  };
>>>  typedef struct evtchn_set_limit evtchn_set_limit_t;
>>>  
>>> +#define EVTCHN_MAX_PORT_UNLIMITED ((1u << 31) - 1)
>>> +#define EVTCHN_MAX_PORT_DEFAULT   (NR_EVENT_CHANNELS - 1)
>> 
>> Does the former really need to be part of the ABI? And does it
>> really need to be 2^31-1 (rather than 2^32-1)?
> 
> The hypervisor uses int for port in places (e.g., get_free_port() where
> it returns a port number or a negative error code).

Ah, right, yes.

> I will remove UNLIMITED from the ABI and set_limit will map any limit >
> UNLIMITED to UNLIMITED.
> 
> At some point some one should go a change all the uses for port number
> to unsigned but I think this is work independent of this series.

Indeed.

Jan


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