| Message ID | 1312463195-13605-2-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/04/2011 08:05 AM, Avi Kivity wrote: > From: "Michael S. Tsirkin"<mst@redhat.com> > > We originally did get config on map, so that > following write accesses are done on an updated config. > New memory API doesn't give us a callback > on map, and arguably, devices don't know when > cpu really can access there. So updating on > init seems cleaner. > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > Signed-off-by: Avi Kivity<avi@redhat.com> > --- > hw/virtio-pci.c | 7 ++++--- > 1 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index d685243..ca1f12f 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -506,9 +506,6 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, > register_ioport_read(addr, config_len, 1, virtio_pci_config_readb, proxy); > register_ioport_read(addr, config_len, 2, virtio_pci_config_readw, proxy); > register_ioport_read(addr, config_len, 4, virtio_pci_config_readl, proxy); > - > - if (vdev->config_len) > - vdev->get_config(vdev, vdev->config); > } > > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > @@ -689,6 +686,10 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > proxy->host_features |= 0x1<< VIRTIO_F_NOTIFY_ON_EMPTY; > proxy->host_features |= 0x1<< VIRTIO_F_BAD_FEATURE; > proxy->host_features = vdev->get_features(vdev, proxy->host_features); > + > + if (vdev->config_len) { > + vdev->get_config(vdev, vdev->config); > + } Thinking more closely, I don't think this right. Updating on map ensured that the config was refreshed after each time the bar was mapped. In the very least, the config needs to be refreshed during reset because the guest may write to the guest space which should get cleared after reset. Regards, Anthony Liguori > } > > static int virtio_blk_init_pci(PCIDevice *pci_dev)
On 08/05/2011 04:52 PM, Anthony Liguori wrote: >> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >> @@ -689,6 +686,10 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, >> VirtIODevice *vdev) >> proxy->host_features |= 0x1<< VIRTIO_F_NOTIFY_ON_EMPTY; >> proxy->host_features |= 0x1<< VIRTIO_F_BAD_FEATURE; >> proxy->host_features = vdev->get_features(vdev, >> proxy->host_features); >> + >> + if (vdev->config_len) { >> + vdev->get_config(vdev, vdev->config); >> + } > > > Thinking more closely, I don't think this right. > > Updating on map ensured that the config was refreshed after each time > the bar was mapped. In the very least, the config needs to be > refreshed during reset because the guest may write to the guest space > which should get cleared after reset. Michael, please provide the correct fix. Best merged directly, not via my patchset.
On Fri, Aug 05, 2011 at 08:52:25AM -0500, Anthony Liguori wrote: > On 08/04/2011 08:05 AM, Avi Kivity wrote: > >From: "Michael S. Tsirkin"<mst@redhat.com> > > > >We originally did get config on map, so that > >following write accesses are done on an updated config. > >New memory API doesn't give us a callback > >on map, and arguably, devices don't know when > >cpu really can access there. So updating on > >init seems cleaner. > > > >Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > >Signed-off-by: Avi Kivity<avi@redhat.com> > >--- > > hw/virtio-pci.c | 7 ++++--- > > 1 files changed, 4 insertions(+), 3 deletions(-) > > > >diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > >index d685243..ca1f12f 100644 > >--- a/hw/virtio-pci.c > >+++ b/hw/virtio-pci.c > >@@ -506,9 +506,6 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, > > register_ioport_read(addr, config_len, 1, virtio_pci_config_readb, proxy); > > register_ioport_read(addr, config_len, 2, virtio_pci_config_readw, proxy); > > register_ioport_read(addr, config_len, 4, virtio_pci_config_readl, proxy); > >- > >- if (vdev->config_len) > >- vdev->get_config(vdev, vdev->config); > > } > > > > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > >@@ -689,6 +686,10 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > > proxy->host_features |= 0x1<< VIRTIO_F_NOTIFY_ON_EMPTY; > > proxy->host_features |= 0x1<< VIRTIO_F_BAD_FEATURE; > > proxy->host_features = vdev->get_features(vdev, proxy->host_features); > >+ > >+ if (vdev->config_len) { > >+ vdev->get_config(vdev, vdev->config); > >+ } > > Thinking more closely, I don't think this right. > > Updating on map ensured that the config was refreshed after each > time the bar was mapped. In the very least, the config needs to be > refreshed during reset because the guest may write to the guest > space which should get cleared after reset. > > Regards, > > Anthony Liguori Not sure I understand. Which register, for example, do you have in mind? Could you clarify please? > > } > > > > static int virtio_blk_init_pci(PCIDevice *pci_dev)
On 08/08/2011 05:36 AM, Michael S. Tsirkin wrote: >> Thinking more closely, I don't think this right. >> >> Updating on map ensured that the config was refreshed after each >> time the bar was mapped. In the very least, the config needs to be >> refreshed during reset because the guest may write to the guest >> space which should get cleared after reset. >> >> Regards, >> >> Anthony Liguori > > Not sure I understand. Which register, for example, > do you have in mind? > Could you clarify please? Actually, you never need to call config_get() AFAICT. It's called in every read/write access. So I think the code you changed is extraneous now. Regards, Anthony Liguori > >>> } >>> >>> static int virtio_blk_init_pci(PCIDevice *pci_dev) >
On 08/08/2011 03:45 PM, Anthony Liguori wrote: > > Actually, you never need to call config_get() AFAICT. It's called in > every read/write access. So I think the code you changed is > extraneous now. Ok; I'll drop this patch and report (and just remove the code in virtio_map()).
On Mon, Aug 08, 2011 at 07:45:19AM -0500, Anthony Liguori wrote: > On 08/08/2011 05:36 AM, Michael S. Tsirkin wrote: > >>Thinking more closely, I don't think this right. > >> > >>Updating on map ensured that the config was refreshed after each > >>time the bar was mapped. In the very least, the config needs to be > >>refreshed during reset because the guest may write to the guest > >>space which should get cleared after reset. > >> > >>Regards, > >> > >>Anthony Liguori > > > >Not sure I understand. Which register, for example, > >do you have in mind? > >Could you clarify please? > > Actually, you never need to call config_get() AFAICT. It's called > in every read/write access. Every read, yes. But every write? Are you sure? > So I think the code you changed is > extraneous now. > > Regards, > > Anthony Liguori
On 08/08/2011 07:56 AM, Michael S. Tsirkin wrote: > On Mon, Aug 08, 2011 at 07:45:19AM -0500, Anthony Liguori wrote: >> On 08/08/2011 05:36 AM, Michael S. Tsirkin wrote: >>>> Thinking more closely, I don't think this right. >>>> >>>> Updating on map ensured that the config was refreshed after each >>>> time the bar was mapped. In the very least, the config needs to be >>>> refreshed during reset because the guest may write to the guest >>>> space which should get cleared after reset. >>>> >>>> Regards, >>>> >>>> Anthony Liguori >>> >>> Not sure I understand. Which register, for example, >>> do you have in mind? >>> Could you clarify please? >> >> Actually, you never need to call config_get() AFAICT. It's called >> in every read/write access. > > Every read, yes. But every write? Are you sure? Yeah, not on write, but I think this is a bug. get_config() should be called before doing the memcpy() in order to have a proper RMW. Regards, Anthony Liguori > >> So I think the code you changed is >> extraneous now. >> >> Regards, >> >> Anthony Liguori > >
On Mon, Aug 08, 2011 at 08:02:08AM -0500, Anthony Liguori wrote: > On 08/08/2011 07:56 AM, Michael S. Tsirkin wrote: > >On Mon, Aug 08, 2011 at 07:45:19AM -0500, Anthony Liguori wrote: > >>On 08/08/2011 05:36 AM, Michael S. Tsirkin wrote: > >>>>Thinking more closely, I don't think this right. > >>>> > >>>>Updating on map ensured that the config was refreshed after each > >>>>time the bar was mapped. In the very least, the config needs to be > >>>>refreshed during reset because the guest may write to the guest > >>>>space which should get cleared after reset. > >>>> > >>>>Regards, > >>>> > >>>>Anthony Liguori > >>> > >>>Not sure I understand. Which register, for example, > >>>do you have in mind? > >>>Could you clarify please? > >> > >>Actually, you never need to call config_get() AFAICT. It's called > >>in every read/write access. > > > >Every read, yes. But every write? Are you sure? > > Yeah, not on write, but I think this is a bug. get_config() should > be called before doing the memcpy() in order to have a proper RMW. > > Regards, > > Anthony Liguori Probably not noticeable because guests don't do the RMW in practice. We also send the config over on migration. That's probably a bug as well ...
On 08/08/2011 08:14 AM, Michael S. Tsirkin wrote: > Probably not noticeable because guests don't do the RMW > in practice. > We also send the config over on migration. > That's probably a bug as well ... It's probably unnecessary, but I don't think it's a bug.. Regards, Anthony Liguori >
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index d685243..ca1f12f 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -506,9 +506,6 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, register_ioport_read(addr, config_len, 1, virtio_pci_config_readb, proxy); register_ioport_read(addr, config_len, 2, virtio_pci_config_readw, proxy); register_ioport_read(addr, config_len, 4, virtio_pci_config_readl, proxy); - - if (vdev->config_len) - vdev->get_config(vdev, vdev->config); } static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, @@ -689,6 +686,10 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE; proxy->host_features = vdev->get_features(vdev, proxy->host_features); + + if (vdev->config_len) { + vdev->get_config(vdev, vdev->config); + } } static int virtio_blk_init_pci(PCIDevice *pci_dev)