Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2022 23:49, Stefano Stabellini wrote:
> On Mon, 10 Jan 2022, Jan Beulich wrote:
>> On 08.01.2022 01:49, Stefano Stabellini wrote:
>>> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>>      xsm_evtchn_close_post(chn);
>>>  }
>>>  
>>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
>>
>> Function names want to be the other way around, to be in line with
>> naming rules of the C spec: The static function may be underscore-
>> prefixed, while the non-static one may not.
> 
> OK
> 
> 
>>>  {
>>>      struct evtchn *chn;
>>> +    int port;
>>> +
>>> +    if ( (port = get_free_port(d)) < 0 )
>>> +        return ERR_PTR(port);
>>> +    chn = evtchn_from_port(d, port);
>>> +
>>> +    evtchn_write_lock(chn);
>>> +
>>> +    chn->state = ECS_UNBOUND;
>>> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
>>> +        chn->u.unbound.remote_domid = current->domain->domain_id;
>>
>> I think the resolving of DOMID_SELF should remain in the caller, as I'm
>> pretty sure your planned new user(s) can't sensibly pass that value.
> 
> Yep, no problem
> 
> 
>>> +    evtchn_port_init(d, chn);
>>> +
>>> +    evtchn_write_unlock(chn);
>>> +
>>> +    return chn;
>>> +}
>>> +
>>> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +{
>>> +    struct evtchn *chn = NULL;
>>
>> I don't think the initializer is needed.
> 
> OK
> 
> 
>>> @@ -297,27 +319,22 @@ static int 
>>> evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>>  
>>>      spin_lock(&d->event_lock);
>>>  
>>> -    if ( (port = get_free_port(d)) < 0 )
>>> -        ERROR_EXIT_DOM(port, d);
>>> -    chn = evtchn_from_port(d, port);
>>> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
>>> +    if ( IS_ERR(chn) )
>>> +    {
>>> +        rc = PTR_ERR(chn);
>>> +        ERROR_EXIT_DOM(rc, d);
>>> +    }
>>>  
>>>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>>>      if ( rc )
>>>          goto out;
>>>  
>>> -    evtchn_write_lock(chn);
>>> -
>>> -    chn->state = ECS_UNBOUND;
>>
>> This cannot be pulled ahead of the XSM check (or in general anything
>> potentially resulting in an error), as check_free_port() relies on
>> ->state remaining ECS_FREE until it is known that the calling function
>> can't fail anymore.
> 
> OK, I didn't realize. Unfortunately it means we have to move setting
> chn->state = ECS_UNBOUND to the caller.
> 
> 
>>> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
>>> -        chn->u.unbound.remote_domid = current->domain->domain_id;
>>> -    evtchn_port_init(d, chn);
>>> -
>>> -    evtchn_write_unlock(chn);
>>> -
>>> -    alloc->port = port;
>>> +    alloc->port = chn->port;
>>>  
>>>   out:
>>> -    check_free_port(d, port);
>>> +    if ( chn != NULL )
>>> +        check_free_port(d, chn->port);
>>
>> Without the initializer above it'll then be more obvious that the
>> condition here needs to be !IS_ERR(chn).
>>
>> Also (nit) please prefer the shorter "if ( chn )".
>>
>> Overall I wonder in how far it would be possible to instead re-use PV
>> shim's "backdoor" into port allocation: evtchn_allocate_port() was
>> specifically made available for it, iirc.
> 
> I don't see an obvious way to do it. These are the 4 things we need to
> do:
> 
> 1) call get_free_port/evtchn_allocate_port
> 2) set state = ECS_UNBOUND
> 3) set remote_domid
> 4) call evtchn_port_init
> 
> It doesn't look like we could enhance evtchn_allocate_port to do 2) and
> 3). And probably even 4) couldn't be added to evtchn_allocate_port.
> 
> So basically it is like calling get_free_port() and do 2,3,4 ourselves
> from the caller in xen/arch/arm/domain_build.c. But that might be a good
> idea actually. Maybe we should leave evtchn_alloc_unbound unmodified and
> instead open-code what we need to do in xen/arch/arm/domain_build.c.

Right, that's what I was trying to hint at as an alternative.

Jan

> This is how it would look like as a new function in
> xen/arch/arm/domain_build.c:
> 
> static int alloc_xenstore_evtchn(struct domain *d)
> {
>     struct evtchn *chn;
>     int port;
> 
>     if ( (port = get_free_port(d)) < 0 )
>         return ERR_PTR(port);
>     chn = evtchn_from_port(d, port);
> 
>     chn->state = ECS_UNBOUND;
>     chn->u.unbound.remote_domid = hardware_domain->domain_id;
>     evtchn_port_init(d, chn);
> 
>     return chn->port;
> }
> 
> What do you think? It might not be worth introducing
> evtchn_alloc_unbound / _evtchn_alloc_unbound for this?
> 
> I am happy to follow what you think is best.
> 
> Cheers,
> 
> Stefano
>