| Message ID | ed1cba97c3c9582c918d70de4d18734c70ffe748.1268765204.git.quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
"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.
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.
"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.
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 ...
"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.
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,
"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.
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;
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(-)