[for-1.2] msix: make [un]use vectors on reset/load optional

Submitted by Michael S. Tsirkin on Aug. 29, 2012, 4:40 p.m.

Details

Message ID 20120829164056.GA7792@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Aug. 29, 2012, 4:40 p.m.
The facility to use/unuse vectors dynamically is helpful
for virtio but little else: everyone just seems to use
vectors in their init function.

Avoid clearing msix vector use info on reset and load.
For virtio, clear it explicitly.
This should fix regressions reported with ivshmem - though
I didn't test this, I verified that virtio keeps
working like it did.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c       | 13 +++++++++++--
 hw/virtio-pci.c |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

Comments

Jan Kiszka Aug. 29, 2012, 4:44 p.m.
On 2012-08-29 18:40, Michael S. Tsirkin wrote:
> The facility to use/unuse vectors dynamically is helpful
> for virtio but little else: everyone just seems to use
> vectors in their init function.
> 
> Avoid clearing msix vector use info on reset and load.
> For virtio, clear it explicitly.
> This should fix regressions reported with ivshmem - though
> I didn't test this, I verified that virtio keeps
> working like it did.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/msix.c       | 13 +++++++++++--
>  hw/virtio-pci.c |  2 ++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 800fc32..d040cc2 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -340,6 +340,15 @@ static void msix_free_irq_entries(PCIDevice *dev)
>      }
>  }
>  
> +static void msix_clear_all_vectors(PCIDevice *dev)
> +{
> +    int vector;
> +
> +    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
> +        msix_clr_pending(dev, vector);
> +    }
> +}
> +
>  /* Clean up resources for the device. */
>  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>  {
> @@ -394,7 +403,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>          return;
>      }
>  
> -    msix_free_irq_entries(dev);
> +    msix_clear_all_vectors(dev);
>      qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
>      qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
>      msix_update_function_masked(dev);
> @@ -440,7 +449,7 @@ void msix_reset(PCIDevice *dev)
>      if (!msix_present(dev)) {
>          return;
>      }
> -    msix_free_irq_entries(dev);
> +    msix_clear_all_vectors(dev);
>      dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
>  	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
>      memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 125eded..ca0b204 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -131,6 +131,7 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>      if (ret) {
>          return ret;
>      }
> +    msix_unuse_all_vectors(&proxy->pci_dev);
>      msix_load(&proxy->pci_dev, f);
>      if (msix_present(&proxy->pci_dev)) {
>          qemu_get_be16s(f, &proxy->vdev->config_vector);
> @@ -246,6 +247,7 @@ void virtio_pci_reset(DeviceState *d)
>      VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>      virtio_pci_stop_ioeventfd(proxy);
>      virtio_reset(proxy->vdev);
> +    msix_unuse_all_vectors(&proxy->pci_dev);
>      proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>  }
>  
> 

Fine with me, but let's ask Cam to test.

Jan
Michael S. Tsirkin Aug. 29, 2012, 4:47 p.m.
On Wed, Aug 29, 2012 at 06:44:49PM +0200, Jan Kiszka wrote:
> On 2012-08-29 18:40, Michael S. Tsirkin wrote:
> > The facility to use/unuse vectors dynamically is helpful
> > for virtio but little else: everyone just seems to use
> > vectors in their init function.
> > 
> > Avoid clearing msix vector use info on reset and load.
> > For virtio, clear it explicitly.
> > This should fix regressions reported with ivshmem - though
> > I didn't test this, I verified that virtio keeps
> > working like it did.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/msix.c       | 13 +++++++++++--
> >  hw/virtio-pci.c |  2 ++
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 800fc32..d040cc2 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -340,6 +340,15 @@ static void msix_free_irq_entries(PCIDevice *dev)
> >      }
> >  }
> >  
> > +static void msix_clear_all_vectors(PCIDevice *dev)
> > +{
> > +    int vector;
> > +
> > +    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
> > +        msix_clr_pending(dev, vector);
> > +    }
> > +}
> > +
> >  /* Clean up resources for the device. */
> >  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> >  {
> > @@ -394,7 +403,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
> >          return;
> >      }
> >  
> > -    msix_free_irq_entries(dev);
> > +    msix_clear_all_vectors(dev);
> >      qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
> >      qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
> >      msix_update_function_masked(dev);
> > @@ -440,7 +449,7 @@ void msix_reset(PCIDevice *dev)
> >      if (!msix_present(dev)) {
> >          return;
> >      }
> > -    msix_free_irq_entries(dev);
> > +    msix_clear_all_vectors(dev);
> >      dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> >  	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> >      memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index 125eded..ca0b204 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -131,6 +131,7 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
> >      if (ret) {
> >          return ret;
> >      }
> > +    msix_unuse_all_vectors(&proxy->pci_dev);
> >      msix_load(&proxy->pci_dev, f);
> >      if (msix_present(&proxy->pci_dev)) {
> >          qemu_get_be16s(f, &proxy->vdev->config_vector);
> > @@ -246,6 +247,7 @@ void virtio_pci_reset(DeviceState *d)
> >      VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
> >      virtio_pci_stop_ioeventfd(proxy);
> >      virtio_reset(proxy->vdev);
> > +    msix_unuse_all_vectors(&proxy->pci_dev);
> >      proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
> >  }
> >  
> > 
> 
> Fine with me, but let's ask Cam to test.
> 
> Jan

Cam deadline is today - can u test quickly pls?

> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
Andreas Färber Aug. 29, 2012, 4:54 p.m.
$subject: "[un]used vectors"? -- could be fixed by committer.

/-F

Am 29.08.2012 18:40, schrieb Michael S. Tsirkin:
> The facility to use/unuse vectors dynamically is helpful
> for virtio but little else: everyone just seems to use
> vectors in their init function.
> 
> Avoid clearing msix vector use info on reset and load.
> For virtio, clear it explicitly.
> This should fix regressions reported with ivshmem - though
> I didn't test this, I verified that virtio keeps
> working like it did.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cam Macdonell Aug. 29, 2012, 4:59 p.m.
Yes, will test shortly.

Cam

On Wed, Aug 29, 2012 at 10:47 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Aug 29, 2012 at 06:44:49PM +0200, Jan Kiszka wrote:
>> On 2012-08-29 18:40, Michael S. Tsirkin wrote:
>> > The facility to use/unuse vectors dynamically is helpful
>> > for virtio but little else: everyone just seems to use
>> > vectors in their init function.
>> >
>> > Avoid clearing msix vector use info on reset and load.
>> > For virtio, clear it explicitly.
>> > This should fix regressions reported with ivshmem - though
>> > I didn't test this, I verified that virtio keeps
>> > working like it did.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> >  hw/msix.c       | 13 +++++++++++--
>> >  hw/virtio-pci.c |  2 ++
>> >  2 files changed, 13 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/msix.c b/hw/msix.c
>> > index 800fc32..d040cc2 100644
>> > --- a/hw/msix.c
>> > +++ b/hw/msix.c
>> > @@ -340,6 +340,15 @@ static void msix_free_irq_entries(PCIDevice *dev)
>> >      }
>> >  }
>> >
>> > +static void msix_clear_all_vectors(PCIDevice *dev)
>> > +{
>> > +    int vector;
>> > +
>> > +    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
>> > +        msix_clr_pending(dev, vector);
>> > +    }
>> > +}
>> > +
>> >  /* Clean up resources for the device. */
>> >  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>> >  {
>> > @@ -394,7 +403,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>> >          return;
>> >      }
>> >
>> > -    msix_free_irq_entries(dev);
>> > +    msix_clear_all_vectors(dev);
>> >      qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
>> >      qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
>> >      msix_update_function_masked(dev);
>> > @@ -440,7 +449,7 @@ void msix_reset(PCIDevice *dev)
>> >      if (!msix_present(dev)) {
>> >          return;
>> >      }
>> > -    msix_free_irq_entries(dev);
>> > +    msix_clear_all_vectors(dev);
>> >      dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
>> >         ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
>> >      memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
>> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> > index 125eded..ca0b204 100644
>> > --- a/hw/virtio-pci.c
>> > +++ b/hw/virtio-pci.c
>> > @@ -131,6 +131,7 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>> >      if (ret) {
>> >          return ret;
>> >      }
>> > +    msix_unuse_all_vectors(&proxy->pci_dev);
>> >      msix_load(&proxy->pci_dev, f);
>> >      if (msix_present(&proxy->pci_dev)) {
>> >          qemu_get_be16s(f, &proxy->vdev->config_vector);
>> > @@ -246,6 +247,7 @@ void virtio_pci_reset(DeviceState *d)
>> >      VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>> >      virtio_pci_stop_ioeventfd(proxy);
>> >      virtio_reset(proxy->vdev);
>> > +    msix_unuse_all_vectors(&proxy->pci_dev);
>> >      proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>> >  }
>> >
>> >
>>
>> Fine with me, but let's ask Cam to test.
>>
>> Jan
>
> Cam deadline is today - can u test quickly pls?
>
>> --
>> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
>> Corporate Competence Center Embedded Linux
>
Cam Macdonell Aug. 29, 2012, 5:17 p.m.
Yes, works for me.

On Wed, Aug 29, 2012 at 10:40 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> The facility to use/unuse vectors dynamically is helpful
> for virtio but little else: everyone just seems to use
> vectors in their init function.
>
> Avoid clearing msix vector use info on reset and load.
> For virtio, clear it explicitly.
> This should fix regressions reported with ivshmem - though
> I didn't test this, I verified that virtio keeps
> working like it did.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Cam Macdonell <cam@cs.ualberta.ca>
> ---
>  hw/msix.c       | 13 +++++++++++--
>  hw/virtio-pci.c |  2 ++
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 800fc32..d040cc2 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -340,6 +340,15 @@ static void msix_free_irq_entries(PCIDevice *dev)
>      }
>  }
>
> +static void msix_clear_all_vectors(PCIDevice *dev)
> +{
> +    int vector;
> +
> +    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
> +        msix_clr_pending(dev, vector);
> +    }
> +}
> +
>  /* Clean up resources for the device. */
>  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>  {
> @@ -394,7 +403,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>          return;
>      }
>
> -    msix_free_irq_entries(dev);
> +    msix_clear_all_vectors(dev);
>      qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
>      qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
>      msix_update_function_masked(dev);
> @@ -440,7 +449,7 @@ void msix_reset(PCIDevice *dev)
>      if (!msix_present(dev)) {
>          return;
>      }
> -    msix_free_irq_entries(dev);
> +    msix_clear_all_vectors(dev);
>      dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
>             ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
>      memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 125eded..ca0b204 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -131,6 +131,7 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>      if (ret) {
>          return ret;
>      }
> +    msix_unuse_all_vectors(&proxy->pci_dev);
>      msix_load(&proxy->pci_dev, f);
>      if (msix_present(&proxy->pci_dev)) {
>          qemu_get_be16s(f, &proxy->vdev->config_vector);
> @@ -246,6 +247,7 @@ void virtio_pci_reset(DeviceState *d)
>      VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>      virtio_pci_stop_ioeventfd(proxy);
>      virtio_reset(proxy->vdev);
> +    msix_unuse_all_vectors(&proxy->pci_dev);
>      proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>  }
>
> --
> MST
>
Michael S. Tsirkin Aug. 29, 2012, 6:13 p.m.
On Wed, Aug 29, 2012 at 06:54:35PM +0200, Andreas Färber wrote:
> $subject: "[un]used vectors"? -- could be fixed by committer.
> 
> /-F

Sorry I don't unedrstand. it's not 'unused': it's use and unuse.
What is wrong with the subject?


> Am 29.08.2012 18:40, schrieb Michael S. Tsirkin:
> > The facility to use/unuse vectors dynamically is helpful
> > for virtio but little else: everyone just seems to use
> > vectors in their init function.
> > 
> > Avoid clearing msix vector use info on reset and load.
> > For virtio, clear it explicitly.
> > This should fix regressions reported with ivshmem - though
> > I didn't test this, I verified that virtio keeps
> > working like it did.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber Aug. 30, 2012, 1:34 p.m.
Am 29.08.2012 20:13, schrieb Michael S. Tsirkin:
> On Wed, Aug 29, 2012 at 06:54:35PM +0200, Andreas Färber wrote:
>> $subject: "[un]used vectors"? -- could be fixed by committer.
> 
> Sorry I don't unedrstand. it's not 'unused': it's use and unuse.
> What is wrong with the subject?

The grammar with two verbs "make [un]use" sounded wrong to my ears.
Given your explanation of [un]use above, did you mean "make clearing
[un]use vectors optional on reset/load" or something like that?

/-F
Michael S. Tsirkin Aug. 30, 2012, 2:39 p.m.
On Thu, Aug 30, 2012 at 03:34:42PM +0200, Andreas Färber wrote:
> Am 29.08.2012 20:13, schrieb Michael S. Tsirkin:
> > On Wed, Aug 29, 2012 at 06:54:35PM +0200, Andreas Färber wrote:
> >> $subject: "[un]used vectors"? -- could be fixed by committer.
> > 
> > Sorry I don't unedrstand. it's not 'unused': it's use and unuse.
> > What is wrong with the subject?
> 
> The grammar with two verbs "make [un]use" sounded wrong to my ears.
> Given your explanation of [un]use above, did you mean "make clearing
> [un]use vectors optional on reset/load" or something like that?
> 
> /-F

No, sorry. What is meant is simply functions
msix_vector_use/msix_vector_unuse:
calling these on reset/load was required but
is now optional.


> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch hide | download patch | download mbox

diff --git a/hw/msix.c b/hw/msix.c
index 800fc32..d040cc2 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -340,6 +340,15 @@  static void msix_free_irq_entries(PCIDevice *dev)
     }
 }
 
+static void msix_clear_all_vectors(PCIDevice *dev)
+{
+    int vector;
+
+    for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
+        msix_clr_pending(dev, vector);
+    }
+}
+
 /* Clean up resources for the device. */
 void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
 {
@@ -394,7 +403,7 @@  void msix_load(PCIDevice *dev, QEMUFile *f)
         return;
     }
 
-    msix_free_irq_entries(dev);
+    msix_clear_all_vectors(dev);
     qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
     qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
     msix_update_function_masked(dev);
@@ -440,7 +449,7 @@  void msix_reset(PCIDevice *dev)
     if (!msix_present(dev)) {
         return;
     }
-    msix_free_irq_entries(dev);
+    msix_clear_all_vectors(dev);
     dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
 	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
     memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 125eded..ca0b204 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -131,6 +131,7 @@  static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     if (ret) {
         return ret;
     }
+    msix_unuse_all_vectors(&proxy->pci_dev);
     msix_load(&proxy->pci_dev, f);
     if (msix_present(&proxy->pci_dev)) {
         qemu_get_be16s(f, &proxy->vdev->config_vector);
@@ -246,6 +247,7 @@  void virtio_pci_reset(DeviceState *d)
     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     virtio_pci_stop_ioeventfd(proxy);
     virtio_reset(proxy->vdev);
+    msix_unuse_all_vectors(&proxy->pci_dev);
     proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
 }