Re: [PATCH v2] xen/evtchn: Introduce new IOCTL to bind static evtchn
From: Rahul Singh
Date: Fri Jun 30 2023 - 09:03:27 EST
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