| Message ID | 6dc9aa9764b1cfddf557a98f269e0f7d31ce03ac.1320259840.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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;
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(-)