Re: [PATCH v2 15/23] vfio-user: forward msix BAR accesses to server

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 8 Feb 2023 06:38:30 +0000
John Johnson <john.g.johnson@oracle.com> wrote:

> > On Feb 6, 2023, at 12:33 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> > 
> > On Wed,  1 Feb 2023 21:55:51 -0800
> > John Johnson <john.g.johnson@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.johnson@oracle.com>
> >> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> >> Signed-off-by: Jagannathan Raman <jag.raman@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.
> >   
> 
> 	It is only used for MSIX masking.  MSI masking is done with
> config space stores, and vfio-kernel and vfio-user handle them the
> same by forwarding the config space writes to the server so it can
> mask interrupts.

I suppose this does slip through on vfio-kernel, though support via
SET_IRQS would really be the uAPI mechanism we'd expect to use for
masking, just as it is for enabling/disabling MSI.  MSI is not well
used enough to have flushed that out and it seems harmless, but is not
the intended API.

> 	MSIX is trickier because the mask bits are in a memory region.
> vfio-kernel doesn’t support SET_IRQS on MSIX vectors, so if the host
> doesn’t allow client mapping of the MSIX table to do the masking, vfio
> switches a masked vector’s event FD destination from KVM to QEMU, then
> uses the QEMU PCI emulation to mask the vector.

Lack of support for MSIX ACTION_[UN]MASK is not an API limitation, it's
an implementation issue in the kernel.  Same for MSI.  I believe this
is resolved now, that there are mask/unmask APIs available within the
kernel, as well as mechanisms to avoid the NORESIZE behavior now, so
the intention would be to implement that support, along with a
mechanism for the user to know the support is present.  We already have
that for NORESIZE via IRQ_INFO, so I suspect the right answer might be
to add a new VFIO_IRQ_INFO_MSI_MASK to advertise masking support.
 
> 	vfio-user has to use messages to mask MSIX vectors, since there
> is no host config space to map.  I originally forwarded the MSIX table
> writes to the server to do the masking, but the feedback on the vfio-user
> server changes was to add SET_IRQS support.

Which is what I'm describing above, QEMU should know via the VFIO uAPI
whether MSI/X masking is supported and use that to configure the code
to make use of it rather than some inferred value based on the
interface type.  Thanks,

Alex