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

[Jason Wang <jasowang@redhat.com>]

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

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