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

[John Johnson]

> On Feb 8, 2023, at 1:30 PM, Alex Williamson <alex.williamson@redhat.com> 
> wrote:
> 
> 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.
> 

        I looked at the linux next vfio kernel driver, and it doesn't seem
to support MSI/X masking.


>>      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,
> 

        The kernel doesn’t advertising it working either, so I think I can
key it off the presence of VFIO_IRQ_INFO_MASKABLE in irq_info

                                                        JJ