[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=560d656a9a792450530eeefd0d06cfd54dcd7685This 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |