Re: [Qemu-arm] [Qemu-devel] [PATCH for-4.2 v10 14/15] virtio-iommu-pci: Add virtio iommu pci support

[Auger Eric]

Hi Michael,

On 7/30/19 9:35 PM, Michael S. Tsirkin wrote:
> On Tue, Jul 30, 2019 at 07:21:36PM +0200, Eric Auger wrote:
>> This patch adds virtio-iommu-pci, which is the pci proxy for
>> the virtio-iommu device.
>>
>> Signed-off-by: Eric Auger <address@hidden>
> 
> This part I'm not sure we should merge just yet.  The reason being I
> think we should limit it to mmio where DT can be used to describe iommu
> topology. For PCI I don't see why we shouldn't always expose this
> in the config space, and I think it's preferable not to
> need to support a mix of DT,ACPI and PCI as options.

For context, some discussion related to this topic already arose on v7
revision of the driver:

[1] Re: [PATCH v7 0/7] Add virtio-iommu driver
https://lore.kernel.org/linux-pci/87a7ioby9u.fsf@morokweng.localdomain/

Some additional thoughts.

First considering DT boot.

THE DT description features an iommu-map property in the
pci-host-ecam-generic node that describes which RIDs are handled by the
virtio-iommu and a possible offset/mask to be applied inbetween the RID
and the streamID at the input of the IOMMU
(Documentation/devicetree/bindings/pci/pci-iommu.txt)

As far as I understand when a DMA capable device is setup, its DMA
configuration is built using that call chain:

pci_dma_configure
|_ of_dma_configure
   |_ of_iommu_configure
      |_ of_pci_iommu_init
         |_ of_map_rid

I understand you would like the iommu-map/iommu-map-mask info to be
exposed directly into the config space of the device instead of inside
the DT or IORT table. Assuming a module is initialized sufficiently
early to retrieve this info, we would need the resulting info to be
consolidated to allow pci_dma_configure chain to work seemlessly. This
sounds a significant impact on above kernel infrastructure.

This comes in addition to the development of the "small module that
loads early and pokes at the IOMMU sufficiently to get the data about
which devices use the IOMMU out of it using standard virtio config
space" evoked in [1] + the definition of the data formats to be put in
the very cfg space.

With ACPI I understand we have the same kind of infrastructure:
drivers/acpi/arm64/iort.c currently extracts the mapping between RC RIDs
and IOMMU streamids

pci_dma_configure(
|_ acpi_dma_configure
   |_ iort_iommu_configure
      |_ iort_pci_iommu_init
         |_ iort_node_map_id
            |_ iort_id_map

Maybe I fail to see the easy and right way to do the integration at
kernel level but I am a bit frightened by the efforts that would be
requested to follow your suggestion, whereas the DT infra is ready and
fully upstreamed to accept the use case.

For ACPI I agree AFAIK IORT was primarily defined by ARM, for ARM but we
prototyped IORT integration with x86 and it worked for pc machine
without major trouble.

I sent the kernel and qemu patches prototyping this IORT integration:

https://github.com/eauger/linux/tree/virtio-iommu-v0.9-iort-x86
https://github.com/eauger/qemu/tree/v3.1.0-rc3-virtio-iommu-v0.9-x86

There ACPI IORT was built for PC machine and the integration effort at
both kernel and QEMU level was low. This work would need to be rebased
and depends on kernel ACPI related patches that are not yet upstreamed
though.

Thanks

Eric
> 
>> ---
>>
>> v8 -> v9:
>> - add the msi-bypass property
>> - create virtio-iommu-pci.c
>> ---
>>  hw/virtio/Makefile.objs          |  1 +
>>  hw/virtio/virtio-iommu-pci.c     | 88 ++++++++++++++++++++++++++++++++
>>  include/hw/pci/pci.h             |  1 +
>>  include/hw/virtio/virtio-iommu.h |  1 +
>>  qdev-monitor.c                   |  1 +
>>  5 files changed, 92 insertions(+)
>>  create mode 100644 hw/virtio/virtio-iommu-pci.c
>>
>> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
>> index f42e4dd94f..80ca719f1c 100644
>> --- a/hw/virtio/Makefile.objs
>> +++ b/hw/virtio/Makefile.objs
>> @@ -27,6 +27,7 @@ obj-$(CONFIG_VIRTIO_INPUT_HOST) += virtio-input-host-pci.o
>>  obj-$(CONFIG_VIRTIO_INPUT) += virtio-input-pci.o
>>  obj-$(CONFIG_VIRTIO_RNG) += virtio-rng-pci.o
>>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon-pci.o
>> +obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu-pci.o
>>  obj-$(CONFIG_VIRTIO_9P) += virtio-9p-pci.o
>>  obj-$(CONFIG_VIRTIO_SCSI) += virtio-scsi-pci.o
>>  obj-$(CONFIG_VIRTIO_BLK) += virtio-blk-pci.o
>> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
>> new file mode 100644
>> index 0000000000..f9977096bd
>> --- /dev/null
>> +++ b/hw/virtio/virtio-iommu-pci.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * Virtio IOMMU PCI Bindings
>> + *
>> + * Copyright (c) 2019 Red Hat, Inc.
>> + * Written by Eric Auger
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License version 2 or
>> + *  (at your option) any later version.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#include "virtio-pci.h"
>> +#include "hw/virtio/virtio-iommu.h"
>> +
>> +typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI;
>> +
>> +/*
>> + * virtio-iommu-pci: This extends VirtioPCIProxy.
>> + *
>> + */
>> +#define VIRTIO_IOMMU_PCI(obj) \
>> +        OBJECT_CHECK(VirtIOIOMMUPCI, (obj), TYPE_VIRTIO_IOMMU_PCI)
>> +
>> +struct VirtIOIOMMUPCI {
>> +    VirtIOPCIProxy parent_obj;
>> +    VirtIOIOMMU vdev;
>> +};
>> +
>> +static Property virtio_iommu_pci_properties[] = {
>> +    DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
>> +    DEFINE_PROP_BOOL("msi-bypass", VirtIOIOMMUPCI, vdev.msi_bypass, true),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>> +{
>> +    VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev);
>> +    DeviceState *vdev = DEVICE(&dev->vdev);
>> +
>> +    qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
>> +    object_property_set_link(OBJECT(dev),
>> +                             OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
>> +                             "primary-bus", errp);
>> +    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
>> +}
>> +
>> +static void virtio_iommu_pci_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
>> +    PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
>> +    k->realize = virtio_iommu_pci_realize;
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +    dc->props = virtio_iommu_pci_properties;
>> +    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>> +    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_IOMMU;
>> +    pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
>> +    pcidev_k->class_id = PCI_CLASS_OTHERS;
>> +}
>> +
>> +static void virtio_iommu_pci_instance_init(Object *obj)
>> +{
>> +    VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(obj);
>> +
>> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>> +                                TYPE_VIRTIO_IOMMU);
>> +}
>> +
>> +static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = {
>> +    .base_name             = TYPE_VIRTIO_IOMMU_PCI,
>> +    .generic_name          = "virtio-iommu-pci",
>> +    .transitional_name     = "virtio-iommu-pci-transitional",
>> +    .non_transitional_name = "virtio-iommu-pci-non-transitional",
>> +    .instance_size = sizeof(VirtIOIOMMUPCI),
>> +    .instance_init = virtio_iommu_pci_instance_init,
>> +    .class_init    = virtio_iommu_pci_class_init,
>> +};
>> +
>> +static void virtio_iommu_pci_register(void)
>> +{
>> +    virtio_pci_types_register(&virtio_iommu_pci_info);
>> +}
>> +
>> +type_init(virtio_iommu_pci_register)
>> +
>> +
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index aaf1b9f70d..492ea7e68d 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -86,6 +86,7 @@ extern bool pci_available;
>>  #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>>  #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
>>  #define PCI_DEVICE_ID_VIRTIO_PMEM        0x1013
>> +#define PCI_DEVICE_ID_VIRTIO_IOMMU       0x1014
>>  
>>  #define PCI_VENDOR_ID_REDHAT             0x1b36
>>  #define PCI_DEVICE_ID_REDHAT_BRIDGE      0x0001
>> diff --git a/include/hw/virtio/virtio-iommu.h 
>> b/include/hw/virtio/virtio-iommu.h
>> index 56c8b4e57f..893ac65c0b 100644
>> --- a/include/hw/virtio/virtio-iommu.h
>> +++ b/include/hw/virtio/virtio-iommu.h
>> @@ -25,6 +25,7 @@
>>  #include "hw/pci/pci.h"
>>  
>>  #define TYPE_VIRTIO_IOMMU "virtio-iommu-device"
>> +#define TYPE_VIRTIO_IOMMU_PCI "virtio-iommu-device-base"
>>  #define VIRTIO_IOMMU(obj) \
>>          OBJECT_CHECK(VirtIOIOMMU, (obj), TYPE_VIRTIO_IOMMU)
>>  
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 58222c2211..74cf090c61 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -63,6 +63,7 @@ static const QDevAlias qdev_alias_table[] = {
>>      { "virtio-input-host-ccw", "virtio-input-host", QEMU_ARCH_S390X },
>>      { "virtio-input-host-pci", "virtio-input-host",
>>              QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>> +    { "virtio-iommu-pci", "virtio-iommu", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X 
>> },
>>      { "virtio-keyboard-ccw", "virtio-keyboard", QEMU_ARCH_S390X },
>>      { "virtio-keyboard-pci", "virtio-keyboard",
>>              QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
>> -- 
>> 2.20.1
>