| Message ID | 27ef57671106d9214757df8a1d4cc28287dae07c.1321893802.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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 > > > >
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;
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(-)