virtio-pci: polling mode support
Michael S. Tsirkin
maintainercover virtio-pci: polling mode support
MSIX spec requires that device can be operated with all vectors masked, by polling.
So the following patchset (lightly tested) adds this
ability: when driver reads ISR, the device recalls a pending notification, and returns pending status in the ISR register.
The polling driver can operate as follows:
- map all VQs and config to the same vector
- poll ISR to get status - this also flushes VQ updates to memory
- handle config change or VQ event depending on ISR
Comments?
Signature · 1 line
1.7.5.53.gc233e
Michael S. Tsirkin
maintainerRFC PATCH 1/2 virtio: ISR bit constants
Add constants for ISR bits values, and use them in virtio.
--- hw/virtio.c | 7 ++++--- hw/virtio.h | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-)
hw/virtio.c +4 -3
@@ -672,7 +672,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,void virtio_irq (VirtQueue *vq) {trace_virtio_irq (vq); - vq->vdev->isr |=0x01 ; + vq->vdev->isr |=VIRTIO_ISR_VQ ;virtio_notify_vector (vq->vdev, vq->vector); }
@@ -717,7 +717,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq) }trace_virtio_notify (vdev, vq); - vdev->isr |=0x01 ; + vdev->isr |=VIRTIO_ISR_VQ ;virtio_notify_vector (vdev, vq->vector); }
@@ -726,7 +726,8 @@ void virtio_notify_config(VirtIODevice *vdev)if (!(vdev->status &VIRTIO_CONFIG_S_DRIVER_OK ))return ; - vdev->isr |=0x03 ; +/* TODO: why do we set VIRTIO_ISR_VQ here? */ + vdev->isr |=VIRTIO_ISR_VQ |VIRTIO_ISR_CONFIG ;virtio_notify_vector (vdev, vdev->config_vector); }
hw/virtio.h +5 -0
@@ -54,6 +54,11 @@/* A guest should never accept this. It implies negotiation is broken. */ #define VIRTIO_F_BAD_FEATURE 30 +/* The bit of the ISR which indicates a device vq event. */ +#define VIRTIO_ISR_VQ 0x1 +/* The bit of the ISR which indicates a device configuration change. */ +#define VIRTIO_ISR_CONFIG 0x2 +/* from Linux's linux/virtio_ring.h */ /* This marks a buffer as continuing via the next field. */
Signature · 1 line
1.7.5.53.gc233e
Michael S. Tsirkin
maintainerRFC PATCH 2/2 virtio-pci: recall and return msix notifications on ISR read
In reply to Michael S. TsirkinMSIX spec requires that device can be operated with all vectors masked, by polling pending bits.
Add APIs to recall an msix notification, and make polling mode possible in virtio-pci by clearing the pending bits and setting ISR appropriately on ISR read.
--- hw/msix.c | 26 ++++++++++++++++++++++++++ hw/msix.h | 3 +++ hw/virtio-pci.c | 11 ++++++++++- 3 files changed, 39 insertions(+), 1 deletions(-)
hw/msix.c +26 -0
@@ -349,6 +349,32 @@ void msix_notify(PCIDevice *dev, unsigned vector)stl_le_phys (address, data); } +/* Recall outstanding MSI-X notifications for a vector, if possible. +* Return true if any were outstanding. */ +bool msix_recall (PCIDevice *dev,unsigned vector) +{ +bool ret; +if (vector >= dev->msix_entries_nr) +return false; + ret =msix_is_pending (dev, vector); +msix_clr_pending (dev, vector); +return ret; +} + +/* Recall outstanding MSI-X notifications for all vectors, if possible. +* Return true if any were outstanding. */ +bool msix_recall_all (PCIDevice *dev) +{ +uint8_t ret =0 ; +uint8_t *b; +for (b = dev->msix_table_page +MSIX_PAGE_PENDING ; + b <=msix_pending_byte (dev, dev->msix_entries_nr -1 ); ++b) { + ret |= *b; + *b =0 ; + } +return ret; +} +void msix_reset (PCIDevice *dev) {if (!(dev->cap_present &QEMU_PCI_CAP_MSIX ))
hw/msix.h +3 -0
@@ -27,6 +27,9 @@ void msix_unuse_all_vectors(PCIDevice *dev);void msix_notify (PCIDevice *dev,unsigned vector); +bool msix_recall (PCIDevice *dev,unsigned vector); +bool msix_recall_all (PCIDevice *dev); +void msix_reset (PCIDevice *dev);extern int msix_supported;
hw/virtio-pci.c +10 -1
@@ -393,7 +393,16 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)/* reading from the ISR also clears it. */ ret = vdev->isr; vdev->isr =0 ; -qemu_set_irq (proxy->pci_dev.irq[0 ],0 ); +if (msix_enabled (&proxy->pci_dev)) { +if (msix_recall (&proxy->pci_dev, vdev->config_vector)) { + ret |=VIRTIO_ISR_CONFIG ; + } +if (msix_recall_all (&proxy->pci_dev)) { + ret |=VIRTIO_ISR_VQ ; + } + }else { +qemu_set_irq (proxy->pci_dev.irq[0 ],0 ); + }break ;case VIRTIO_MSI_CONFIG_VECTOR : ret = vdev->config_vector;
Signature · 1 line
1.7.5.53.gc233e
Quoted 8 lines · Michael S. Tsirkin
MSIX spec requires that device can be operated with all vectors masked, by polling.
So the following patchset (lightly tested) adds this
ability: when driver reads ISR, the device recalls a pending notification, and returns pending status in the ISR register.
Quoted 6 lines
The polling driver can operate as follows:
- map all VQs and config to the same vector
- poll ISR to get status - this also flushes VQ updates to memory
- handle config change or VQ event depending on ISRComments?
This seems sane to me...
Cheers,
Rusty.
Quoted 8 lines · Michael S. Tsirkin
MSIX spec requires that device can be operated with all vectors masked, by polling.
So the following patchset (lightly tested) adds this
ability: when driver reads ISR, the device recalls a pending notification, and returns pending status in the ISR register.
Quoted 6 lines
The polling driver can operate as follows:
- map all VQs and config to the same vector
- poll ISR to get status - this also flushes VQ updates to memory
- handle config change or VQ event depending on ISRComments?
This seems sane to me...
Cheers,
Rusty.
Re: [Qemu-devel] [PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read
In reply to Michael S. TsirkinQuoted 22 lines · Michael S. Tsirkin
MSIX spec requires that device can be operated with all vectors masked, by polling pending bits.
Add APIs to recall an msix notification, and make polling mode possible in virtio-pci by clearing the pending bits and setting ISR appropriately on ISR read.Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/msix.c | 26 ++++++++++++++++++++++++++ hw/msix.h | 3 +++ hw/virtio-pci.c | 11 ++++++++++- 3 files changed, 39 insertions(+), 1 deletions(-)hw/msix.c +1
-0@@ -349,6 +349,32 @@ void msix_notify(PCIDevice *dev, unsigned vector)stl_le_phys (address, data); } +/* Recall outstanding MSI-X notifications for a vector, if possible.
hw/msix.c void msix_notify(PCIDevice *dev, unsigned vector) · line 362
Quoted 10 lines
hw/msix.c +10
-0+* Return true if any were outstanding. */ +bool msix_recall (PCIDevice *dev,unsigned vector) +{ +bool ret; +if (vector >= dev->msix_entries_nr) +return false; + ret =msix_is_pending (dev, vector); +msix_clr_pending (dev, vector); +return ret; +}
I would prefer to have a single API instead to clarify the tight relation:
bool msi[x]_set_notify(PCIDevice *dev, unsigned vector, unsigned level)
Would return true for level=1 if the message was either sent directly or queued (we could deliver false if it was already queued, but I see no use case for this yet).
Also, I don't see the generic value of some msix_recall_all. I think it's better handled in a single loop over all vectors at caller site, clearing the individual interrupt reason bits on a per-vector basis there. msix_recall_all is only useful in the virtio case where you have one vector of reason A and all the rest of B. Once you had multiple reason C vectors as well, it would not help anymore.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Michael S. Tsirkin
maintainerRe: [Qemu-devel] [PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read
Quoted 36 lines · Jan Kiszka
Quoted 22 lines · Michael S. Tsirkin
MSIX spec requires that device can be operated with all vectors masked, by polling pending bits.
Add APIs to recall an msix notification, and make polling mode possible in virtio-pci by clearing the pending bits and setting ISR appropriately on ISR read.Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/msix.c | 26 ++++++++++++++++++++++++++ hw/msix.h | 3 +++ hw/virtio-pci.c | 11 ++++++++++- 3 files changed, 39 insertions(+), 1 deletions(-)hw/msix.c +1
-0@@ -349,6 +349,32 @@ void msix_notify(PCIDevice *dev, unsigned vector)stl_le_phys (address, data); } +/* Recall outstanding MSI-X notifications for a vector, if possible. hw/msix.c void msix_notify(PCIDevice *dev, unsigned vector) · line 362
Quoted 10 lines
hw/msix.c +10
-0+* Return true if any were outstanding. */ +bool msix_recall (PCIDevice *dev,unsigned vector) +{ +bool ret; +if (vector >= dev->msix_entries_nr) +return false; + ret =msix_is_pending (dev, vector); +msix_clr_pending (dev, vector); +return ret; +}I would prefer to have a single API instead to clarify the tight relation:
Quoted 5 lines
bool msi[x]_set_notify(PCIDevice *dev, unsigned vector, unsigned level)
Would return true for level=1 if the message was either sent directly or queued (we could deliver false if it was already queued, but I see no use case for this yet).
It's a matter of taste: some people like functions with flags, some prefer separate functions. I really prefer two functions.
But I agree it woulkd be better to have a name that makes it clear that what we recall is a notification. msix_notify_queue/msix_notify_dequeue?
Quoted 8 lines
Also, I don't see the generic value of some
msix_recall_all. I think it's better handled in a single loop over all vectors at caller site, clearing the individual interrupt reason bits on a per-vector basis there.msix_recall_allis only useful in the virtio case where you have one vector of reason A and all the rest of B. Once you had multiple reason C vectors as well, it would not help anymore.Jan
The reason I wanted to have it is to reduce the overhead this adds:
since PBA is packed, it's much faster to check whether any bits are set than by going through them all, one by one. Typically all PBA bits are clear ...
I agree it might not help non-virtio devices, but to me it looks like a harmless little helper - what's the issue with it?
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read
Quoted 43 lines · Michael S. Tsirkin
Quoted 36 lines · Jan Kiszka
Quoted 22 lines · Michael S. Tsirkin
MSIX spec requires that device can be operated with all vectors masked, by polling pending bits.
Add APIs to recall an msix notification, and make polling mode possible in virtio-pci by clearing the pending bits and setting ISR appropriately on ISR read.Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/msix.c | 26 ++++++++++++++++++++++++++ hw/msix.h | 3 +++ hw/virtio-pci.c | 11 ++++++++++- 3 files changed, 39 insertions(+), 1 deletions(-)hw/msix.c +1
-0@@ -349,6 +349,32 @@ void msix_notify(PCIDevice *dev, unsigned vector)stl_le_phys (address, data); } +/* Recall outstanding MSI-X notifications for a vector, if possible. hw/msix.c void msix_notify(PCIDevice *dev, unsigned vector) · line 362
Quoted 10 lines
hw/msix.c +10
-0+* Return true if any were outstanding. */ +bool msix_recall (PCIDevice *dev,unsigned vector) +{ +bool ret; +if (vector >= dev->msix_entries_nr) +return false; + ret =msix_is_pending (dev, vector); +msix_clr_pending (dev, vector); +return ret; +}I would prefer to have a single API instead to clarify the tight relation:
Quoted 5 lines
bool msi[x]_set_notify(PCIDevice *dev, unsigned vector, unsigned level)
Would return true for level=1 if the message was either sent directly or queued (we could deliver false if it was already queued, but I see no use case for this yet).
Quoted 6 lines
It's a matter of taste: some people like functions with flags, some prefer separate functions. I really prefer two functions.
But I agree it woulkd be better to have a name that makes it clear that what we recall is a notification. msix_notify_queue/msix_notify_dequeue?
OK, that doesn't sound bad.
Quoted 11 lines
Quoted 10 lines
Also, I don't see the generic value of some
msix_recall_all. I think it's better handled in a single loop over all vectors at caller site, clearing the individual interrupt reason bits on a per-vector basis there.msix_recall_allis only useful in the virtio case where you have one vector of reason A and all the rest of B. Once you had multiple reason C vectors as well, it would not help anymore.Jan
Quoted 7 lines
The reason I wanted to have it is to reduce the overhead this adds:
since PBA is packed, it's much faster to check whether any bits are set than by going through them all, one by one. Typically all PBA bits are clear ...I agree it might not help non-virtio devices, but to me it looks like a harmless little helper - what's the issue with it?
*If* there is a noticeable performance gain, I'm fine with msix_notify_dequeue_all (about how may vectors are we talking in the vitio case?). But the code would be more regular the other way around.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux