Re: [PATCH v2] xen/evtchn: Introduce new IOCTL to bind static evtchn

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 30 Jun 2023, Rahul Singh wrote:
> 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.

I don't have any further comments.