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

[Marcel Apfelbaum <marcel@redhat.com>]

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 <pbonzini@redhat.com>
>>>> Signed-off-by: Jason Wang <jasowang@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.

>
>> 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

> Thanks
>
>>
>> An example is vfio-pci device. It is using
>> pci_device_iommu_address_space() in vfio_realize(). Though I _guess_
>> there may be other reasons behind, e.g., VFIOAddressSpace should be
>> 1:1 mapped to bus master address space, so we may want to make sure
>> this address space will be the same when different devices are using
>> the same address space (while bus_master_as is one per device, even if
>> two devices have the same backend address space, there will be two
>> distinct bus_master_as).
>>
>> IIUC, bus_master_as is only used to emulate the master bit in PCI
>> command register. In that case, that should only be there for the
>> guest operations, while imho device realization is "emulation code",
>> which can directly use pci_device_iommu_address_space(). Actually, if
>> it plays with bus_master_as even if it can, I guess it'll just fail
>> since that region has not yet been enabled.
>>
>> Please kindly correct me if I made a mistake...
>>
>> Thanks,
>>
>> -- peterx
>