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

[Rahul Singh]

Hi ,

> On 9 Jul 2023, at 1:10 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> 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.

Thanks for the update. I will send the next version.


Regards,
Rahul