diff mbox

[20/39] virtio-pci: convert to memory API

Message ID 20110803151635.GA15804@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Aug. 3, 2011, 3:16 p.m. UTC 
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.

Comments

Avi Kivity Aug. 3, 2011, 3:23 p.m. UTC | #1 
On 08/03/2011 06:16 PM, Michael S. Tsirkin wrote:
> >
> >  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.

How can I decide?  This is your area.

I'll be happy to rebase if needed.
Michael S. Tsirkin Aug. 3, 2011, 3:38 p.m. UTC | #2 
On Wed, Aug 03, 2011 at 06:23:41PM +0300, Avi Kivity wrote:
> On 08/03/2011 06:16 PM, Michael S. Tsirkin wrote:
> >>
> >>  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.
> 
> How can I decide?  This is your area.
> 
> I'll be happy to rebase if needed.

OK, I've sent a patch, pls stick it on your branch
and your changes can go on top. Thanks!

> -- 
> error compiling committee.c: too many arguments to function
diff mbox

Patch

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)