Re: [PATCH V4] xen: privcmd: Add support for irqfd

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 8348 bytes --]

On 25.07.23 12:57, Viresh Kumar wrote:
> Xen provides support for injecting interrupts to the guests via the
> HYPERVISOR_dm_op() hypercall. The same is used by the Virtio based
> device backend implementations, in an inefficient manner currently.
> 
> Generally, the Virtio backends are implemented to work with the Eventfd
> based mechanism. In order to make such backends work with Xen, another
> software layer needs to poll the Eventfds and raise an interrupt to the
> guest using the Xen based mechanism. This results in an extra context
> switch.
> 
> This is not a new problem in Linux though. It is present with other
> hypervisors like KVM, etc. as well. The generic solution implemented in
> the kernel for them is to provide an IOCTL call to pass the interrupt
> details and eventfd, which lets the kernel take care of polling the
> eventfd and raising of the interrupt, instead of handling this in user
> space (which involves an extra context switch).
> 
> This patch adds support to inject a specific interrupt to guest using
> the eventfd mechanism, by preventing the extra context switch.
> 
> Inspired by existing implementations for KVM, etc..
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V3->V4
> - Drop the imported definitions to hvm/dm_op.h.
> - Make the caller pass a pointer to pre-filled "struct xen_dm_op" instance and
>    get rid of irq and level fields.
> - Enable the irqfd feature under a new Kconfig entry.
> 
> V2->V3
> - Select EVENTFD from Kconfig
> 
> V1->V2:
> - Improve error handling.
> - Remove the unnecessary usage of list_for_each_entry_safe().
> - Restrict the use of XEN_DMOP_set_irq_level to only ARM64.
> - Import definitions from Xen to hvm/dm_op.h.
> 
>   drivers/xen/Kconfig        |   8 ++
>   drivers/xen/privcmd.c      | 286 ++++++++++++++++++++++++++++++++++++-
>   include/uapi/xen/privcmd.h |  14 ++
>   3 files changed, 306 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index d5d7c402b651..c7fabfab4c20 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -269,6 +269,14 @@ config XEN_PRIVCMD
>   	  disaggregated Xen setups this driver might be needed for other
>   	  domains, too.
>   
> +config XEN_PRIVCMD_IRQFD
> +	bool "Xen irqfd support"
> +	depends on XEN_PRIVCMD && XEN_VIRTIO && EVENTFD
> +	default m

"m"? I guess you meant "n"? In this case the "default" line should just
be dropped.

> +	help
> +	  irqfd is a mechanism to inject a specific interrupt to a User VM using
> +	  a decoupled eventfd mechanism.

I think the help text should mention "virtio".

What about:

"Using the irqfd mechanism a virtio backend running in a daemon can speed
  up interrupt injection into a guest."

> +
>   config XEN_ACPI_PROCESSOR
>   	tristate "Xen ACPI processor"
>   	depends on XEN && XEN_PV_DOM0 && X86 && ACPI_PROCESSOR && CPU_FREQ
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index e2f580e30a86..584c8de56c5e 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -9,11 +9,16 @@
>   
>   #define pr_fmt(fmt) "xen:" KBUILD_MODNAME ": " fmt
>   
> +#include <linux/eventfd.h>
> +#include <linux/file.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/poll.h>
>   #include <linux/sched.h>
>   #include <linux/slab.h>
>   #include <linux/string.h>
> +#include <linux/workqueue.h>
>   #include <linux/errno.h>
>   #include <linux/mm.h>
>   #include <linux/mman.h>
> @@ -833,6 +838,267 @@ static long privcmd_ioctl_mmap_resource(struct file *file,
>   	return rc;
>   }
>   
> +#ifdef CONFIG_XEN_PRIVCMD_IRQFD
> +/* Irqfd support */
> +static struct workqueue_struct *irqfd_cleanup_wq;
> +static DEFINE_MUTEX(irqfds_lock);
> +static LIST_HEAD(irqfds_list);
> +
> +struct privcmd_kernel_irqfd {
> +	struct xen_dm_op_buf xbufs;
> +	domid_t dom;
> +	bool error;
> +	struct eventfd_ctx *eventfd;
> +	struct work_struct shutdown;
> +	wait_queue_entry_t wait;
> +	struct list_head list;
> +	poll_table pt;
> +};
> +
> +static void irqfd_deactivate(struct privcmd_kernel_irqfd *kirqfd)
> +{
> +	lockdep_assert_held(&irqfds_lock);
> +
> +	list_del_init(&kirqfd->list);
> +	queue_work(irqfd_cleanup_wq, &kirqfd->shutdown);
> +}
> +
> +static void irqfd_shutdown(struct work_struct *work)
> +{
> +	struct privcmd_kernel_irqfd *kirqfd =
> +		container_of(work, struct privcmd_kernel_irqfd, shutdown);
> +	u64 cnt;
> +
> +	eventfd_ctx_remove_wait_queue(kirqfd->eventfd, &kirqfd->wait, &cnt);
> +	eventfd_ctx_put(kirqfd->eventfd);
> +	kfree(kirqfd);
> +}
> +
> +static void irqfd_inject(struct privcmd_kernel_irqfd *kirqfd)
> +{
> +	u64 cnt;
> +	long rc;
> +
> +	eventfd_ctx_do_read(kirqfd->eventfd, &cnt);
> +
> +	xen_preemptible_hcall_begin();
> +	rc = HYPERVISOR_dm_op(kirqfd->dom, 1, &kirqfd->xbufs);
> +	xen_preemptible_hcall_end();
> +
> +	/* Don't repeat the error message for consecutive failures */
> +	if (rc && !kirqfd->error) {
> +		pr_err("Failed to configure irq for guest domain: %d\n",
> +		       kirqfd->dom);
> +	}
> +
> +	kirqfd->error = !!rc;

No need for the "!!".

> +}
> +
> +static int
> +irqfd_wakeup(wait_queue_entry_t *wait, unsigned int mode, int sync, void *key)
> +{
> +	struct privcmd_kernel_irqfd *kirqfd =
> +		container_of(wait, struct privcmd_kernel_irqfd, wait);
> +	__poll_t flags = key_to_poll(key);
> +
> +	if (flags & EPOLLIN)
> +		irqfd_inject(kirqfd);
> +
> +	if (flags & EPOLLHUP) {
> +		mutex_lock(&irqfds_lock);
> +		irqfd_deactivate(kirqfd);
> +		mutex_unlock(&irqfds_lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +irqfd_poll_func(struct file *file, wait_queue_head_t *wqh, poll_table *pt)
> +{
> +	struct privcmd_kernel_irqfd *kirqfd =
> +		container_of(pt, struct privcmd_kernel_irqfd, pt);
> +
> +	add_wait_queue_priority(wqh, &kirqfd->wait);
> +}
> +
> +static int privcmd_irqfd_assign(struct privcmd_irqfd *irqfd)
> +{
> +	struct privcmd_kernel_irqfd *kirqfd, *tmp;
> +	struct eventfd_ctx *eventfd;
> +	__poll_t events;
> +	struct fd f;
> +	void *dm_op;
> +	int ret;
> +
> +	kirqfd = kzalloc(sizeof(*kirqfd) + irqfd->size, GFP_KERNEL);
> +	if (!kirqfd)
> +		return -ENOMEM;
> +	dm_op = kirqfd + 1;
> +
> +	if (copy_from_user(dm_op, irqfd->dm_op, irqfd->size)) {
> +		ret = -EFAULT;
> +		goto error_kfree;
> +	}
> +
> +	kirqfd->xbufs.size = irqfd->size;
> +	set_xen_guest_handle(kirqfd->xbufs.h, dm_op);
> +	kirqfd->dom = irqfd->dom;
> +	INIT_LIST_HEAD(&kirqfd->list);

I don't think INIT_LIST_HEAD() is needed here. list_add_tail() doesn't
require that.

> +	INIT_WORK(&kirqfd->shutdown, irqfd_shutdown);
> +
> +	f = fdget(irqfd->fd);
> +	if (!f.file) {
> +		ret = -EBADF;
> +		goto error_kfree;
> +	}
> +
> +	eventfd = eventfd_ctx_fileget(f.file);
> +	if (IS_ERR(eventfd)) {
> +		ret = PTR_ERR(eventfd);
> +		goto error_fd_put;
> +	}
> +
> +	kirqfd->eventfd = eventfd;

I don't see why you need the local eventfd variable.

> +
> +	/*
> +	 * Install our own custom wake-up handling so we are notified via a
> +	 * callback whenever someone signals the underlying eventfd.
> +	 */
> +	init_waitqueue_func_entry(&kirqfd->wait, irqfd_wakeup);
> +	init_poll_funcptr(&kirqfd->pt, irqfd_poll_func);
> +
> +	mutex_lock(&irqfds_lock);
> +
> +	list_for_each_entry(tmp, &irqfds_list, list) {
> +		if (kirqfd->eventfd == tmp->eventfd) {
> +			ret = -EBUSY;
> +			mutex_unlock(&irqfds_lock);
> +			goto error_eventfd;
> +		}
> +	}
> +
> +	list_add_tail(&kirqfd->list, &irqfds_list);
> +	mutex_unlock(&irqfds_lock);
> +
> +	/*
> +	 * Check if there was an event already pending on the eventfd before we
> +	 * registered, and trigger it as if we didn't miss it.
> +	 */
> +	events = vfs_poll(f.file, &kirqfd->pt);
> +	if (events & EPOLLIN)
> +		irqfd_inject(kirqfd);
> +
> +	/*
> +	 * Do not drop the file until the kirqfd is fully initialized, otherwise
> +	 * we might race against the EPOLLHUP.
> +	 */
> +	fdput(f);
> +	return 0;
> +
> +error_eventfd:
> +	eventfd_ctx_put(eventfd);
> +
> +error_fd_put:
> +	fdput(f);
> +
> +error_kfree:
> +	kfree(kirqfd);
> +	return ret;
> +}

Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]