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

2011-12-05 Thread 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 m...@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

2011-12-05 Thread Cam Macdonell 
On Mon, Dec 5, 2011 at 2:08 AM, Michael S. Tsirkin m...@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 m...@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;
  }

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

2011-12-04 Thread 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 m...@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 m...@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

2011-12-04 Thread 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 m...@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

2011-12-04 Thread Jan Kiszka 
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 m...@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



signature.asc
Description: OpenPGP digital signature

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

2011-12-04 Thread 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 m...@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

2011-12-04 Thread Cam Macdonell 
On Sun, Dec 4, 2011 at 3:20 AM, Michael S. Tsirkin m...@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 m...@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

2011-12-03 Thread Jan Kiszka 
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



signature.asc
Description: OpenPGP digital signature

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

2011-12-02 Thread 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 m...@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 m...@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

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

2011-11-21 Thread 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 m...@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