| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240205153551.429d9d76.alex.williamson@redhat.com>
Date: Mon, 5 Feb 2024 15:35:51 -0700
From: Alex Williamson <alex.williamson@...hat.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: jgg@...dia.com, yishaih@...dia.com,
shameerali.kolothum.thodi@...wei.com, kevin.tian@...el.com,
kvm@...r.kernel.org, dave.jiang@...el.com, ashok.raj@...el.com,
linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH 17/17] vfio/pci: Remove duplicate interrupt management
flow
On Thu, 1 Feb 2024 20:57:11 -0800
Reinette Chatre <reinette.chatre@...el.com> wrote:
> vfio_pci_set_intx_trigger() and vfio_pci_set_trigger() have the
> same flow that calls interrupt type (INTx, MSI, MSI-X) specific
> functions.
>
> Create callbacks for the interrupt type specific code that
> can be called by the shared code so that only one of these functions
> are needed. Rename the final generic function shared by all
> interrupt types vfio_pci_set_trigger().
>
> Relocate the "IOCTL support" marker to correctly mark the
> now generic code.
>
> Signed-off-by: Reinette Chatre <reinette.chatre@...el.com>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 104 ++++++++++--------------------
> 1 file changed, 35 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index daa84a317f40..a5b337cfae60 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -32,6 +32,12 @@ struct vfio_pci_irq_ctx {
> };
>
> struct vfio_pci_intr_ops {
> + int (*enable)(struct vfio_pci_core_device *vdev, unsigned int start,
> + unsigned int count, unsigned int index);
> + void (*disable)(struct vfio_pci_core_device *vdev,
> + unsigned int index);
> + void (*send_eventfd)(struct vfio_pci_core_device *vdev,
> + struct vfio_pci_irq_ctx *ctx);
> int (*request_interrupt)(struct vfio_pci_core_device *vdev,
> struct vfio_pci_irq_ctx *ctx,
> unsigned int vector, unsigned int index);
> @@ -356,6 +362,12 @@ static void vfio_intx_disable(struct vfio_pci_core_device *vdev,
> /*
> * MSI/MSI-X
> */
> +static void vfio_send_msi_eventfd(struct vfio_pci_core_device *vdev,
> + struct vfio_pci_irq_ctx *ctx)
> +{
> + eventfd_signal(ctx->trigger);
> +}
> +
> static irqreturn_t vfio_msihandler(int irq, void *arg)
> {
> struct eventfd_ctx *trigger = arg;
> @@ -544,13 +556,22 @@ static void vfio_msi_unregister_producer(struct vfio_pci_irq_ctx *ctx)
> irq_bypass_unregister_producer(&ctx->producer);
> }
>
> +/*
> + * IOCTL support
> + */
> static struct vfio_pci_intr_ops intr_ops[] = {
> [VFIO_PCI_INTX_IRQ_INDEX] = {
> + .enable = vfio_intx_enable,
> + .disable = vfio_intx_disable,
> + .send_eventfd = vfio_send_intx_eventfd_ctx,
> .request_interrupt = vfio_intx_request_interrupt,
> .free_interrupt = vfio_intx_free_interrupt,
> .device_name = vfio_intx_device_name,
> },
> [VFIO_PCI_MSI_IRQ_INDEX] = {
> + .enable = vfio_msi_enable,
> + .disable = vfio_msi_disable,
> + .send_eventfd = vfio_send_msi_eventfd,
> .request_interrupt = vfio_msi_request_interrupt,
> .free_interrupt = vfio_msi_free_interrupt,
> .device_name = vfio_msi_device_name,
> @@ -558,6 +579,9 @@ static struct vfio_pci_intr_ops intr_ops[] = {
> .unregister_producer = vfio_msi_unregister_producer,
> },
> [VFIO_PCI_MSIX_IRQ_INDEX] = {
> + .enable = vfio_msi_enable,
> + .disable = vfio_msi_disable,
> + .send_eventfd = vfio_send_msi_eventfd,
> .request_interrupt = vfio_msi_request_interrupt,
> .free_interrupt = vfio_msi_free_interrupt,
> .device_name = vfio_msi_device_name,
> @@ -646,9 +670,6 @@ static int vfio_irq_set_block(struct vfio_pci_core_device *vdev,
> return ret;
> }
>
> -/*
> - * IOCTL support
> - */
> static int vfio_pci_set_intx_unmask(struct vfio_pci_core_device *vdev,
> unsigned int index, unsigned int start,
> unsigned int count, uint32_t flags,
> @@ -701,71 +722,16 @@ static int vfio_pci_set_intx_mask(struct vfio_pci_core_device *vdev,
> return 0;
> }
>
> -static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> - unsigned int index, unsigned int start,
> - unsigned int count, uint32_t flags,
> - void *data)
> -{
> - struct vfio_pci_irq_ctx *ctx;
> - unsigned int i;
> -
> - if (is_intx(vdev) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> - vfio_intx_disable(vdev, index);
> - return 0;
> - }
> -
> - if (!(is_intx(vdev) || is_irq_none(vdev)))
> - return -EINVAL;
> -
> - if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> - int32_t *fds = data;
> - int ret;
> -
> - if (is_intx(vdev))
> - return vfio_irq_set_block(vdev, start, count, fds, index);
> -
> - ret = vfio_intx_enable(vdev, start, count, index);
> - if (ret)
> - return ret;
> -
> - ret = vfio_irq_set_block(vdev, start, count, fds, index);
> - if (ret)
> - vfio_intx_disable(vdev, index);
> -
> - return ret;
> - }
> -
> - if (!is_intx(vdev))
> - return -EINVAL;
> -
> - /* temporary */
> - for (i = start; i < start + count; i++) {
> - ctx = vfio_irq_ctx_get(vdev, i);
> - if (!ctx || !ctx->trigger)
> - continue;
> - if (flags & VFIO_IRQ_SET_DATA_NONE) {
> - vfio_send_intx_eventfd_ctx(vdev, ctx);
> - } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> - uint8_t *bools = data;
> -
> - if (bools[i - start])
> - vfio_send_intx_eventfd_ctx(vdev, ctx);
> - }
> - }
> -
> - return 0;
> -}
> -
> -static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
> - unsigned int index, unsigned int start,
> - unsigned int count, uint32_t flags,
> - void *data)
> +static int vfio_pci_set_trigger(struct vfio_pci_core_device *vdev,
> + unsigned int index, unsigned int start,
> + unsigned int count, uint32_t flags,
> + void *data)
> {
> struct vfio_pci_irq_ctx *ctx;
> unsigned int i;
>
> if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> - vfio_msi_disable(vdev, index);
> + intr_ops[index].disable(vdev, index);
> return 0;
> }
>
> @@ -780,13 +746,13 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
> return vfio_irq_set_block(vdev, start, count,
> fds, index);
>
> - ret = vfio_msi_enable(vdev, start, count, index);
> + ret = intr_ops[index].enable(vdev, start, count, index);
> if (ret)
> return ret;
>
> ret = vfio_irq_set_block(vdev, start, count, fds, index);
> if (ret)
> - vfio_msi_disable(vdev, index);
> + intr_ops[index].disable(vdev, index);
>
> return ret;
> }
> @@ -799,11 +765,11 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
> if (!ctx || !ctx->trigger)
> continue;
> if (flags & VFIO_IRQ_SET_DATA_NONE) {
> - eventfd_signal(ctx->trigger);
> + intr_ops[index].send_eventfd(vdev, ctx);
> } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> uint8_t *bools = data;
Nit, an opportunity to add the missing new line between variable
declaration and code here. Thanks,
Alex
> if (bools[i - start])
> - eventfd_signal(ctx->trigger);
> + intr_ops[index].send_eventfd(vdev, ctx);
> }
> }
> return 0;
> @@ -912,7 +878,7 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_core_device *vdev, uint32_t flags,
> func = vfio_pci_set_intx_unmask;
> break;
> case VFIO_IRQ_SET_ACTION_TRIGGER:
> - func = vfio_pci_set_intx_trigger;
> + func = vfio_pci_set_trigger;
> break;
> }
> break;
> @@ -924,7 +890,7 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_core_device *vdev, uint32_t flags,
> /* XXX Need masking support exported */
> break;
> case VFIO_IRQ_SET_ACTION_TRIGGER:
> - func = vfio_pci_set_msi_trigger;
> + func = vfio_pci_set_trigger;
> break;
> }
> break;
Powered by blists - more mailing lists