[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound
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. 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |