Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2022 02:10, Stefano Stabellini wrote:
> On Sun, 23 Jan 2022, Julien Grall wrote:
>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>> index da88ad141a..5b0bcaaad4 100644
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t
>>> port)
>>>       return 0;
>>>   }
>>>   -static int get_free_port(struct domain *d)
>>> +int get_free_port(struct domain *d)
>>
>> I dislike the idea to expose get_free_port() (or whichever name we decide)
>> because this can be easily misused.
>>
>> In fact looking at your next patch (#3), you are misusing it as it is meant 
>> to
>> be called with d->event_lock. I know this doesn't much matter
>> in your situation because this is done at boot with no other domains running
>> (or potentially any event channel allocation). However, I still think we
>> should get the API right.
>>
>> I am also not entirely happy of open-coding the allocation in domain_build.c.
>> Instead, I would prefer if we provide a new helper to allocate an unbound
>> event channel. This would be similar to your v1 (I still need to review the
>> patch though).
> 
> I am happy to go back to v1 and address feedback on that patch. However,
> I am having difficulties with the implementation. Jan pointed out:
> 
> 
>>> -
>>> -    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.
> 
> This makes it difficult to reuse _evtchn_alloc_unbound for the
> implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
> to do it.
> 
> Instead, I just create a new public function called
> "evtchn_alloc_unbound" and renamed the existing funtion to
> "_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
> static function should be the one starting with "_"). So the function
> names are inverted compared to v1.
> 
> Please let me know if you have any better suggestions.
> 
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index da88ad141a..c6b7dd7fbd 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -18,6 +18,7 @@
>  
>  #include <xen/init.h>
>  #include <xen/lib.h>
> +#include <xen/err.h>
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/irq.h>
> @@ -284,7 +285,27 @@ 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)
> +{
> +    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;
> +    chn->u.unbound.remote_domid = remote_dom;
> +    evtchn_port_init(d, chn);
> +
> +    evtchn_write_unlock(chn);
> +
> +    return chn;
> +}
> +
> +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>  {
>      struct evtchn *chn;
>      struct domain *d;

Instead of introducing a clone of this function (with, btw, still
insufficient locking), did you consider simply using the existing
evtchn_alloc_unbound() as-is, i.e. with the caller passing
evtchn_alloc_unbound_t *?

Jan