Re: [PATCH 09/11] evtchn: implement EVTCHNOP_set_limit

[David Vrabel <david.vrabel@citrix.com>]

On 16/09/13 08:07, Jan Beulich wrote:
>>>> On 13.09.13 at 18:56, David Vrabel <david.vrabel@citrix.com> 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.

>> @@ -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).

However, I'm going to change it so the hypervisor always sets the limit
to unlimited.  The toolstack should be responsible for setting any
limits (and picking a sensible default).

>> --- 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).

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.

David