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

[Rahul Singh]

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