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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

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.


> +             close.port = port;
> +             if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
> +                     BUG();
> +     }
>       del_evtchn(u, evtchn);
>       return rc;
>   }

[snip]