[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Jan,

On 25/01/2022 08:22, Jan Beulich wrote:
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 *?

This is feasible with some tweaking. Which reminds me that I have a similar patch to what you describe:

https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=560d656a9a792450530eeefd0d06cfd54dcd7685

This is doing more than what we need here as it takes care about restoring a port (for Live-Update).

Note that They are forward port from 4.11 to unstable and untested on the latter.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.