Skip to contentSkip to conversation

qemu/msi: fix segfault in msix_save

PATCH Superseded Michael S. Tsirkin 3 participants 10 messages Jul 5, 2009 1 message not in the archive KVM

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Michael S. Tsirkin

author
#1 lore raw

PATCH v4 1/5 qemu/msi: fix segfault in msix_save

In reply to a message not in the archive

This fixes segfault reported by Kevin Wolf,
and simplifies the code in msix_save.

Reported-by Kevin Wolf <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>

---
Fixed brace usage reported by Blue Swirl.

 hw/msix.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
hw/msix.c +7 -5
index 4ab6da6..b67ea39 100644
@@ -284,11 +284,13 @@ int msix_uninit(PCIDevice *dev)
 
 void msix_save(PCIDevice *dev, QEMUFile *f)
 {
-    unsigned nentries = (pci_get_word(dev->config + PCI_MSIX_FLAGS) &
-                         PCI_MSIX_FLAGS_QSIZE) + 1;
-    qemu_put_buffer(f, dev->msix_table_page, nentries * MSIX_ENTRY_SIZE);
-    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING,
-                    (nentries + 7) / 8);
+    unsigned n = dev->msix_entries_nr;
+
+    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
+        return;
+
+    qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
+    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
 }
 
 /* Should be called after restoring the config space. */
Signature · 1 line
1.6.2.2
author
#2 lore raw

PATCH v4 2/5 qemu/virtio: remove control vector save

In reply to a message not in the archive

control vector is already saved/restored by virtio-pci, it does not need to be done in virtio.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/virtio.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
hw/virtio.c +0 -3
index 843be86..41e7ca2 100644
@@ -626,9 +626,6 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_be32(f, vdev->config_len);
     qemu_put_buffer(f, vdev->config, vdev->config_len);
 
-    if (vdev->nvectors)
-        qemu_put_be16s(f, &vdev->config_vector);
-
     for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
         if (vdev->vq[i].vring.num == 0)
             break;
Signature · 1 line
1.6.2.2
author
#3 lore raw

PATCH v4 3/5 qemu/msi: clean used vectors state on load

In reply to a message not in the archive

Clean up msix vector usage state on load. Since guest might have control over it through the device, the device will have to load this state from file.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/msix.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
hw/msix.c +1 -0
index b67ea39..33549f5 100644
@@ -301,6 +301,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
     if (!dev->cap_present & QEMU_PCI_CAP_MSIX)
         return;
 
+    msix_free_irq_entries(dev);
     qemu_get_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
     qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
 }
Signature · 1 line
1.6.2.2
author
#4 lore raw

PATCH v4 4/5 qemu/msi: missing braces

In reply to a message not in the archive

MSIX present bit is tested incorrectly, and only happens to work because the bit we are testing is 0x1. Add braces to fix this.

Reported-by Blue Swirl <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/msix.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
hw/msix.c +1 -1
index 33549f5..db72cc3 100644
@@ -298,7 +298,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;
 
-    if (!dev->cap_present & QEMU_PCI_CAP_MSIX)
+    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
         return;
 
     msix_free_irq_entries(dev);
Signature · 1 line
1.6.2.2
author
#10 lore raw

PATCH v4 5/5 qemu/virtio: mark msi vectors used on load

In reply to a message not in the archive

Usage of msi vectors is controlled by the guest and so needs to be restored on load. Do this for msi vectors used by the virtio device.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/virtio-pci.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)
hw/virtio-pci.c +10 -3
index f7da503..ddc3abb 100644
@@ -131,6 +131,10 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     msix_load(&proxy->pci_dev, f);
     if (msix_present(&proxy->pci_dev))
         qemu_get_be16s(f, &proxy->vdev->config_vector);
+    else
+        proxy->vdev->config_vector = VIRTIO_NO_VECTOR;
+    if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR)
+        return msix_vector_use(&proxy->pci_dev, proxy->vdev->config_vector);
     return 0;
 }
 

@@ -138,10 +142,13 @@ static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = opaque;
     uint16_t vector;
-    if (!msix_present(&proxy->pci_dev))
-        return 0;
-    qemu_get_be16s(f, &vector);
+    if (msix_present(&proxy->pci_dev))
+        qemu_get_be16s(f, &vector);
+    else
+        vector = VIRTIO_NO_VECTOR;
     virtio_queue_set_vector(proxy->vdev, n, vector);
+    if (vector != VIRTIO_NO_VECTOR)
+        return msix_vector_use(&proxy->pci_dev, vector);
     return 0;
 }
Signature · 1 line
1.6.2.2
#5 lore raw

Re: [PATCHv4 4/5] qemu/msi: missing braces

In reply to Michael S. Tsirkin
Quoted 14 lines · Michael S. Tsirkin

MSIX present bit is tested incorrectly, and only happens to work because
the bit we are testing is 0x1. Add braces to fix this.

Reported-by Blue Swirl <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>

---

  hw/msix.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 33549f5..db72cc3 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -298,7 +298,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)

Quoted 5 lines
  {
     unsigned n = dev->msix_entries_nr;

- if (!dev->cap_present & QEMU_PCI_CAP_MSIX)
+ if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))

With the braces comment I meant that while working on the code, you should update it to match CODING_STYLE:

if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
    return;
}
author
#6 lore raw

Re: [PATCHv4 4/5] qemu/msi: missing braces

Quoted 18 lines · Blue Swirl
Quoted 17 lines · Michael S. Tsirkin

MSIX present bit is tested incorrectly, and only happens to work because
the bit we are testing is 0x1. Add braces to fix this.

Reported-by Blue Swirl <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>

---

  hw/msix.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 33549f5..db72cc3 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -298,7 +298,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)

  {
     unsigned n = dev->msix_entries_nr;
Quoted 8 lines
Quoted 2 lines

- if (!dev->cap_present & QEMU_PCI_CAP_MSIX)
+ if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))

With the braces comment I meant that while working on the code, you should update it to match CODING_STYLE:

if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
    return;
}

Yea ... it's probably better to do this all over the file, not piecewise, though. No?

Signature · 1 line
MST
#7 lore raw

Re: [PATCHv4 4/5] qemu/msi: missing braces

Quoted 29 lines · Michael S. Tsirkin
Quoted 18 lines · Blue Swirl
Quoted 17 lines · Michael S. Tsirkin

MSIX present bit is tested incorrectly, and only happens to work because
the bit we are testing is 0x1. Add braces to fix this.

Reported-by Blue Swirl <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>

---

  hw/msix.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 33549f5..db72cc3 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -298,7 +298,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)

  {
     unsigned n = dev->msix_entries_nr;
Quoted 8 lines
Quoted 2 lines

- if (!dev->cap_present & QEMU_PCI_CAP_MSIX)
+ if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))

With the braces comment I meant that while working on the code, you should update it to match CODING_STYLE:

if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
    return;
}
Quoted 2 lines

Yea ... it's probably better to do this all over the file, not piecewise,
though. No?

I think it's better to do it together with other changes:
http://lists.gnu.org/archive/html/qemu-devel/2009-05/msg00925.html

#8 lore raw

Re: [PATCHv4 4/5] qemu/msi: missing braces

In reply to Michael S. Tsirkin
Quoted 10 lines · Michael S. Tsirkin
Quoted 6 lines

With the braces comment I meant that while working on the code, you should update it to match CODING_STYLE:

if (!(dev->cap_present&  QEMU_PCI_CAP_MSIX)) {
     return;
}

Yea ... it's probably better to do this all over the file, not piecewise, though. No?

No, that just causes churn (and merge conflicts for me). Better to only fix if you have a patch that modifies the same place.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#9 lore raw

Re: [PATCHv4 4/5] qemu/msi: missing braces

Quoted 8 lines · Avi Kivity
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines

With the braces comment I meant that while working on the code, you should update it to match CODING_STYLE:

if (!(dev->cap_present&  QEMU_PCI_CAP_MSIX)) {
     return;
}
Quoted 6 lines
Quoted 3 lines

Yea ... it's probably better to do this all over the file, not piecewise, though. No?

No, that just causes churn (and merge conflicts for me). Better to only fix if you have a patch that modifies the same place.

OK, I redid these patches with {}.
Even though the kernel style is better! ;)

Signature · 1 line
MST