| Message ID | 08fab20e71d280396d7b65397339ad9d9ab96d5c.1655903088.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/evtchn: implement static event channel signaling | expand |
On 22.06.2022 16:38, Rahul Singh wrote: > @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) > spin_lock(&ld->event_lock); > } > > - if ( (lport = get_free_port(ld)) < 0 ) > - ERROR_EXIT(lport); > + if ( lport != 0 ) > + { > + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 ) > + ERROR_EXIT_DOM(rc, ld); > + } > + else > + { > + int alloc_port = get_free_port(ld); > + > + if ( alloc_port < 0 ) > + ERROR_EXIT_DOM(alloc_port, ld); > + lport = alloc_port; > + } This is then the 3rd instance of this pattern. I think the logic wants moving into get_free_port() (perhaps with a name change). And of course like in the earlier patch the issue with sparse port numbers needs to be resolved. Jan
Hi Jan, On 05/07/2022 16:11, Jan Beulich wrote: > On 22.06.2022 16:38, Rahul Singh wrote: >> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) >> spin_lock(&ld->event_lock); >> } >> >> - if ( (lport = get_free_port(ld)) < 0 ) >> - ERROR_EXIT(lport); >> + if ( lport != 0 ) >> + { >> + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 ) >> + ERROR_EXIT_DOM(rc, ld); >> + } >> + else >> + { >> + int alloc_port = get_free_port(ld); >> + >> + if ( alloc_port < 0 ) >> + ERROR_EXIT_DOM(alloc_port, ld); >> + lport = alloc_port; >> + } > > This is then the 3rd instance of this pattern. I think the logic > wants moving into get_free_port() (perhaps with a name change). I think the code below would be suitable. I can send it or Rahul can re-use the commit [1]: diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index e6fb024218e949529c587b7c4755295786d6e7a7..757088580c2b2a3e55774f50b8ad7b3ec9243788 100644 (file) --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -292,6 +292,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn) xsm_evtchn_close_post(chn); } +static int evtchn_get_port(struct domain *d, uint32_t port) +{ + int rc; + + if ( port != 0 ) + rc = evtchn_allocate_port(d, port); + else + rc = get_free_port(d); + + return rc != 0 ? rc : port; +} + static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) { struct evtchn *chn; @@ -451,19 +463,10 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) if ( read_atomic(&v->virq_to_evtchn[virq]) ) ERROR_EXIT(-EEXIST); - if ( port != 0 ) - { - if ( (rc = evtchn_allocate_port(d, port)) != 0 ) - ERROR_EXIT(rc); - } - else - { - int alloc_port = get_free_port(d); - - if ( alloc_port < 0 ) - ERROR_EXIT(alloc_port); - port = alloc_port; - } + port = rc = evtchn_get_port(d, port); + if ( rc < 0 ) + ERROR_EXIT(rc); + rc = 0; chn = evtchn_from_port(d, port); [1] https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=3860ed9915d6fee97a1088110ffd0a6f29f04d9a > > And of course like in the earlier patch the issue with sparse port > numbers needs to be resolved. > > Jan
On 05.07.2022 17:22, Julien Grall wrote: > Hi Jan, > > On 05/07/2022 16:11, Jan Beulich wrote: >> On 22.06.2022 16:38, Rahul Singh wrote: >>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) >>> spin_lock(&ld->event_lock); >>> } >>> >>> - if ( (lport = get_free_port(ld)) < 0 ) >>> - ERROR_EXIT(lport); >>> + if ( lport != 0 ) >>> + { >>> + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 ) >>> + ERROR_EXIT_DOM(rc, ld); >>> + } >>> + else >>> + { >>> + int alloc_port = get_free_port(ld); >>> + >>> + if ( alloc_port < 0 ) >>> + ERROR_EXIT_DOM(alloc_port, ld); >>> + lport = alloc_port; >>> + } >> >> This is then the 3rd instance of this pattern. I think the logic >> wants moving into get_free_port() (perhaps with a name change). > > I think the code below would be suitable. I can send it or Rahul can > re-use the commit [1]: Ah yes; probably makes sense (right now) only in the context of his series. > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -292,6 +292,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn) > xsm_evtchn_close_post(chn); > } > > +static int evtchn_get_port(struct domain *d, uint32_t port) Preferably evtchn_port_t. > +{ > + int rc; > + > + if ( port != 0 ) > + rc = evtchn_allocate_port(d, port); > + else > + rc = get_free_port(d); > + > + return rc != 0 ? rc : port; We commonly use "rc ?: port" in such cases. At which point I'd be inclined to make it just static int evtchn_get_port(struct domain *d, evtchn_port_t port) { return (port ? evtchn_allocate_port(d, port) : get_free_port(d)) ?: port; } But I can see you or others having reservations against such ... Jan
Hi Julien, > On 5 Jul 2022, at 4:22 pm, Julien Grall <julien@xen.org> wrote: > > Hi Jan, > > On 05/07/2022 16:11, Jan Beulich wrote: >> On 22.06.2022 16:38, Rahul Singh wrote: >>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) >>> spin_lock(&ld->event_lock); >>> } >>> - if ( (lport = get_free_port(ld)) < 0 ) >>> - ERROR_EXIT(lport); >>> + if ( lport != 0 ) >>> + { >>> + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 ) >>> + ERROR_EXIT_DOM(rc, ld); >>> + } >>> + else >>> + { >>> + int alloc_port = get_free_port(ld); >>> + >>> + if ( alloc_port < 0 ) >>> + ERROR_EXIT_DOM(alloc_port, ld); >>> + lport = alloc_port; >>> + } >> This is then the 3rd instance of this pattern. I think the logic >> wants moving into get_free_port() (perhaps with a name change). > > I think the code below would be suitable. I can send it or Rahul can re-use the commit [1]: Thanks for the code. I will include this patch in next version. Regards, Rahul
Hi Jan > On 5 Jul 2022, at 4:42 pm, Jan Beulich <jbeulich@suse.com> wrote: > > On 05.07.2022 17:22, Julien Grall wrote: >> Hi Jan, >> >> On 05/07/2022 16:11, Jan Beulich wrote: >>> On 22.06.2022 16:38, Rahul Singh wrote: >>>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) >>>> spin_lock(&ld->event_lock); >>>> } >>>> >>>> - if ( (lport = get_free_port(ld)) < 0 ) >>>> - ERROR_EXIT(lport); >>>> + if ( lport != 0 ) >>>> + { >>>> + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 ) >>>> + ERROR_EXIT_DOM(rc, ld); >>>> + } >>>> + else >>>> + { >>>> + int alloc_port = get_free_port(ld); >>>> + >>>> + if ( alloc_port < 0 ) >>>> + ERROR_EXIT_DOM(alloc_port, ld); >>>> + lport = alloc_port; >>>> + } >>> >>> This is then the 3rd instance of this pattern. I think the logic >>> wants moving into get_free_port() (perhaps with a name change). >> >> I think the code below would be suitable. I can send it or Rahul can >> re-use the commit [1]: > > Ah yes; probably makes sense (right now) only in the context of his > series. Ack. > >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -292,6 +292,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn) >> xsm_evtchn_close_post(chn); >> } >> >> +static int evtchn_get_port(struct domain *d, uint32_t port) > > Preferably evtchn_port_t. Ack. > >> +{ >> + int rc; >> + >> + if ( port != 0 ) >> + rc = evtchn_allocate_port(d, port); >> + else >> + rc = get_free_port(d); >> + >> + return rc != 0 ? rc : port; > > We commonly use "rc ?: port" in such cases. At which point I'd be > inclined to make it just > > static int evtchn_get_port(struct domain *d, evtchn_port_t port) > { > return (port ? evtchn_allocate_port(d, port) > : get_free_port(d)) ?: port; > } > > But I can see you or others having reservations against such ... If everyone agree with above code I will modify the Julien patch to include this code in next version. Regards, Rahul
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 80a88c1544..bf5dc2c8ad 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -363,11 +363,16 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn) evtchn_write_unlock(rchn); } -int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) +/* + * If lport is zero get the next free port and allocate. If port is non-zero + * allocate the specified lport. + */ +int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, + evtchn_port_t lport) { struct evtchn *lchn, *rchn; struct domain *ld = current->domain, *rd; - int lport, rc; + int rc; evtchn_port_t rport = bind->remote_port; domid_t rdom = bind->remote_dom; @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) spin_lock(&ld->event_lock); } - if ( (lport = get_free_port(ld)) < 0 ) - ERROR_EXIT(lport); + if ( lport != 0 ) + { + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 ) + ERROR_EXIT_DOM(rc, ld); + } + else + { + int alloc_port = get_free_port(ld); + + if ( alloc_port < 0 ) + ERROR_EXIT_DOM(alloc_port, ld); + lport = alloc_port; + } lchn = evtchn_from_port(ld, lport); rchn = _evtchn_from_port(rd, rport); @@ -1232,7 +1248,7 @@ long cf_check do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) struct evtchn_bind_interdomain bind_interdomain; if ( copy_from_guest(&bind_interdomain, arg, 1) != 0 ) return -EFAULT; - rc = evtchn_bind_interdomain(&bind_interdomain); + rc = evtchn_bind_interdomain(&bind_interdomain, 0); if ( !rc && __copy_to_guest(arg, &bind_interdomain, 1) ) rc = -EFAULT; /* Cleaning up here would be a mess! */ break; diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index 48820e393e..6e26879793 100644 --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -76,7 +76,8 @@ int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port); /* Bind an event channel port to interdomain */ -int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind); +int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, + evtchn_port_t port); /* Unmask a local event-channel port. */ int evtchn_unmask(unsigned int port);
evtchn_bind_interdomain() always allocates the next available local port. Static event channel support for dom0less domains requires allocating a specified port. Modify the evtchn_bind_interdomain to accept the port number as an argument and allocate the specified port if available. If the port number argument is zero, the next available port will be allocated. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- xen/common/event_channel.c | 26 +++++++++++++++++++++----- xen/include/xen/event.h | 3 ++- 2 files changed, 23 insertions(+), 6 deletions(-)