[Qemu-devel] [PATCH for v1.0 0/3] msix: fixes for 1.0

[Qemu-devel] [PATCH for v1.0 0/3] msix: fixes for 1.0

[Michael S. Tsirkin]

This fixes bugs dealing with msi-x mask bits pointed out by Jan Kiszka.

Jan Kiszka (1):
  msix: Prevent bogus mask updates on MMIO accesses

Michael S. Tsirkin (2):
  msix: track function masked in pci device state
  msix: avoid mask updates if mask is unchanged

 hw/msix.c |   48 ++++++++++++++++++++++++++++++++++++------------
 hw/pci.h  |    2 ++
 2 files changed, 38 insertions(+), 12 deletions(-)

-- 
1.7.5.53.gc233e

[Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Michael S. Tsirkin]

Only go over the table when function is masked.
This is not really important for qemu.git but helps
fix a bug in qemu-kvm.git.

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

diff --git a/hw/msix.c b/hw/msix.c
index b15bafc..63b41b9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
     /* Make flags bit writable. */
     pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
 	    MSIX_MASKALL_MASK;
+    pdev->msix_function_masked = true;
     return 0;
 }
 
@@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
 }
 
-static int msix_function_masked(PCIDevice *dev)
-{
-    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
-}
-
 static int msix_is_masked(PCIDevice *dev, int vector)
 {
     unsigned offset =
         vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-    return msix_function_masked(dev) ||
+    return dev->msix_function_masked ||
 	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
 }
 
@@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
     }
 }
 
+static void msix_update_function_masked(PCIDevice *dev)
+{
+    dev->msix_function_masked = !msix_enabled(dev) ||
+        (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK);
+}
+
 /* Handle MSI-X capability config write. */
 void msix_write_config(PCIDevice *dev, uint32_t addr,
                        uint32_t val, int len)
 {
     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     int vector;
+    bool was_masked;
 
     if (!range_covers_byte(addr, len, enable_pos)) {
         return;
     }
 
+    was_masked = dev->msix_function_masked;
+    msix_update_function_masked(dev);
+
     if (!msix_enabled(dev)) {
         return;
     }
 
     pci_device_deassert_intx(dev);
 
-    if (msix_function_masked(dev)) {
+    if (dev->msix_function_masked == was_masked) {
         return;
     }
 
@@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
     msix_free_irq_entries(dev);
     qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
     qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
+    msix_update_function_masked(dev);
 }
 
 /* Does device support MSI-X? */
diff --git a/hw/pci.h b/hw/pci.h
index 4b2e785..625e717 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -178,6 +178,8 @@ struct PCIDevice {
     unsigned *msix_entry_used;
     /* Region including the MSI-X table */
     uint32_t msix_bar_size;
+    /* MSIX function mask set or MSIX disabled */
+    bool msix_function_masked;
     /* Version id needed for VMState */
     int32_t version_id;
 
-- 
1.7.5.53.gc233e

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Cam Macdonell]

Based on a git bisect, this patch breaks msi-x interrupt delivery in
the ivshmem device.

On Mon, Nov 21, 2011 at 9:57 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Only go over the table when function is masked.
> This is not really important for qemu.git but helps
> fix a bug in qemu-kvm.git.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/msix.c |   21 ++++++++++++++-------
>  hw/pci.h  |    2 ++
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index b15bafc..63b41b9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>     /* Make flags bit writable. */
>     pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>            MSIX_MASKALL_MASK;
> +    pdev->msix_function_masked = true;
>     return 0;

iiuc, this masks the msix by default.

>  }
>
> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
>  }
>
> -static int msix_function_masked(PCIDevice *dev)
> -{
> -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> -}
> -
>  static int msix_is_masked(PCIDevice *dev, int vector)
>  {
>     unsigned offset =
>         vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> -    return msix_function_masked(dev) ||
> +    return dev->msix_function_masked ||
>           dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  }
>
> @@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
>     }
>  }
>
> +static void msix_update_function_masked(PCIDevice *dev)
> +{
> +    dev->msix_function_masked = !msix_enabled(dev) ||
> +        (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK);
> +}
> +
>  /* Handle MSI-X capability config write. */
>  void msix_write_config(PCIDevice *dev, uint32_t addr,
>                        uint32_t val, int len)
>  {
>     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
>     int vector;
> +    bool was_masked;
>
>     if (!range_covers_byte(addr, len, enable_pos)) {
>         return;
>     }
>
> +    was_masked = dev->msix_function_masked;
> +    msix_update_function_masked(dev);
> +
>     if (!msix_enabled(dev)) {
>         return;
>     }
>
>     pci_device_deassert_intx(dev);
>
> -    if (msix_function_masked(dev)) {
> +    if (dev->msix_function_masked == was_masked) {
>         return;
>     }

So I believe my bug is due to the fact the new logic included in this
patch requires msix_write_config() to be called to unmask the vectors.
 Virtio-pci calls msix_write_config(), but ivshmem does not (nor does
PCIe so I'm not sure if it's also affected).

I haven't been able to fix the bug yet, but I wanted to make sure I
was looking in the correct place.  Any help of further explanation of
this patch would be greatly appreciated.

Sincerely,
Cam

>
> @@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>     msix_free_irq_entries(dev);
>     qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
>     qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> +    msix_update_function_masked(dev);
>  }
>
>  /* Does device support MSI-X? */
> diff --git a/hw/pci.h b/hw/pci.h
> index 4b2e785..625e717 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -178,6 +178,8 @@ struct PCIDevice {
>     unsigned *msix_entry_used;
>     /* Region including the MSI-X table */
>     uint32_t msix_bar_size;
> +    /* MSIX function mask set or MSIX disabled */
> +    bool msix_function_masked;
>     /* Version id needed for VMState */
>     int32_t version_id;
>
> --
> 1.7.5.53.gc233e
>
>

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

On 2011-12-03 00:34, Cam Macdonell wrote:
> So I believe my bug is due to the fact the new logic included in this
> patch requires msix_write_config() to be called to unmask the vectors.
>  Virtio-pci calls msix_write_config(), but ivshmem does not (nor does
> PCIe so I'm not sure if it's also affected).

msi[x]_write_config should not have to be called from device models but
it must be right now. I proposed a patch a while ago to improve this.
Same for msix_reset.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Michael S. Tsirkin]

On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> the ivshmem device.
> 
> On Mon, Nov 21, 2011 at 9:57 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Only go over the table when function is masked.
> > This is not really important for qemu.git but helps
> > fix a bug in qemu-kvm.git.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/msix.c |   21 ++++++++++++++-------
> >  hw/pci.h  |    2 ++
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index b15bafc..63b41b9 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> >     /* Make flags bit writable. */
> >     pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> >            MSIX_MASKALL_MASK;
> > +    pdev->msix_function_masked = true;
> >     return 0;
> 
> iiuc, this masks the msix by default.

Yes, because msi-x is disabled by default, that's
in the pci spec.

> >  }
> >
> > @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
> >  }
> >
> > -static int msix_function_masked(PCIDevice *dev)
> > -{
> > -    return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> > -}
> > -
> >  static int msix_is_masked(PCIDevice *dev, int vector)
> >  {
> >     unsigned offset =
> >         vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> > -    return msix_function_masked(dev) ||
> > +    return dev->msix_function_masked ||
> >           dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >  }
> >
> > @@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
> >     }
> >  }
> >
> > +static void msix_update_function_masked(PCIDevice *dev)
> > +{
> > +    dev->msix_function_masked = !msix_enabled(dev) ||
> > +        (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK);
> > +}
> > +
> >  /* Handle MSI-X capability config write. */
> >  void msix_write_config(PCIDevice *dev, uint32_t addr,
> >                        uint32_t val, int len)
> >  {
> >     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> >     int vector;
> > +    bool was_masked;
> >
> >     if (!range_covers_byte(addr, len, enable_pos)) {
> >         return;
> >     }
> >
> > +    was_masked = dev->msix_function_masked;
> > +    msix_update_function_masked(dev);
> > +
> >     if (!msix_enabled(dev)) {
> >         return;
> >     }
> >
> >     pci_device_deassert_intx(dev);
> >
> > -    if (msix_function_masked(dev)) {
> > +    if (dev->msix_function_masked == was_masked) {
> >         return;
> >     }
> 
> So I believe my bug is due to the fact the new logic included in this
> patch requires msix_write_config() to be called to unmask the vectors.

Not exactly, to enable msi-x really.

>  Virtio-pci calls msix_write_config(), but ivshmem does not (nor does
> PCIe so I'm not sure if it's also affected).

At this point PCIe is a stub.

> I haven't been able to fix the bug yet, but I wanted to make sure I
> was looking in the correct place.  Any help of further explanation of
> this patch would be greatly appreciated.
> 
> Sincerely,
> Cam

So I think you just need to call msix_write_config,
otherwise msix is not getting enabled.

BTW looking at the ivshmem code, this bit looks wrong:

    pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY;

I think the spec says IO/MEMORY must be disabled at init time since BARs
are not yet set to anything reasonable.

> >
> > @@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
> >     msix_free_irq_entries(dev);
> >     qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> >     qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> > +    msix_update_function_masked(dev);
> >  }
> >
> >  /* Does device support MSI-X? */
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 4b2e785..625e717 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -178,6 +178,8 @@ struct PCIDevice {
> >     unsigned *msix_entry_used;
> >     /* Region including the MSI-X table */
> >     uint32_t msix_bar_size;
> > +    /* MSIX function mask set or MSIX disabled */
> > +    bool msix_function_masked;
> >     /* Version id needed for VMState */
> >     int32_t version_id;
> >
> > --
> > 1.7.5.53.gc233e
> >
> >

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Michael S. Tsirkin]

On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> the ivshmem device.

I think the following should fix it. Compiled-only -
could you pls check? If yes let's apply to the stable branch.

-->

ivshmem: add missing msix calls

ivshmem used msix but didn't call it on either reset or
config write paths. This used to partically work since
guests don't use all of msi-x configuration fields,
and reset is rarely used, but the patch 'msix: track function masked
in pci device state' broke that. Fix by adding appropriate calls.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..3680c0f 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
 
     s->intrstatus = 0;
+    msix_reset(&s->dev);
     return;
 }
 
@@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }
 
+static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
+				 uint32_t val, int len)
+{
+    pci_default_write_config(pci_dev, address, val, len);
+    msix_write_config(pci_dev, address, val, len);
+}
+
 static int pci_ivshmem_init(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
@@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
     }
 
+    s->dev.config_write = ivshmem_write_config;
+
     return 0;
 }
 

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On 2011-12-04 11:20, Michael S. Tsirkin wrote:
> On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
>> Based on a git bisect, this patch breaks msi-x interrupt delivery in
>> the ivshmem device.
> 
> I think the following should fix it. Compiled-only -
> could you pls check? If yes let's apply to the stable branch.
> 
> -->
> 
> ivshmem: add missing msix calls
> 
> ivshmem used msix but didn't call it on either reset or
> config write paths. This used to partically work since
> guests don't use all of msi-x configuration fields,
> and reset is rarely used, but the patch 'msix: track function masked
> in pci device state' broke that. Fix by adding appropriate calls.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> --
> 
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..3680c0f 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
>      IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>  
>      s->intrstatus = 0;
> +    msix_reset(&s->dev);
>      return;
>  }
>  
> @@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> +				 uint32_t val, int len)
> +{
> +    pci_default_write_config(pci_dev, address, val, len);
> +    msix_write_config(pci_dev, address, val, len);
> +}
> +
>  static int pci_ivshmem_init(PCIDevice *dev)
>  {
>      IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>  
>      }
>  
> +    s->dev.config_write = ivshmem_write_config;
> +
>      return 0;
>  }
>  
> 
> 

But please fix this for real and merge [1]&[2] (with depending patches)
into master. The above is just boilerplate code from device POV.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80240
[2] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80244


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Michael S. Tsirkin]

On Sun, Dec 04, 2011 at 01:35:03PM +0100, Jan Kiszka wrote:
> On 2011-12-04 11:20, Michael S. Tsirkin wrote:
> > On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> >> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> >> the ivshmem device.
> > 
> > I think the following should fix it. Compiled-only -
> > could you pls check? If yes let's apply to the stable branch.
> > 
> > -->
> > 
> > ivshmem: add missing msix calls
> > 
> > ivshmem used msix but didn't call it on either reset or
> > config write paths. This used to partically work since
> > guests don't use all of msi-x configuration fields,
> > and reset is rarely used, but the patch 'msix: track function masked
> > in pci device state' broke that. Fix by adding appropriate calls.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > --
> > 
> > diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> > index 242fbea..3680c0f 100644
> > --- a/hw/ivshmem.c
> > +++ b/hw/ivshmem.c
> > @@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
> >      IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
> >  
> >      s->intrstatus = 0;
> > +    msix_reset(&s->dev);
> >      return;
> >  }
> >  
> > @@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
> >      return 0;
> >  }
> >  
> > +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> > +				 uint32_t val, int len)
> > +{
> > +    pci_default_write_config(pci_dev, address, val, len);
> > +    msix_write_config(pci_dev, address, val, len);
> > +}
> > +
> >  static int pci_ivshmem_init(PCIDevice *dev)
> >  {
> >      IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> > @@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
> >  
> >      }
> >  
> > +    s->dev.config_write = ivshmem_write_config;
> > +
> >      return 0;
> >  }
> >  
> > 
> > 
> 
> But please fix this for real and merge [1]&[2] (with depending patches)
> into master. The above is just boilerplate code from device POV.
> 
> Jan
> 
> [1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80240
> [2] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80244
> 

Yes, I agree we should make it easier for devices.
What annoyed me was the need to put msix in save/load.

And that is because of the need to do this in a specific
order.  I hope to switch to an unordered format and
then this will become straight-forward.

-- 
MST

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Cam Macdonell]

On Sun, Dec 4, 2011 at 3:20 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
>> Based on a git bisect, this patch breaks msi-x interrupt delivery in
>> the ivshmem device.
>
> I think the following should fix it. Compiled-only -
> could you pls check? If yes let's apply to the stable branch.

Thanks for the patch Michael.

It addresses the need for msix_write_config() to be called, but the
addition of the msix_reset() is causing a reset of the vectors after
they've been initialized in pci_ivshmem_init().  So, interrupts still
aren't delivered with this patch applied as it is.

In particular, a reset occurs after pci_ivshmem_init runs, so the
msix_entry_used array is reset to 0s, which causes the interrupt
delivery to fail.

If I comment out the msix_reset(), then interrupts are delivered.
Would the reset be caused by a bug in the guest driver?  or do I need
to reconfigure the msix after reset?  I'm unclear as to the proper
behaviour after a reset.

Thanks,
Cam

>
> -->
>
> ivshmem: add missing msix calls
>
> ivshmem used msix but didn't call it on either reset or
> config write paths. This used to partically work since
> guests don't use all of msi-x configuration fields,
> and reset is rarely used, but the patch 'msix: track function masked
> in pci device state' broke that. Fix by adding appropriate calls.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> --
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..3680c0f 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
>     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>
>     s->intrstatus = 0;
> +    msix_reset(&s->dev);
>     return;
>  }
>
> @@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
>     return 0;
>  }
>
> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> +                                uint32_t val, int len)
> +{
> +    pci_default_write_config(pci_dev, address, val, len);
> +    msix_write_config(pci_dev, address, val, len);
> +}
> +
>  static int pci_ivshmem_init(PCIDevice *dev)
>  {
>     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
>     }
>
> +    s->dev.config_write = ivshmem_write_config;
> +
>     return 0;
>  }
>
>

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Michael S. Tsirkin]

On Sun, Dec 04, 2011 at 04:47:17PM -0700, Cam Macdonell wrote:
> On Sun, Dec 4, 2011 at 3:20 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> >> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> >> the ivshmem device.
> >
> > I think the following should fix it. Compiled-only -
> > could you pls check? If yes let's apply to the stable branch.
> 
> Thanks for the patch Michael.
> 
> It addresses the need for msix_write_config() to be called, but the
> addition of the msix_reset() is causing a reset of the vectors after
> they've been initialized in pci_ivshmem_init().  So, interrupts still
> aren't delivered with this patch applied as it is.
> 
> In particular, a reset occurs after pci_ivshmem_init runs, so the
> msix_entry_used array is reset to 0s, which causes the interrupt
> delivery to fail.
> 
> If I comment out the msix_reset(), then interrupts are delivered.
> Would the reset be caused by a bug in the guest driver?  or do I need
> to reconfigure the msix after reset?  I'm unclear as to the proper
> behaviour after a reset.
> 
> Thanks,
> Cam
> 

I didn't anticipate this, virtio only configures vectors when msi-x is enabled.
For 1.0 it's safest to do the same and just re-enable after reset.

So we'll end up with something like the following - does it help?

Side note: exit(1) is not the best way to handle
errors in the init function. I think you should add error_report, then
goto err on failures to init, cleanup what you have set up
meanwhile and return an error code.

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..c58f4d3 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -500,11 +500,29 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
     return;
 }
 
+/* Select the MSI-X vectors used by device.
+ * ivshmem maps events to vectors statically, so
+ * we just enable all vectors on init and after reset. */
+static void ivshmem_use_msix(IVShmemState * s)
+{
+    int i;
+
+    if (!msix_present(&s->dev)) {
+        return;
+    }
+
+    for (i = 0; i < s->vectors; i++) {
+        msix_vector_use(&s->dev, i);
+    }
+}
+
 static void ivshmem_reset(DeviceState *d)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
 
     s->intrstatus = 0;
+    msix_reset(&s->dev);
+    ivshmem_use_msix(s);
     return;
 }
 
@@ -535,12 +553,8 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
     return value;
 }
 
-static void ivshmem_setup_msi(IVShmemState * s) {
-
-    int i;
-
-    /* allocate the MSI-X vectors */
-
+static void ivshmem_setup_msi(IVShmemState * s)
+{
     memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
     if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
         pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
@@ -551,13 +565,10 @@ static void ivshmem_setup_msi(IVShmemState * s) {
         exit(1);
     }
 
-    /* 'activate' the vectors */
-    for (i = 0; i < s->vectors; i++) {
-        msix_vector_use(&s->dev, i);
-    }
-
     /* allocate Qemu char devices for receiving interrupts */
     s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
+
+    ivshmem_use_msix(s);
 }
 
 static void ivshmem_save(QEMUFile* f, void *opaque)
@@ -610,6 +621,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }
 
+static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
+				 uint32_t val, int len)
+{
+    pci_default_write_config(pci_dev, address, val, len);
+    msix_write_config(pci_dev, address, val, len);
+}
+
 static int pci_ivshmem_init(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
@@ -734,6 +752,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
     }
 
+    s->dev.config_write = ivshmem_write_config;
+
     return 0;
 }
 

Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state

[Cam Macdonell]

On Mon, Dec 5, 2011 at 2:08 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Dec 04, 2011 at 04:47:17PM -0700, Cam Macdonell wrote:
>> On Sun, Dec 4, 2011 at 3:20 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
>> >> Based on a git bisect, this patch breaks msi-x interrupt delivery in
>> >> the ivshmem device.
>> >
>> > I think the following should fix it. Compiled-only -
>> > could you pls check? If yes let's apply to the stable branch.
>>
>> Thanks for the patch Michael.
>>
>> It addresses the need for msix_write_config() to be called, but the
>> addition of the msix_reset() is causing a reset of the vectors after
>> they've been initialized in pci_ivshmem_init().  So, interrupts still
>> aren't delivered with this patch applied as it is.
>>
>> In particular, a reset occurs after pci_ivshmem_init runs, so the
>> msix_entry_used array is reset to 0s, which causes the interrupt
>> delivery to fail.
>>
>> If I comment out the msix_reset(), then interrupts are delivered.
>> Would the reset be caused by a bug in the guest driver?  or do I need
>> to reconfigure the msix after reset?  I'm unclear as to the proper
>> behaviour after a reset.
>>
>> Thanks,
>> Cam
>>
>
> I didn't anticipate this, virtio only configures vectors when msi-x is enabled.
> For 1.0 it's safest to do the same and just re-enable after reset.
>
> So we'll end up with something like the following - does it help?

Yes, this fixes interrupt delivery.

>
> Side note: exit(1) is not the best way to handle
> errors in the init function. I think you should add error_report, then
> goto err on failures to init, cleanup what you have set up
> meanwhile and return an error code.

Thanks, I'll fix that.

>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..c58f4d3 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -500,11 +500,29 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>     return;
>  }
>
> +/* Select the MSI-X vectors used by device.
> + * ivshmem maps events to vectors statically, so
> + * we just enable all vectors on init and after reset. */
> +static void ivshmem_use_msix(IVShmemState * s)
> +{
> +    int i;
> +
> +    if (!msix_present(&s->dev)) {
> +        return;
> +    }
> +
> +    for (i = 0; i < s->vectors; i++) {
> +        msix_vector_use(&s->dev, i);
> +    }
> +}
> +
>  static void ivshmem_reset(DeviceState *d)
>  {
>     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>
>     s->intrstatus = 0;
> +    msix_reset(&s->dev);
> +    ivshmem_use_msix(s);
>     return;
>  }
>
> @@ -535,12 +553,8 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
>     return value;
>  }
>
> -static void ivshmem_setup_msi(IVShmemState * s) {
> -
> -    int i;
> -
> -    /* allocate the MSI-X vectors */
> -
> +static void ivshmem_setup_msi(IVShmemState * s)
> +{
>     memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
>     if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
>         pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> @@ -551,13 +565,10 @@ static void ivshmem_setup_msi(IVShmemState * s) {
>         exit(1);
>     }
>
> -    /* 'activate' the vectors */
> -    for (i = 0; i < s->vectors; i++) {
> -        msix_vector_use(&s->dev, i);
> -    }
> -
>     /* allocate Qemu char devices for receiving interrupts */
>     s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
> +
> +    ivshmem_use_msix(s);
>  }
>
>  static void ivshmem_save(QEMUFile* f, void *opaque)
> @@ -610,6 +621,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
>     return 0;
>  }
>
> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> +                                uint32_t val, int len)
> +{
> +    pci_default_write_config(pci_dev, address, val, len);
> +    msix_write_config(pci_dev, address, val, len);
> +}
> +
>  static int pci_ivshmem_init(PCIDevice *dev)
>  {
>     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -734,6 +752,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
>     }
>
> +    s->dev.config_write = ivshmem_write_config;
> +
>     return 0;
>  }
>
>

[Qemu-devel] [PATCH master/v1.0.x] ivshmem: add missing msix calls

[Michael S. Tsirkin]

ivshmem used msix but didn't call it on either reset or
config write paths. This used to partically work since
guests don't use all of msi-x configuration fields,
and reset is rarely used, but the patch 'msix: track function masked
in pci device state' broke that. Fix by adding appropriate calls.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reported-by: Cam Macdonell <cam@cs.ualberta.ca>
Tested-by: Cam Macdonell <cam@cs.ualberta.ca>

---

Please apply the following to both master
and 1.0 stable branch. Thanks!

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..c58f4d3 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -500,11 +500,29 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
     return;
 }
 
+/* Select the MSI-X vectors used by device.
+ * ivshmem maps events to vectors statically, so
+ * we just enable all vectors on init and after reset. */
+static void ivshmem_use_msix(IVShmemState * s)
+{
+    int i;
+
+    if (!msix_present(&s->dev)) {
+        return;
+    }
+
+    for (i = 0; i < s->vectors; i++) {
+        msix_vector_use(&s->dev, i);
+    }
+}
+
 static void ivshmem_reset(DeviceState *d)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
 
     s->intrstatus = 0;
+    msix_reset(&s->dev);
+    ivshmem_use_msix(s);
     return;
 }
 
@@ -535,12 +553,8 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
     return value;
 }
 
-static void ivshmem_setup_msi(IVShmemState * s) {
-
-    int i;
-
-    /* allocate the MSI-X vectors */
-
+static void ivshmem_setup_msi(IVShmemState * s)
+{
     memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
     if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
         pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
@@ -551,13 +565,10 @@ static void ivshmem_setup_msi(IVShmemState * s) {
         exit(1);
     }
 
-    /* 'activate' the vectors */
-    for (i = 0; i < s->vectors; i++) {
-        msix_vector_use(&s->dev, i);
-    }
-
     /* allocate Qemu char devices for receiving interrupts */
     s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
+
+    ivshmem_use_msix(s);
 }
 
 static void ivshmem_save(QEMUFile* f, void *opaque)
@@ -610,6 +621,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }
 
+static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
+				 uint32_t val, int len)
+{
+    pci_default_write_config(pci_dev, address, val, len);
+    msix_write_config(pci_dev, address, val, len);
+}
+
 static int pci_ivshmem_init(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
@@ -734,6 +752,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
     }
 
+    s->dev.config_write = ivshmem_write_config;
+
     return 0;
 }
 

Re: [Qemu-devel] [PATCH master/v1.0.x] ivshmem: add missing msix calls

[Cam Macdonell]

Hello,

Can this patch be merged, please?

Thanks,
Cam

On Mon, Dec 5, 2011 at 12:48 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> ivshmem used msix but didn't call it on either reset or
> config write paths. This used to partically work since
> guests don't use all of msi-x configuration fields,
> and reset is rarely used, but the patch 'msix: track function masked
> in pci device state' broke that. Fix by adding appropriate calls.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reported-by: Cam Macdonell <cam@cs.ualberta.ca>
> Tested-by: Cam Macdonell <cam@cs.ualberta.ca>
>
> ---
>
> Please apply the following to both master
> and 1.0 stable branch. Thanks!
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..c58f4d3 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -500,11 +500,29 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>     return;
>  }
>
> +/* Select the MSI-X vectors used by device.
> + * ivshmem maps events to vectors statically, so
> + * we just enable all vectors on init and after reset. */
> +static void ivshmem_use_msix(IVShmemState * s)
> +{
> +    int i;
> +
> +    if (!msix_present(&s->dev)) {
> +        return;
> +    }
> +
> +    for (i = 0; i < s->vectors; i++) {
> +        msix_vector_use(&s->dev, i);
> +    }
> +}
> +
>  static void ivshmem_reset(DeviceState *d)
>  {
>     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>
>     s->intrstatus = 0;
> +    msix_reset(&s->dev);
> +    ivshmem_use_msix(s);
>     return;
>  }
>
> @@ -535,12 +553,8 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
>     return value;
>  }
>
> -static void ivshmem_setup_msi(IVShmemState * s) {
> -
> -    int i;
> -
> -    /* allocate the MSI-X vectors */
> -
> +static void ivshmem_setup_msi(IVShmemState * s)
> +{
>     memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
>     if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
>         pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> @@ -551,13 +565,10 @@ static void ivshmem_setup_msi(IVShmemState * s) {
>         exit(1);
>     }
>
> -    /* 'activate' the vectors */
> -    for (i = 0; i < s->vectors; i++) {
> -        msix_vector_use(&s->dev, i);
> -    }
> -
>     /* allocate Qemu char devices for receiving interrupts */
>     s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
> +
> +    ivshmem_use_msix(s);
>  }
>
>  static void ivshmem_save(QEMUFile* f, void *opaque)
> @@ -610,6 +621,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
>     return 0;
>  }
>
> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> +                                uint32_t val, int len)
> +{
> +    pci_default_write_config(pci_dev, address, val, len);
> +    msix_write_config(pci_dev, address, val, len);
> +}
> +
>  static int pci_ivshmem_init(PCIDevice *dev)
>  {
>     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -734,6 +752,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
>     }
>
> +    s->dev.config_write = ivshmem_write_config;
> +
>     return 0;
>  }
>
>

Re: [Qemu-devel] [PATCH master/v1.0.x] ivshmem: add missing msix calls

[Andreas Färber]

Am 13.01.2012 23:43, schrieb Cam Macdonell:
> Can this patch be merged, please?

You need to cc qemu-stable if you want it backported to v1.0.x once applied.

Andreas

> On Mon, Dec 5, 2011 at 12:48 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> ivshmem used msix but didn't call it on either reset or
>> config write paths. This used to partically work since
>> guests don't use all of msi-x configuration fields,
>> and reset is rarely used, but the patch 'msix: track function masked
>> in pci device state' broke that. Fix by adding appropriate calls.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Reported-by: Cam Macdonell <cam@cs.ualberta.ca>
>> Tested-by: Cam Macdonell <cam@cs.ualberta.ca>
>>
>> ---
>>
>> Please apply the following to both master
>> and 1.0 stable branch. Thanks!
>>
>> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
>> index 242fbea..c58f4d3 100644
>> --- a/hw/ivshmem.c
>> +++ b/hw/ivshmem.c
>> @@ -500,11 +500,29 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>>     return;
>>  }
>>
>> +/* Select the MSI-X vectors used by device.
>> + * ivshmem maps events to vectors statically, so
>> + * we just enable all vectors on init and after reset. */
>> +static void ivshmem_use_msix(IVShmemState * s)
>> +{
>> +    int i;
>> +
>> +    if (!msix_present(&s->dev)) {
>> +        return;
>> +    }
>> +
>> +    for (i = 0; i < s->vectors; i++) {
>> +        msix_vector_use(&s->dev, i);
>> +    }
>> +}
>> +
>>  static void ivshmem_reset(DeviceState *d)
>>  {
>>     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>>
>>     s->intrstatus = 0;
>> +    msix_reset(&s->dev);
>> +    ivshmem_use_msix(s);
>>     return;
>>  }
>>
>> @@ -535,12 +553,8 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
>>     return value;
>>  }
>>
>> -static void ivshmem_setup_msi(IVShmemState * s) {
>> -
>> -    int i;
>> -
>> -    /* allocate the MSI-X vectors */
>> -
>> +static void ivshmem_setup_msi(IVShmemState * s)
>> +{
>>     memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
>>     if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
>>         pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
>> @@ -551,13 +565,10 @@ static void ivshmem_setup_msi(IVShmemState * s) {
>>         exit(1);
>>     }
>>
>> -    /* 'activate' the vectors */
>> -    for (i = 0; i < s->vectors; i++) {
>> -        msix_vector_use(&s->dev, i);
>> -    }
>> -
>>     /* allocate Qemu char devices for receiving interrupts */
>>     s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
>> +
>> +    ivshmem_use_msix(s);
>>  }
>>
>>  static void ivshmem_save(QEMUFile* f, void *opaque)
>> @@ -610,6 +621,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
>>     return 0;
>>  }
>>
>> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
>> +                                uint32_t val, int len)
>> +{
>> +    pci_default_write_config(pci_dev, address, val, len);
>> +    msix_write_config(pci_dev, address, val, len);
>> +}
>> +
>>  static int pci_ivshmem_init(PCIDevice *dev)
>>  {
>>     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
>> @@ -734,6 +752,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>>
>>     }
>>
>> +    s->dev.config_write = ivshmem_write_config;
>> +
>>     return 0;
>>  }
>>

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

[Qemu-devel] [PATCH for v1.0 2/3] msix: Prevent bogus mask updates on MMIO accesses

[Michael S. Tsirkin]

>From: Jan Kiszka <jan.kiszka@siemens.com>

Only accesses to the MSI-X table must trigger a call to
msix_handle_mask_update, otherwise the vector
value might be out of range.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 63b41b9..2969601 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -176,6 +176,12 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
     PCIDevice *dev = opaque;
     unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
     int vector = offset / PCI_MSIX_ENTRY_SIZE;
+
+    /* MSI-X page includes a read-only PBA and a writeable Vector Control. */
+    if (vector >= dev->msix_entries_nr) {
+        return;
+    }
+
     pci_set_long(dev->msix_table_page + offset, val);
     msix_handle_mask_update(dev, vector);
 }
-- 
1.7.5.53.gc233e

[Qemu-devel] [PATCH for v1.0 3/3] msix: avoid mask updates if mask is unchanged

[Michael S. Tsirkin]

Check pending bit only if vector mask status changed.
This is not really important for qemu.git but helps
fix a bug in qemu-kvm.git.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/msix.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 2969601..149eed2 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -118,17 +118,25 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
 }
 
-static int msix_is_masked(PCIDevice *dev, int vector)
+static bool msix_vector_masked(PCIDevice *dev, int vector, bool fmask)
 {
-    unsigned offset =
-        vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-    return dev->msix_function_masked ||
-	   dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
+    unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
+    return fmask || dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
 }
 
-static void msix_handle_mask_update(PCIDevice *dev, int vector)
+static bool msix_is_masked(PCIDevice *dev, int vector)
 {
-    if (!msix_is_masked(dev, vector) && msix_is_pending(dev, vector)) {
+    return msix_vector_masked(dev, vector, dev->msix_function_masked);
+}
+
+static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked)
+{
+    bool is_masked = msix_is_masked(dev, vector);
+    if (is_masked == was_masked) {
+        return;
+    }
+
+    if (!is_masked && msix_is_pending(dev, vector)) {
         msix_clr_pending(dev, vector);
         msix_notify(dev, vector);
     }
@@ -166,7 +174,8 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
     }
 
     for (vector = 0; vector < dev->msix_entries_nr; ++vector) {
-        msix_handle_mask_update(dev, vector);
+        msix_handle_mask_update(dev, vector,
+                                msix_vector_masked(dev, vector, was_masked));
     }
 }
 
@@ -176,14 +185,16 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
     PCIDevice *dev = opaque;
     unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
     int vector = offset / PCI_MSIX_ENTRY_SIZE;
+    bool was_masked;
 
     /* MSI-X page includes a read-only PBA and a writeable Vector Control. */
     if (vector >= dev->msix_entries_nr) {
         return;
     }
 
+    was_masked = msix_is_masked(dev, vector);
     pci_set_long(dev->msix_table_page + offset, val);
-    msix_handle_mask_update(dev, vector);
+    msix_handle_mask_update(dev, vector, was_masked);
 }
 
 static const MemoryRegionOps msix_mmio_ops = {
-- 
1.7.5.53.gc233e

Re: [Qemu-devel] [PATCH for v1.0 0/3] msix: fixes for 1.0

[Anthony Liguori]

On 11/21/2011 10:56 AM, Michael S. Tsirkin wrote:
> This fixes bugs dealing with msi-x mask bits pointed out by Jan Kiszka.

Applied.  Thanks.

Regards,

Anthony Liguori

>
> Jan Kiszka (1):
>    msix: Prevent bogus mask updates on MMIO accesses
>
> Michael S. Tsirkin (2):
>    msix: track function masked in pci device state
>    msix: avoid mask updates if mask is unchanged
>
>   hw/msix.c |   48 ++++++++++++++++++++++++++++++++++++------------
>   hw/pci.h  |    2 ++
>   2 files changed, 38 insertions(+), 12 deletions(-)
>