[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound
On Wed, 12 Jan 2022, Jan Beulich wrote: > On 11.01.2022 23:49, Stefano Stabellini wrote: > > 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. > > Right, that's what I was trying to hint at as an alternative. OK, I'll do that then
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |