> On Mar 29, 2022, at 10:24 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Sat, Mar 26, 2022 at 11:47:36PM +0000, Jag Raman wrote:
>>
>>
>>> On Mar 7, 2022, at 5:24 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>>>
>>> On Thu, Feb 17, 2022 at 02:49:02AM -0500, Jagannathan Raman wrote:
>>>> Forward remote device's interrupts to the guest
>>>>
>>>> Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com>
>>>> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
>>>> Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
>>>> ---
>>>> include/hw/pci/pci.h | 6 ++
>>>> include/hw/remote/vfio-user-obj.h | 6 ++
>>>> hw/pci/msi.c | 13 +++-
>>>> hw/pci/msix.c | 12 +++-
>>>> hw/remote/machine.c | 11 +--
>>>> hw/remote/vfio-user-obj.c | 107 ++++++++++++++++++++++++++++++
>>>> stubs/vfio-user-obj.c | 6 ++
>>>> MAINTAINERS | 1 +
>>>> hw/remote/trace-events | 1 +
>>>> stubs/meson.build | 1 +
>>>> 10 files changed, 158 insertions(+), 6 deletions(-)
>>>> create mode 100644 include/hw/remote/vfio-user-obj.h
>>>> create mode 100644 stubs/vfio-user-obj.c
>>>>
>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>>> index c3f3c90473..d42d526a48 100644
>>>> --- a/include/hw/pci/pci.h
>>>> +++ b/include/hw/pci/pci.h
>>>> @@ -129,6 +129,8 @@ typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev,
>>>> typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
>>>> pcibus_t addr, pcibus_t size, int type);
>>>> typedef void PCIUnregisterFunc(PCIDevice *pci_dev);
>>>> +typedef void PCIMSINotify(PCIDevice *pci_dev, unsigned vector);
>>>> +typedef void PCIMSIxNotify(PCIDevice *pci_dev, unsigned vector);
>>>>
>>>> typedef struct PCIIORegion {
>>>> pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
>>>> @@ -323,6 +325,10 @@ struct PCIDevice {
>>>> /* Space to store MSIX table & pending bit array */
>>>> uint8_t *msix_table;
>>>> uint8_t *msix_pba;
>>>> +
>>>> + PCIMSINotify *msi_notify;
>>>> + PCIMSIxNotify *msix_notify;
>>>> +
>>>> /* MemoryRegion container for msix exclusive BAR setup */
>>>> MemoryRegion msix_exclusive_bar;
>>>> /* Memory Regions for MSIX table and pending bit entries. */
>>>> diff --git a/include/hw/remote/vfio-user-obj.h
>>>> b/include/hw/remote/vfio-user-obj.h
>>>> new file mode 100644
>>>> index 0000000000..87ab78b875
>>>> --- /dev/null
>>>> +++ b/include/hw/remote/vfio-user-obj.h
>>>> @@ -0,0 +1,6 @@
>>>> +#ifndef VFIO_USER_OBJ_H
>>>> +#define VFIO_USER_OBJ_H
>>>> +
>>>> +void vfu_object_set_bus_irq(PCIBus *pci_bus);
>>>> +
>>>> +#endif
>>>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
>>>> index 47d2b0f33c..93f5e400cc 100644
>>>> --- a/hw/pci/msi.c
>>>> +++ b/hw/pci/msi.c
>>>> @@ -51,6 +51,8 @@
>>>> */
>>>> bool msi_nonbroken;
>>>>
>>>> +static void pci_msi_notify(PCIDevice *dev, unsigned int vector);
>>>> +
>>>> /* If we get rid of cap allocator, we won't need this. */
>>>> static inline uint8_t msi_cap_sizeof(uint16_t flags)
>>>> {
>>>> @@ -225,6 +227,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>>>> dev->msi_cap = config_offset;
>>>> dev->cap_present |= QEMU_PCI_CAP_MSI;
>>>>
>>>> + dev->msi_notify = pci_msi_notify;
>>>
>>> Are you sure it's correct to skip the msi_is_masked() logic? I think the
>>> callback function should only override the behavior of
>>> msi_send_message(), not the entire msi_notify() function.
>>>
>>> The same applies to MSI-X.
>>
>> Hi Stefan,
>>
>> We noticed that the client is handling the masking and unmasking of MSIx
>> interrupts.
>>
>> Concerning MSIx, vfio_msix_vector_use() handles unmasking and
>> vfio_msix_vector_release() handles masking operations. The server triggers
>> an MSIx interrupt by signaling the eventfd associated with the vector. If
>> the vector
>> is unmasked, the interrupt bypasses the client/QEMU and takes this
>> path: “server -> KVM -> guest”. Whereas, if the vector is masked, it lands
>> on the
>> client via: “server -> vfio_msi_interrupt()”. vfio_msi_interrupt()
>> suppresses the
>> interrupt if the vector is masked. The use and release functions switch the
>> server’s
>> eventfd between VFIOPCIDevice->VFIOMSIVector[i]->kvm_interrupt and
>> VFIOPCIDevice->VFIOMSIVector[i]->interrupt using the
>> VFIO_DEVICE_SET_IRQS message.
>>
>> Concerning MSI, the server should check if the vector is unmasked before
>> triggering. The server is not doing it presently, will update it. For some
>> reason,
>> I had assumed that MSI handling is similar to MSIx in terms of masking -
>> sorry
>> about that. The masking and unmasking information for MSI is in the config
>> space
>> registers, so the server should have this information.
>>
>> You had previously suggested using callbacks for msi_get_message &
>> msi_send_message, considering the masking issue. Given MSIx masking
>> (including the MSIx table BAR) is handled at the client, the masking
>> information
>> doesn’t reach the server - so msix_notify will never invoke the
>> msi_send_message callback - all the vectors remain masked at the server
>> end (msix_init() -> msix_mask_all()).
>
> I was expecting vfio-user devices to be involved in MSI-X masking so
> they can implement the Pending bit semantics described in the spec:
>
> If a masked vector has its Pending bit Set, and the associated
> underlying interrupt events are somehow satisfied (usually by software
> though the exact manner is Function-specific), the Function must Clear
> the Pending bit, to avoid sending a spurious interrupt message later
> when software unmasks the vector.
>
> Does QEMU VFIO support this?
QEMU VFIO doesn’t seem to support it - I couldn’t find a place where
an assigned/passthru PCI device clears the pending bits in QEMU.
>
> What happens when a hw/net/e1000e_core.c vfio-user device uses
> msix_clr_pending() and related APIs?
>
> Also, having the vfio-user daemon write to the eventfd while the vector
> is masked is a waste of CPU cycles. The PCIe spec describes using MSI-X
> masking for poll mode operation and going via eventfd is suboptimal:
>
> Software is permitted to mask one or more vectors indefinitely, and
> service their associated interrupt events strictly based on polling
> their Pending bits. A Function must Set and Clear its Pending bits as
> necessary to support this “pure polling” mode of operation.
>
> Maybe the answer is these issues don't matter in practice because MSI-X
> masking is not used much?
From what I can tell, “pure polling” is used by ivshmem and virtio-pci devices
in QEMU.
e1000e doesn’t use “pure polling”, but it does clear pending interrupts.
John Johnson, John Levon & Thanos,
Any thoughts?
Thank you!
--
Jag
>
> Stefan
