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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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;
@@ -1195,7 +1216,7 @@ long do_event_channel_op(int cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_alloc_unbound alloc_unbound;
         if ( copy_from_guest(&alloc_unbound, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_alloc_unbound(&alloc_unbound);
+        rc = _evtchn_alloc_unbound(&alloc_unbound);
         if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
         break;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..85dcf1d0c4 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -68,6 +68,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest);
 /* Free an event channel. */
 void evtchn_free(struct domain *d, struct evtchn *chn);
 
+/* Create a new event channel port */
+struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom);
+
 /* Allocate a specific event channel port. */
 int evtchn_allocate_port(struct domain *d, unsigned int port);