[Qemu-devel] [PATCH RFC 0/2] virtio-pci: polling mode support

[Qemu-devel] [PATCH RFC 0/2] virtio-pci: polling mode support

[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.

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?

-- 
1.7.5.53.gc233e

[PATCH RFC 0/2] virtio-pci: polling mode support

[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.

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?

-- 
1.7.5.53.gc233e

[Qemu-devel] [PATCH (repost) RFC 1/2] virtio: ISR bit constants

[Michael S. Tsirkin]

Add constants for ISR bits values, and use them in virtio.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio.c |    7 ++++---
 hw/virtio.h |    5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 7011b5b..74627e4 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -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);
 }
 
diff --git a/hw/virtio.h b/hw/virtio.h
index 2d18209..1bb6210 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -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. */
-- 
1.7.5.53.gc233e

[PATCH (repost) RFC 1/2] virtio: ISR bit constants

[Michael S. Tsirkin]

Add constants for ISR bits values, and use them in virtio.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio.c |    7 ++++---
 hw/virtio.h |    5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 7011b5b..74627e4 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -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);
 }
 
diff --git a/hw/virtio.h b/hw/virtio.h
index 2d18209..1bb6210 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -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. */
-- 
1.7.5.53.gc233e

[Qemu-devel] [PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read

[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 <mst@redhat.com>
---
 hw/msix.c       |   26 ++++++++++++++++++++++++++
 hw/msix.h       |    3 +++
 hw/virtio-pci.c |   11 ++++++++++-
 3 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 63b41b9..fe967c9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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))
diff --git a/hw/msix.h b/hw/msix.h
index 7e04336..86a92b1 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -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;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index df27c19..cab7dde 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -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;
-- 
1.7.5.53.gc233e

[PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read

[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 <mst@redhat.com>
---
 hw/msix.c       |   26 ++++++++++++++++++++++++++
 hw/msix.h       |    3 +++
 hw/virtio-pci.c |   11 ++++++++++-
 3 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 63b41b9..fe967c9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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))
diff --git a/hw/msix.h b/hw/msix.h
index 7e04336..86a92b1 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -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;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index df27c19..cab7dde 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -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;
-- 
1.7.5.53.gc233e

Re: [Qemu-devel] [PATCH RFC 0/2] virtio-pci: polling mode support

[Rusty Russell]

On Wed, 2 Nov 2011 21:02:42 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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?

This seems sane to me...

Cheers,
Rusty.

Re: [PATCH RFC 0/2] virtio-pci: polling mode support

[Rusty Russell]

On Wed, 2 Nov 2011 21:02:42 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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?

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

[Jan Kiszka]

On 2011-11-02 21:11, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
>  hw/msix.c       |   26 ++++++++++++++++++++++++++
>  hw/msix.h       |    3 +++
>  hw/virtio-pci.c |   11 ++++++++++-
>  3 files changed, 39 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 63b41b9..fe967c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -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;
> +}

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read

[Jan Kiszka]

On 2011-11-02 21:11, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
>  hw/msix.c       |   26 ++++++++++++++++++++++++++
>  hw/msix.h       |    3 +++
>  hw/virtio-pci.c |   11 ++++++++++-
>  3 files changed, 39 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 63b41b9..fe967c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -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;
> +}

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

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

[Michael S. Tsirkin]

On Thu, Nov 03, 2011 at 12:42:55PM +0100, Jan Kiszka wrote:
> On 2011-11-02 21:11, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > ---
> >  hw/msix.c       |   26 ++++++++++++++++++++++++++
> >  hw/msix.h       |    3 +++
> >  hw/virtio-pci.c |   11 ++++++++++-
> >  3 files changed, 39 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 63b41b9..fe967c9 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -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;
> > +}
> 
> 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).

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?


> 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

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?

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read

[Michael S. Tsirkin]

On Thu, Nov 03, 2011 at 12:42:55PM +0100, Jan Kiszka wrote:
> On 2011-11-02 21:11, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > ---
> >  hw/msix.c       |   26 ++++++++++++++++++++++++++
> >  hw/msix.h       |    3 +++
> >  hw/virtio-pci.c |   11 ++++++++++-
> >  3 files changed, 39 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 63b41b9..fe967c9 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -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;
> > +}
> 
> 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).

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?


> 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

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?

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

[Jan Kiszka]

On 2011-11-03 13:07, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2011 at 12:42:55PM +0100, Jan Kiszka wrote:
>> On 2011-11-02 21:11, Michael S. Tsirkin wrote:
>>> 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 <mst@redhat.com>
>>> ---
>>>  hw/msix.c       |   26 ++++++++++++++++++++++++++
>>>  hw/msix.h       |    3 +++
>>>  hw/virtio-pci.c |   11 ++++++++++-
>>>  3 files changed, 39 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/msix.c b/hw/msix.c
>>> index 63b41b9..fe967c9 100644
>>> --- a/hw/msix.c
>>> +++ b/hw/msix.c
>>> @@ -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;
>>> +}
>>
>> 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).
> 
> 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.

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH (repost) RFC 2/2] virtio-pci: recall and return msix notifications on ISR read

[Jan Kiszka]

On 2011-11-03 13:07, Michael S. Tsirkin wrote:
> On Thu, Nov 03, 2011 at 12:42:55PM +0100, Jan Kiszka wrote:
>> On 2011-11-02 21:11, Michael S. Tsirkin wrote:
>>> 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 <mst@redhat.com>
>>> ---
>>>  hw/msix.c       |   26 ++++++++++++++++++++++++++
>>>  hw/msix.h       |    3 +++
>>>  hw/virtio-pci.c |   11 ++++++++++-
>>>  3 files changed, 39 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/msix.c b/hw/msix.c
>>> index 63b41b9..fe967c9 100644
>>> --- a/hw/msix.c
>>> +++ b/hw/msix.c
>>> @@ -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;
>>> +}
>>
>> 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).
> 
> 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.

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux