| Message ID | 20220108004912.3820176-2-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dom0less PV drivers | expand |
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. > { > 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. > + 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. > @@ -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. > - 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. Jan
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
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. Jan > 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 >
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
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index da88ad141a..8a19bbf7ae 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,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) { 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; + 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; struct domain *d; - int port, rc; + int rc; domid_t dom = alloc->dom; d = rcu_lock_domain_by_any_id(dom); @@ -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; - 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); spin_unlock(&d->event_lock); rcu_unlock_domain(d); diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index 21c95e14fd..6aedbccbf1 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);