Patchwork [v3,01/39] virtio-pci: get config on init

login
register
mail settings
Submitter Avi Kivity
Date Aug. 4, 2011, 1:05 p.m.
Message ID <1312463195-13605-2-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/108486/
State New
Headers show

Comments

Avi Kivity - Aug. 4, 2011, 1:05 p.m.
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(-)
Anthony Liguori - Aug. 5, 2011, 1:52 p.m.
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)
Avi Kivity - Aug. 7, 2011, 8:20 a.m.
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.
Michael S. Tsirkin - Aug. 8, 2011, 10:36 a.m.
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)
Anthony Liguori - Aug. 8, 2011, 12:45 p.m.
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)
>
Avi Kivity - Aug. 8, 2011, 12:48 p.m.
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()).
Michael S. Tsirkin - Aug. 8, 2011, 12:56 p.m.
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
Anthony Liguori - Aug. 8, 2011, 1:02 p.m.
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
>
>
Michael S. Tsirkin - Aug. 8, 2011, 1:14 p.m.
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 ...
Anthony Liguori - Aug. 8, 2011, 1:15 p.m.
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

>

Patch

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)