[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] xen/evtchn: Introduce new IOCTL to bind static evtchn
Hi Oleksandr, Thanks for reviewing the code. > On 29 Jun 2023, at 7:06 pm, Oleksandr Tyshchenko > <oleksandr_tyshchenko@xxxxxxxx> wrote: > > > > On 29.06.23 18:46, Rahul Singh wrote: > > Hello Rahul > > >> Xen 4.17 supports the creation of static evtchns. To allow user space >> application to bind static evtchns introduce new ioctl >> "IOCTL_EVTCHN_BIND_STATIC". Existing IOCTL doing more than binding >> that’s why we need to introduce the new IOCTL to only bind the static >> event channels. >> >> Also, static evtchns to be available for use during the lifetime of the >> guest. When the application exits, __unbind_from_irq() ends up being >> called from release() file operations because of that static evtchns >> are getting closed. To avoid closing the static event channel, add the >> new bool variable "is_static" in "struct irq_info" to mark the event >> channel static when creating the event channel to avoid closing the >> static evtchn. >> >> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx> >> --- >> v2: >> * Use bool in place u8 to define is_static variable. >> * Avoid closing the static evtchns in error path. > > > Patch looks good to me, just a nit (question) below. > > >> --- >> drivers/xen/events/events_base.c | 7 +++++-- >> drivers/xen/evtchn.c | 30 ++++++++++++++++++++++-------- >> include/uapi/xen/evtchn.h | 9 +++++++++ >> include/xen/events.h | 2 +- >> 4 files changed, 37 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/xen/events/events_base.c >> b/drivers/xen/events/events_base.c >> index c7715f8bd452..5d3b5c7cfe64 100644 >> --- a/drivers/xen/events/events_base.c >> +++ b/drivers/xen/events/events_base.c >> @@ -112,6 +112,7 @@ struct irq_info { >> unsigned int irq_epoch; /* If eoi_cpu valid: irq_epoch of event */ >> u64 eoi_time; /* Time in jiffies when to EOI. */ >> raw_spinlock_t lock; >> + bool is_static; /* Is event channel static */ >> >> union { >> unsigned short virq; >> @@ -982,7 +983,8 @@ static void __unbind_from_irq(unsigned int irq) >> unsigned int cpu = cpu_from_irq(irq); >> struct xenbus_device *dev; >> >> - xen_evtchn_close(evtchn); >> + if (!info->is_static) >> + xen_evtchn_close(evtchn); >> >> switch (type_from_irq(irq)) { >> case IRQT_VIRQ: >> @@ -1574,7 +1576,7 @@ int xen_set_irq_priority(unsigned irq, unsigned >> priority) >> } >> EXPORT_SYMBOL_GPL(xen_set_irq_priority); >> >> -int evtchn_make_refcounted(evtchn_port_t evtchn) >> +int evtchn_make_refcounted(evtchn_port_t evtchn, bool is_static) >> { >> int irq = get_evtchn_to_irq(evtchn); >> struct irq_info *info; >> @@ -1590,6 +1592,7 @@ int evtchn_make_refcounted(evtchn_port_t evtchn) >> WARN_ON(info->refcnt != -1); >> >> info->refcnt = 1; >> + info->is_static = is_static; >> >> return 0; >> } >> diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c >> index c99415a70051..e6d2303478b2 100644 >> --- a/drivers/xen/evtchn.c >> +++ b/drivers/xen/evtchn.c >> @@ -366,7 +366,8 @@ static int evtchn_resize_ring(struct per_user_data *u) >> return 0; >> } >> >> -static int evtchn_bind_to_user(struct per_user_data *u, evtchn_port_t port) >> +static int evtchn_bind_to_user(struct per_user_data *u, evtchn_port_t port, >> + bool is_static) >> { >> struct user_evtchn *evtchn; >> struct evtchn_close close; >> @@ -402,14 +403,16 @@ static int evtchn_bind_to_user(struct per_user_data >> *u, evtchn_port_t port) >> if (rc < 0) >> goto err; >> >> - rc = evtchn_make_refcounted(port); >> + rc = evtchn_make_refcounted(port, is_static); >> return rc; >> >> err: >> /* bind failed, should close the port now */ >> - close.port = port; >> - if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0) >> - BUG(); >> + if (!is_static) { > > > I think now "struct evtchn_close close;" can be placed here as it is not > used outside of this block. > > Also this block looks like an open-coded version of xen_evtchn_close() > defined at events_base.c, so maybe it is worth making xen_evtchn_close() > static inline and placing it into events.h, then calling helper here? > Please note, I will be ok either way. Make sense. I will modify the patch as per your request in the next version. I will wait for other maintainers to review the patch before sending the next version. Regards, Rahul
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |