On Tue, 7 Mar 2017 14:47:30 +0200
Marcel Apfelbaum <mar...@redhat.com> 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 <pbonz...@redhat.com>
> > Signed-off-by: Jason Wang <jasow...@redhat.com>
> > ---
> > 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).
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();
So the same question as Marcel's but other way around,
why this hunk "has to" be moved.
>
> Thanks,
> Marcel
>
> > /* rom loading */
> > is_default_rom = false;
> > if (pci_dev->romfile == NULL && pc->romfile != NULL) {
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index b76f3f6..21cbda2 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1845,6 +1845,14 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
> > address_space_destroy(&proxy->modern_as);
> > }
> >
> > +static void virtio_pci_bus_master_ready(PCIDevice *pci_dev)
> > +{
> > + VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> > + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > +
> > + virtio_device_reset_dma_as(vdev);
> > +}
> > +
> > static void virtio_pci_reset(DeviceState *qdev)
> > {
> > VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> > @@ -1904,6 +1912,7 @@ static void virtio_pci_class_init(ObjectClass *klass,
> > void *data)
> > dc->props = virtio_pci_properties;
> > k->realize = virtio_pci_realize;
> > k->exit = virtio_pci_exit;
> > + k->bus_master_ready = virtio_pci_bus_master_ready;
> > k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> > k->revision = VIRTIO_PCI_ABI_VERSION;
> > k->class_id = PCI_CLASS_OTHERS;
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index efce4b3..09f4cf4 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2594,6 +2594,25 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice
> > *vdev)
> > return virtio_bus_ioeventfd_enabled(vbus);
> > }
> >
> > +void virtio_device_reset_dma_as(VirtIODevice *vdev)
> > +{
> > + DeviceState *qdev = DEVICE(vdev);
> > + BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> > + VirtioBusState *bus = VIRTIO_BUS(qbus);
> > + VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
> > + bool has_iommu = virtio_host_has_feature(vdev,
> > VIRTIO_F_IOMMU_PLATFORM);
> > +
> > + if (klass->get_dma_as != NULL && has_iommu) {
> > + vdev->dma_as = klass->get_dma_as(qbus->parent);
> > + } else {
> > + vdev->dma_as = &address_space_memory;
> > + }
> > +
> > + memory_listener_unregister(&vdev->listener);
> > + memory_listener_register(&vdev->listener, vdev->dma_as);
> > +}
this mostly subset of virtio_bus_device_plugged(), generalize?
> > +
> > +
> > static const TypeInfo virtio_device_info = {
> > .name = TYPE_VIRTIO_DEVICE,
> > .parent = TYPE_DEVICE,
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 9349acb..e700a58 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -207,6 +207,7 @@ typedef struct PCIDeviceClass {
> >
> > void (*realize)(PCIDevice *dev, Error **errp);
> > int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> > + void (*bus_master_ready)(PCIDevice *dev);
> > PCIUnregisterFunc *exit;
> > PCIConfigReadFunc *config_read;
> > PCIConfigWriteFunc *config_write;
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 15efcf2..21fa273 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -283,6 +283,7 @@ void virtio_device_stop_ioeventfd(VirtIODevice *vdev);
> > int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
> > void virtio_device_release_ioeventfd(VirtIODevice *vdev);
> > bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
> > +void virtio_device_reset_dma_as(VirtIODevice *vdev);
> > EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
> > void virtio_queue_host_notifier_read(EventNotifier *n);
> > void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext
> > *ctx,
> >
>
>
