Re: [Qemu-devel] [PATCH V3] virtio: do not require IOMMU to be created in advance

[Jason Wang]

On 2017年03月09日 00:40, Igor Mammedov wrote:
> On Tue, 7 Mar 2017 14:47:30 +0200
> Marcel Apfelbaum<address@hidden>  wrote:
>
> > On 03/07/2017 11:09 AM, Jason Wang wrote:
> > > After commit 96a8821d2141 ("virtio: unbreak virtio-pci with IOMMU
> > > after caching ring translations"), IOMMU was required to be created in
> > > advance. This is because we can only get the correct dma_as after pci
> > > IOMMU (e.g intel_iommu) was initialized. This is suboptimal and
> > > inconvenient for user. This patch releases this by:
> > >
> > > - introduce a bus_master_ready method for PCIDeviceClass and trigger
> > >    this during pci_init_bus_master
> > > - implement virtio-pci method and 1) reset the dma_as 2) re-register
> > >    the memory listener to the new dma_as
> > >   
> > Hi Jason,
> >
> > > Cc: Paolo Bonzini<address@hidden>
> > > Signed-off-by: Jason Wang<address@hidden>
> > > ---
> > > Changes from V2:
> > > - delay pci_init_bus_master() after pci device is realized to make
> > >    bus_master_ready a more generic method
> > > ---
> > >   hw/pci/pci.c               | 11 ++++++++---
> > >   hw/virtio/virtio-pci.c     |  9 +++++++++
> > >   hw/virtio/virtio.c         | 19 +++++++++++++++++++
> > >   include/hw/pci/pci.h       |  1 +
> > >   include/hw/virtio/virtio.h |  1 +
> > >   5 files changed, 38 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 273f1e4..22e6ad9 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -83,6 +83,7 @@ static const VMStateDescription vmstate_pcibus = {
> > >   static void pci_init_bus_master(PCIDevice *pci_dev)
> > >   {
> > >       AddressSpace *dma_as = pci_device_iommu_address_space(pci_dev);
> > > +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> > >
> > >       memory_region_init_alias(&pci_dev->bus_master_enable_region,
> > >                                OBJECT(pci_dev), "bus master",
> > > @@ -90,6 +91,9 @@ static void pci_init_bus_master(PCIDevice *pci_dev)
> > >       memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
> > >       address_space_init(&pci_dev->bus_master_as,
> > >                          &pci_dev->bus_master_enable_region, pci_dev->name);
> > > +    if (pc->bus_master_ready) {
> > > +        pc->bus_master_ready(pci_dev);
> > > +    }
> > >   }
> > >
> > >   static void pcibus_machine_done(Notifier *notifier, void *data)
> > > @@ -995,9 +999,6 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> > > *pci_dev, PCIBus *bus,
> > >       pci_dev->devfn = devfn;
> > >       pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev);
> > >
> > > -    if (qdev_hotplug) {
> > > -        pci_init_bus_master(pci_dev);
> > > -    }
> > >       pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
> > >       pci_dev->irq_state = 0;
> > >       pci_config_alloc(pci_dev);
> > > @@ -1995,6 +1996,10 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
> > > **errp)
> > >           }
> > >       }
> > >
> > > +    if (qdev_hotplug) {
> > > +        pci_init_bus_master(pci_dev);
> > > +    }
> > > +
> > How does it help if we move qdev_hotplug check outside "do_pci_register_device"?
> > I suppose you want the code to run after the "realize" function?
> > If so, what happens if a "realize" function of another device needs the 
> > bus_master_as
> > (at hotplug time)?
> Conceptually,
> I'm not sure that inherited device class realize
> should be aware of uninitialized bus_master,
> which belongs to PCI device, nor should it initialize
> it by calling pci_init_bus_master() explicitly,
> it's parent's class job (PCIDevice).

Yes, I was trying to propose a workaround for 2.9. I'm sure we will 
refine the ordering in 2.10. And consider we have asked libvirt to 
create IOMMU first, I think I will withdraw the patch.

> more close to current code:
> if xen-pci-passthrough were hotplugged then it looks like
> this hunk could break it:
> hw/xen/xen_pt.c:
>   memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
> would happen with uninitialized bus_master_as
> as realize is called before pci_init_bus_master();

Yes, this won't work. This is exactly the same issue as virtio, but this 
will also break if it was created before an IOMMU.

> So the same question as Marcel's but other way around,
> why this hunk "has to" be moved.

Right.

Thanks