Re: [Qemu-devel] [PATCH 20/39] virtio-pci: convert to memory API

[Michael S. Tsirkin]

On Mon, Aug 01, 2011 at 01:23:22PM +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 01, 2011 at 12:35:44PM +0300, Avi Kivity wrote:
> > On 08/01/2011 11:26 AM, Michael S. Tsirkin wrote:
> > >>
> > >>   static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> > >>                                   uint32_t val, int len)
> > >>   {
> > >>       VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, 
> > >> pci_dev);
> > >>  +    VirtIODevice *vdev = proxy->vdev;
> > >>
> > >>       if (PCI_COMMAND == address) {
> > >>           if (!(val&  PCI_COMMAND_MASTER)) {
> > >>  @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, 
> > >> uint32_t address,
> > >>               }
> > >>           }
> > >>       }
> > >>  +    if (address == PCI_BASE_ADDRESS_0&&  vdev->config_len) {
> > >>  +        vdev->get_config(vdev, vdev->config);
> > >>  +    }
> > >>
> > >>       pci_default_write_config(pci_dev, address, val, len);
> > >>       msix_write_config(pci_dev, address, val, len);
> > >
> > >I'm not really sure why did we get the config on map,
> > >specifically - Anthony, do you know?
> > >But if we want to do that, memory space enable might
> > >be a better place. Or maybe we just want a callback on
> > >map.
> > 
> > 
> > Just because a memory region becomes visible to the cpu is no reason
> > to have a callback.  From the device perspective, it can't tell that
> > it happened.
> 
> Well, the reason we have this logic here, I think, is
> to make sure it runs before the guest accesses
> the configuration with a write access.
> 
> I'm not sure why we don't do this in the init
> callback - Anthony?

So the following should do this.
Anthony, could you ack please?

Avi, this is on top of the memory API branch.
But since it's not exactly the same,
maybe this should go in *before* the memory API
patches, to make it easier to bisect etc?

You decide.

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 92ba3c9..cb0e0a9 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -492,7 +492,6 @@ static void virtio_write_config(PCIDevice *pci_dev, 
uint32_t address,
                                 uint32_t val, int len)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
-    VirtIODevice *vdev = proxy->vdev;
 
     if (PCI_COMMAND == address) {
         if (!(val & PCI_COMMAND_MASTER)) {
@@ -503,9 +502,6 @@ static void virtio_write_config(PCIDevice *pci_dev, 
uint32_t address,
             }
         }
     }
-    if (address == PCI_BASE_ADDRESS_0 && vdev->config_len) {
-        vdev->get_config(vdev, vdev->config);
-    }
 
     pci_default_write_config(pci_dev, address, val, len);
     msix_write_config(pci_dev, address, val, len);
@@ -672,6 +668,7 @@ 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);
+    vdev->get_config(vdev, vdev->config);
 }
 
 static int virtio_blk_init_pci(PCIDevice *pci_dev)