Patchwork [6/9] virtio-pci: Remove duplicate test

login
register
mail settings
Submitter Juan Quintela
Date March 16, 2010, 6:51 p.m.
Message ID <ed1cba97c3c9582c918d70de4d18734c70ffe748.1268765204.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/47895/
State New
Headers show

Comments

Juan Quintela - March 16, 2010, 6:51 p.m.
We already do the test for msix on the caller, just use that test

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/msix.c       |    8 --------
 hw/virtio-pci.c |    7 ++++---
 2 files changed, 4 insertions(+), 11 deletions(-)
Michael S. Tsirkin - March 18, 2010, 7:25 a.m.
On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> We already do the test for msix on the caller, just use that test
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

NAK

I think we are better off not making assumptions
about caller behaviour in msix.c, virtio
will not be the only user forever.

> ---
>  hw/msix.c       |    8 --------
>  hw/virtio-pci.c |    7 ++++---
>  2 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 2ca0900..867f0b1 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -323,10 +323,6 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
>  {
>      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);
>  }
> @@ -336,10 +332,6 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> 
> -    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);
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 799f664..ce48ddc 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -115,9 +115,10 @@ static void virtio_pci_save_config(void * opaque, QEMUFile *f)
>  {
>      VirtIOPCIProxy *proxy = opaque;
>      pci_device_save(&proxy->pci_dev, f);
> -    msix_save(&proxy->pci_dev, f);
> -    if (msix_present(&proxy->pci_dev))
> +    if (msix_present(&proxy->pci_dev)) {
> +        msix_save(&proxy->pci_dev, f);
>          qemu_put_be16(f, proxy->vdev->config_vector);
> +    }
>  }
> 
>  static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
> @@ -135,8 +136,8 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f)
>      if (ret) {
>          return ret;
>      }
> -    msix_load(&proxy->pci_dev, f);
>      if (msix_present(&proxy->pci_dev)) {
> +        msix_load(&proxy->pci_dev, f);
>          qemu_get_be16s(f, &proxy->vdev->config_vector);
>      } else {
>          proxy->vdev->config_vector = VIRTIO_NO_VECTOR;
> -- 
> 1.6.6.1
> 
>
Juan Quintela - March 18, 2010, 8:26 a.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
>> We already do the test for msix on the caller, just use that test
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> NAK
>
> I think we are better off not making assumptions
> about caller behaviour in msix.c, virtio
> will not be the only user forever.

That makes migration testing more difficult.  Basically we are testing
if we are using msix in two places.  Obvious thing is:
- we don't test in msix_save() if msix is used.
- we don't test it in virtio_pci_save_config()

I don't care if it is one way or another, but requiring to check it in
the caller and the callee is a bit too much for me.

Later, Juan.
Michael S. Tsirkin - March 18, 2010, 8:47 a.m.
On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> >> We already do the test for msix on the caller, just use that test
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> > NAK
> >
> > I think we are better off not making assumptions
> > about caller behaviour in msix.c, virtio
> > will not be the only user forever.
> 
> That makes migration testing more difficult.  Basically we are testing
> if we are using msix in two places.  Obvious thing is:
> - we don't test in msix_save() if msix is used.
> - we don't test it in virtio_pci_save_config()
> 
> I don't care if it is one way or another, but requiring to check it in
> the caller and the callee is a bit too much for me.
> 
> Later, Juan.

msix does not require the check in the caller, by design it is
safe to call msix_save when msix is not present.
Juan Quintela - March 18, 2010, 8:59 a.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
>> >> We already do the test for msix on the caller, just use that test
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >
>> > NAK
>> >
>> > I think we are better off not making assumptions
>> > about caller behaviour in msix.c, virtio
>> > will not be the only user forever.
>> 
>> That makes migration testing more difficult.  Basically we are testing
>> if we are using msix in two places.  Obvious thing is:
>> - we don't test in msix_save() if msix is used.
>> - we don't test it in virtio_pci_save_config()
>> 
>> I don't care if it is one way or another, but requiring to check it in
>> the caller and the callee is a bit too much for me.
>> 
>> Later, Juan.
>
> msix does not require the check in the caller, by design it is
> safe to call msix_save when msix is not present.

look at it, it requires to test msix support for other things, which
amount to the same thing :(

Later, Juan.
Michael S. Tsirkin - March 18, 2010, 9:11 a.m.
On Thu, Mar 18, 2010 at 09:59:03AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> >> >> We already do the test for msix on the caller, just use that test
> >> >> 
> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> >
> >> > NAK
> >> >
> >> > I think we are better off not making assumptions
> >> > about caller behaviour in msix.c, virtio
> >> > will not be the only user forever.
> >> 
> >> That makes migration testing more difficult.  Basically we are testing
> >> if we are using msix in two places.  Obvious thing is:
> >> - we don't test in msix_save() if msix is used.
> >> - we don't test it in virtio_pci_save_config()
> >> 
> >> I don't care if it is one way or another, but requiring to check it in
> >> the caller and the callee is a bit too much for me.
> >> 
> >> Later, Juan.
> >
> > msix does not require the check in the caller, by design it is
> > safe to call msix_save when msix is not present.
> 
> look at it, it requires to test msix support for other things, which
> amount to the same thing :(
> 
> Later, Juan.

Yes, but it does not have to be the only user. We'll have at least pci
assignment use it, at some point.  IOW, msix tries to present an API
that is safe to use, and checks capability so we do not crash if it's
not there.  virtio happens to need the same test for its own reasons
(vmsave format backwards compatibility).

We could optimize the test if we cared about speed here, but we don't
...
Juan Quintela - March 18, 2010, 11:40 a.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 18, 2010 at 09:59:03AM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
>> >> >> We already do the test for msix on the caller, just use that test
>> >> >> 
>> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> >
>> >> > NAK
>> >> >
>> >> > I think we are better off not making assumptions
>> >> > about caller behaviour in msix.c, virtio
>> >> > will not be the only user forever.
>> >> 
>> >> That makes migration testing more difficult.  Basically we are testing
>> >> if we are using msix in two places.  Obvious thing is:
>> >> - we don't test in msix_save() if msix is used.
>> >> - we don't test it in virtio_pci_save_config()
>> >> 
>> >> I don't care if it is one way or another, but requiring to check it in
>> >> the caller and the callee is a bit too much for me.
>> >> 
>> >> Later, Juan.
>> >
>> > msix does not require the check in the caller, by design it is
>> > safe to call msix_save when msix is not present.
>> 
>> look at it, it requires to test msix support for other things, which
>> amount to the same thing :(
>> 
>> Later, Juan.
>
> Yes, but it does not have to be the only user. We'll have at least pci
> assignment use it, at some point.  IOW, msix tries to present an API
> that is safe to use, and checks capability so we do not crash if it's
> not there.  virtio happens to need the same test for its own reasons
> (vmsave format backwards compatibility).
>
> We could optimize the test if we cared about speed here, but we don't
> ...

It makes my vmstate work more complex, because this is the only user
that calls a vvnmstat* and uses _nothing_ of it.

rest of users are something like:

VMSTATE(...., test_function);

on the caller side.  As virtio already needs to know msix use for other
reasons, I really expect that anything else that uses msix would need
the same test for the same reason.

Later, Juan.
Michael S. Tsirkin - March 18, 2010, 1:24 p.m.
On Thu, Mar 18, 2010 at 12:40:36PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Mar 18, 2010 at 09:59:03AM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > On Thu, Mar 18, 2010 at 09:26:10AM +0100, Juan Quintela wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> > On Tue, Mar 16, 2010 at 07:51:22PM +0100, Juan Quintela wrote:
> >> >> >> We already do the test for msix on the caller, just use that test
> >> >> >> 
> >> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> >> >
> >> >> > NAK
> >> >> >
> >> >> > I think we are better off not making assumptions
> >> >> > about caller behaviour in msix.c, virtio
> >> >> > will not be the only user forever.
> >> >> 
> >> >> That makes migration testing more difficult.  Basically we are testing
> >> >> if we are using msix in two places.  Obvious thing is:
> >> >> - we don't test in msix_save() if msix is used.
> >> >> - we don't test it in virtio_pci_save_config()
> >> >> 
> >> >> I don't care if it is one way or another, but requiring to check it in
> >> >> the caller and the callee is a bit too much for me.
> >> >> 
> >> >> Later, Juan.
> >> >
> >> > msix does not require the check in the caller, by design it is
> >> > safe to call msix_save when msix is not present.
> >> 
> >> look at it, it requires to test msix support for other things, which
> >> amount to the same thing :(
> >> 
> >> Later, Juan.
> >
> > Yes, but it does not have to be the only user. We'll have at least pci
> > assignment use it, at some point.  IOW, msix tries to present an API
> > that is safe to use, and checks capability so we do not crash if it's
> > not there.  virtio happens to need the same test for its own reasons
> > (vmsave format backwards compatibility).
> >
> > We could optimize the test if we cared about speed here, but we don't
> > ...
> 
> It makes my vmstate work more complex, because this is the only user
> that calls a vvnmstat* and uses _nothing_ of it.
> 
> rest of users are something like:
> 
> VMSTATE(...., test_function);
> 
> on the caller side.  As virtio already needs to know msix use for other
> reasons, I really expect that anything else that uses msix would need
> the same test for the same reason.
> 
> Later, Juan.


Well, ok.  If you add assumptions about callers please add a comment
before function thought,
Juan Quintela - March 18, 2010, 1:47 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:

>> It makes my vmstate work more complex, because this is the only user
>> that calls a vvnmstat* and uses _nothing_ of it.
>> 
>> rest of users are something like:
>> 
>> VMSTATE(...., test_function);
>> 
>> on the caller side.  As virtio already needs to know msix use for other
>> reasons, I really expect that anything else that uses msix would need
>> the same test for the same reason.
>> 
>> Later, Juan.
>
>
> Well, ok.  If you add assumptions about callers please add a comment
> before function thought,

Yeap, thanks.

Later, Juan.

Patch

diff --git a/hw/msix.c b/hw/msix.c
index 2ca0900..867f0b1 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -323,10 +323,6 @@  void msix_save(PCIDevice *dev, QEMUFile *f)
 {
     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);
 }
@@ -336,10 +332,6 @@  void msix_load(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;

-    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);
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 799f664..ce48ddc 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -115,9 +115,10 @@  static void virtio_pci_save_config(void * opaque, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = opaque;
     pci_device_save(&proxy->pci_dev, f);
-    msix_save(&proxy->pci_dev, f);
-    if (msix_present(&proxy->pci_dev))
+    if (msix_present(&proxy->pci_dev)) {
+        msix_save(&proxy->pci_dev, f);
         qemu_put_be16(f, proxy->vdev->config_vector);
+    }
 }

 static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
@@ -135,8 +136,8 @@  static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     if (ret) {
         return ret;
     }
-    msix_load(&proxy->pci_dev, f);
     if (msix_present(&proxy->pci_dev)) {
+        msix_load(&proxy->pci_dev, f);
         qemu_get_be16s(f, &proxy->vdev->config_vector);
     } else {
         proxy->vdev->config_vector = VIRTIO_NO_VECTOR;