QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Auger Eric <eric.auger@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: jean-philippe@linaro.org, kevin.tian@intel.com,
	peter.maydell@linaro.org, tn@semihalf.com, qemu-devel@nongnu.org,
	peterx@redhat.com, alex.williamson@redhat.com,
	qemu-arm@nongnu.org, bharat.bhushan@nxp.com,
	eric.auger.pro@gmail.com
Subject: Re: [Qemu-devel] [PATCH for-4.2 v10 14/15] virtio-iommu-pci: Add virtio iommu pci support
Date: Thu, 1 Aug 2019 14:15:03 +0200
Message-ID: <a0e88a60-083b-2730-5db8-7bba0299974b@redhat.com> (raw)
In-Reply-To: <20190730153305-mutt-send-email-mst@kernel.org>

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 <eric.auger@redhat.com>
> 
> 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
> 


  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-30 17:21 [Qemu-devel] [PATCH for-4.2 v10 00/15] VIRTIO-IOMMU device Eric Auger
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 01/15] update-linux-headers: Import virtio_iommu.h Eric Auger
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 02/15] linux-headers: update against 5.3-rc2 Eric Auger
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 03/15] virtio-iommu: Add skeleton Eric Auger
2019-08-15 13:54   ` Peter Xu
2019-08-29 12:18     ` Auger Eric
2019-08-30  1:26       ` Peter Xu
2019-08-30  8:12         ` Auger Eric
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 04/15] virtio-iommu: Decode the command payload Eric Auger
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 05/15] virtio-iommu: Add the iommu regions Eric Auger
2019-08-16  4:00   ` Peter Xu
2019-08-29 12:51     ` Auger Eric
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 06/15] virtio-iommu: Endpoint and domains structs and helpers Eric Auger
2019-08-16  4:17   ` Peter Xu
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 07/15] virtio-iommu: Implement attach/detach command Eric Auger
2019-08-16  4:27   ` Peter Xu
2019-08-29 14:24     ` Auger Eric
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 08/15] virtio-iommu: Implement map/unmap Eric Auger
2019-08-19  8:11   ` Peter Xu
2019-09-03 11:37     ` Auger Eric
2019-09-04  1:44       ` Peter Xu
2019-09-04  4:23         ` Tian, Kevin
2019-09-04  5:37           ` Peter Xu
2019-09-04  5:46             ` Tian, Kevin
2019-09-04  7:54               ` Auger Eric
2019-09-04  8:32                 ` Peter Xu
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 09/15] virtio-iommu: Implement translate Eric Auger
2019-08-19  8:24   ` Peter Xu
2019-09-03 11:45     ` Auger Eric
2019-09-04  1:58       ` Peter Xu
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 10/15] virtio-iommu: Implement probe request Eric Auger
2019-08-19 12:08   ` Peter Xu
2019-09-03 12:23     ` Auger Eric
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 11/15] virtio-iommu: Expose the IOAPIC MSI reserved region when relevant Eric Auger
2019-07-30 19:38   ` Michael S. Tsirkin
2019-07-30 23:20     ` Tian, Kevin
2019-07-31  9:05       ` Auger Eric
2019-07-31 19:25       ` Michael S. Tsirkin
2019-07-31 19:44         ` Auger Eric
2019-07-31 23:23           ` Tian, Kevin
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 12/15] virtio-iommu: Implement fault reporting Eric Auger
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 13/15] virtio_iommu: Handle reserved regions in translation process Eric Auger
2019-08-19 12:44   ` Peter Xu
2019-09-01  6:38   ` Michael S. Tsirkin
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 14/15] virtio-iommu-pci: Add virtio iommu pci support Eric Auger
2019-07-30 19:35   ` Michael S. Tsirkin
2019-08-01 12:15     ` Auger Eric [this message]
2019-08-01 13:06       ` Michael S. Tsirkin
2019-08-01 13:49         ` Auger Eric
2019-09-01  6:40           ` Michael S. Tsirkin
2019-09-04 14:19             ` Auger Eric
2019-09-04 21:36               ` Michael S. Tsirkin
2019-07-30 17:21 ` [Qemu-devel] [PATCH for-4.2 v10 15/15] hw/arm/virt: Add the virtio-iommu device tree mappings Eric Auger

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a0e88a60-083b-2730-5db8-7bba0299974b@redhat.com \
    --to=eric.auger@redhat.com \
    --cc=alex.williamson@redhat.com \
    --cc=bharat.bhushan@nxp.com \
    --cc=eric.auger.pro@gmail.com \
    --cc=jean-philippe@linaro.org \
    --cc=kevin.tian@intel.com \
    --cc=mst@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=peterx@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=tn@semihalf.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox