Patchwork [for,v1.0,1/3] msix: track function masked in pci device state

login
register
mail settings
Submitter Michael S. Tsirkin
Date Nov. 21, 2011, 4:57 p.m.
Message ID <27ef57671106d9214757df8a1d4cc28287dae07c.1321893802.git.mst@redhat.com>
Download mbox | patch
Permalink /patch/126861/
State New
Headers show

Comments

Michael S. Tsirkin - Nov. 21, 2011, 4:57 p.m.
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(-)
Cam Macdonell - Dec. 2, 2011, 11:34 p.m.
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
>
>
Jan Kiszka - Dec. 3, 2011, 10:46 a.m.
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
Michael S. Tsirkin - Dec. 4, 2011, 10:08 a.m.
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
> >
> >

Patch

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;