On 2017年03月08日 16:17, Marcel Apfelbaum wrote:
On 03/08/2017 05:15 AM, Jason Wang wrote:
On 2017年03月08日 10:43, Peter Xu wrote:
On Tue, Mar 07, 2017 at 02:47:30PM +0200, Marcel Apfelbaum 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?
Yes.
If so, what happens if a "realize" function of another device needs
the bus_master_as
(at hotplug time)?
I'm not sure this is really needed. What kind of device need to check
hotplug during their realize? (Looks like we don't have such kind of
device now).
I am sorry I was not clear enough:
If a device is added after the system is up (hotplug), we cannot
depend on the "machine_done"
event to enable "bus master".
This is why we have
if (qdev_hotplug)
pci_init_bus_master(pci_dev);
The code you proposed changes the order, so this call is done *after*
realize.
My question was: What if any other device may require the bus_master_as
at realize time (and can be hot-plugged) ?
For example: hcd-ehci/hcd-ohci devices call pci_get_address_space()
and caches the bus_master_as.
It is possible the mentioned devices can't be hot-plugged or
are not relevant for PCIe.
I am only saying we should double check when making such a change.
Can't agree more.
For hcd-ehci/hcd-ohci, simply caching the as should be fine. But if they
want to more things like virito, they should delay it to bus_master_ready.
My unmature understanding is that, we can just call
pci_device_iommu_address_space() if device realization really needs
the address space, rather than using bus_master_as.
A little issue is pci_device_iommu_address_space() can be wrong if it
was called before OMMU was created.
At hotplug time this is irrelevant because the iommu has already been
created.
Thanks,
Marcel
Yes.
Thanks
