[Qemu-devel] [PATCHv4 1/5] qemu/msi: fix segfault in msix_save

[Qemu-devel] [PATCHv4 1/5] qemu/msi: fix segfault in msix_save

[Michael S. Tsirkin]

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

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Fixed brace usage reported by Blue Swirl.

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

diff --git a/hw/msix.c b/hw/msix.c
index 4ab6da6..b67ea39 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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. */
-- 
1.6.2.2

[Qemu-devel] [PATCHv4 2/5] qemu/virtio: remove control vector save

[Michael S. Tsirkin]

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

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

diff --git a/hw/virtio.c b/hw/virtio.c
index 843be86..41e7ca2 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -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;
-- 
1.6.2.2

[Qemu-devel] [PATCHv4 3/5] qemu/msi: clean used vectors state on load

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 hw/msix.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index b67ea39..33549f5 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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);
 }
-- 
1.6.2.2

[Qemu-devel] [PATCHv4 4/5] qemu/msi: missing braces

[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 <blauwirbel@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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;
 
-    if (!dev->cap_present & QEMU_PCI_CAP_MSIX)
+    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
         return;
 
     msix_free_irq_entries(dev);
-- 
1.6.2.2

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

[Blue Swirl]

On 7/5/09, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 <blauwirbel@gmail.com>
>  Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  ---
>   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;
>
>  -    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;
}

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

[Michael S. Tsirkin]

On Sun, Jul 05, 2009 at 02:48:12PM +0300, Blue Swirl wrote:
> On 7/5/09, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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 <blauwirbel@gmail.com>
> >  Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >  ---
> >   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;
> >
> >  -    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?

-- 
MST

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

[Blue Swirl]

On 7/5/09, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Jul 05, 2009 at 02:48:12PM +0300, Blue Swirl wrote:
>  > On 7/5/09, Michael S. Tsirkin <mst@redhat.com> wrote:
>  > > 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 <blauwirbel@gmail.com>
>  > >  Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  > >  ---
>  > >   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;
>  > >
>  > >  -    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?

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

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

[Avi Kivity]

On 07/05/2009 02:56 PM, Michael S. Tsirkin wrote:
>> 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.

-- 
error compiling committee.c: too many arguments to function

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

[Michael S. Tsirkin]

On Sun, Jul 05, 2009 at 03:07:06PM +0300, Avi Kivity wrote:
> On 07/05/2009 02:56 PM, Michael S. Tsirkin wrote:
>>> 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.

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

-- 
MST

[Qemu-devel] [PATCHv4 5/5] qemu/virtio: mark msi vectors used on load

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 hw/virtio-pci.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index f7da503..ddc3abb 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -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;
 }
 
-- 
1.6.2.2