On Wed, 1 Feb 2023 21:55:51 -0800
John Johnson <john.g.john...@oracle.com> wrote:
> Server holds device current device pending state
> Use irq masking commands in socket case
>
> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
> ---
> hw/vfio/pci.h | 1 +
> include/hw/vfio/vfio-common.h | 3 ++
> hw/vfio/ccw.c | 1 +
> hw/vfio/common.c | 26 ++++++++++++++++++
> hw/vfio/pci.c | 23 +++++++++++++++-
> hw/vfio/platform.c | 1 +
> hw/vfio/user-pci.c | 64
> +++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 4f70664..d3e5d5f 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
> uint32_t table_offset;
> uint32_t pba_offset;
> unsigned long *pending;
> + MemoryRegion *pba_region;
> } VFIOMSIXInfo;
>
> /*
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index bbc4b15..2c58d7d 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -143,6 +143,7 @@ typedef struct VFIODevice {
> bool ram_block_discard_allowed;
> bool enable_migration;
> bool use_regfds;
> + bool can_mask_irq;
How can this be a device level property? vfio-pci (kernel) supports
masking of INTx. It seems like there needs to be a per-index info or
probe here to to support this.
> VFIODeviceOps *ops;
> VFIODeviceIO *io;
> unsigned int num_irqs;
> @@ -239,6 +240,8 @@ void vfio_put_base_device(VFIODevice *vbasedev);
> void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
> +void vfio_unmask_single_irq(VFIODevice *vbasedev, int index, int irq);
> +void vfio_mask_single_irq(VFIODevice *vbasedev, int index, int irq);
> int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> int action, int fd, Error **errp);
> void vfio_region_write(void *opaque, hwaddr addr,
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 00605bd..bf67670 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -616,6 +616,7 @@ static void vfio_ccw_get_device(VFIOGroup *group,
> VFIOCCWDevice *vcdev,
> vcdev->vdev.dev = &vcdev->cdev.parent_obj.parent_obj;
> vcdev->vdev.io = &vfio_dev_io_ioctl;
> vcdev->vdev.use_regfds = false;
> + vcdev->vdev.can_mask_irq = false;
>
> return;
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index de64e53..0c1cb21 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -102,6 +102,32 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev, int
> index)
> vbasedev->io->set_irqs(vbasedev, &irq_set);
> }
>
> +void vfio_mask_single_irq(VFIODevice *vbasedev, int index, int irq)
> +{
> + struct vfio_irq_set irq_set = {
> + .argsz = sizeof(irq_set),
> + .flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_MASK,
> + .index = index,
> + .start = irq,
> + .count = 1,
> + };
> +
> + vbasedev->io->set_irqs(vbasedev, &irq_set);
> +}
> +
> +void vfio_unmask_single_irq(VFIODevice *vbasedev, int index, int irq)
> +{
> + struct vfio_irq_set irq_set = {
> + .argsz = sizeof(irq_set),
> + .flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK,
> + .index = index,
> + .start = irq,
> + .count = 1,
> + };
> +
> + vbasedev->io->set_irqs(vbasedev, &irq_set);
> +}
> +
> static inline const char *action_to_str(int action)
> {
> switch (action) {
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 42e7c82..7b16f8f 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -477,6 +477,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> {
> VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
> VFIOMSIVector *vector;
> + bool new_vec = false;
> int ret;
>
> trace_vfio_msix_vector_do_use(vdev->vbasedev.name, nr);
> @@ -490,6 +491,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> error_report("vfio: Error: event_notifier_init failed");
> }
> vector->use = true;
> + new_vec = true;
> msix_vector_use(pdev, nr);
> }
>
> @@ -516,6 +518,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> kvm_irqchip_commit_route_changes(&vfio_route_change);
> vfio_connect_kvm_msi_virq(vector);
> }
> + new_vec = true;
This looks wrong to me, can't we get here when a previously used vector
is unmasked?
> }
> }
>
> @@ -523,6 +526,8 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> * We don't want to have the host allocate all possible MSI vectors
> * for a device if they're not in use, so we shutdown and incrementally
> * increase them as needed.
> + * Otherwise, unmask the vector if the vector is already setup (and we
> can
> + * do so) or send the fd if not.
> */
> if (vdev->nr_vectors < nr + 1) {
> vdev->nr_vectors = nr + 1;
> @@ -533,6 +538,8 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> error_report("vfio: failed to enable vectors, %d", ret);
> }
> }
> + } else if (vdev->vbasedev.can_mask_irq && !new_vec) {
> + vfio_unmask_single_irq(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr);
The correctness of @new_vec is doubtful here, but masking support must
be in reference to a specific IRQ index. INTx masking is implicit, but
we'd probably need something on the VFIOPCIDevice to indicate support
for MSI and MSIX masking support, separately.
> } else {
> Error *err = NULL;
> int32_t fd;
> @@ -574,6 +581,12 @@ static void vfio_msix_vector_release(PCIDevice *pdev,
> unsigned int nr)
>
> trace_vfio_msix_vector_release(vdev->vbasedev.name, nr);
>
> + /* just mask vector if peer supports it */
> + if (vdev->vbasedev.can_mask_irq) {
> + vfio_mask_single_irq(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr);
> + return;
> + }
> +
> /*
> * There are still old guests that mask and unmask vectors on every
> * interrupt. If we're using QEMU bypass with a KVM irqfd, leave all of
> @@ -644,7 +657,7 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
> if (ret) {
> error_report("vfio: failed to enable vectors, %d", ret);
> }
> - } else {
> + } else if (!vdev->vbasedev.can_mask_irq) {
> /*
> * Some communication channels between VF & PF or PF & fw rely on the
> * physical state of the device and expect that enabling MSI-X from
> the
> @@ -660,6 +673,13 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
> */
> vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
> vfio_msix_vector_release(&vdev->pdev, 0);
> + } else {
> + /*
> + * If we can use irq masking, send an invalid fd on vector 0
> + * to enable MSI-X without any vectors enabled.
> + */
> + vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, 0,
> + VFIO_IRQ_SET_ACTION_TRIGGER, -1, NULL);
What does this have to do with masking? I'm not entirely sure this
doesn't also work on vfio-pci (kernel).
In general this feels like a special cased version of MSI/X masking
support rather than actually reworking things to support underlying
masking support for these indexes. Thanks,
Alex
