Re: [PATCH v13 07/10] virtio-iommu-pci: Add virtio iommu pci support

[Michael S. Tsirkin]

On Mon, Feb 03, 2020 at 02:38:22PM +0100, Auger Eric wrote:
> Hi Michael,
> 
> On 2/3/20 2:28 PM, Michael S. Tsirkin wrote:
> > On Mon, Feb 03, 2020 at 02:20:55PM +0100, Auger Eric wrote:
> >> Hi Michael,
> >>
> >> On 2/3/20 2:03 PM, Michael S. Tsirkin wrote:
> >>> On Sat, Jan 25, 2020 at 06:19:52PM +0100, 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>
> >>>> Reviewed-by: Jean-Philippe Brucker <address@hidden>
> >>>
> >>> I commented on v11 of this patch:
> >>>>> Could you send a smaller patchset without pci/acpi bits for now?
> >>> And you answered:
> >>>> Yes I am about to send v12.
> >>>
> >>> I guess this patch is here by mistake then?
> >>>
> >>> I think PCI devices should always have config space so guests are
> >>> not tempted to find work-arounds. Right?
> >> No it is not here by mistake. I removed everything related non DT
> >> integration as we discussed.
> >>
> >> DT support is fully upstream even for virtio-iommu-pci.
> >> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/virtio/iommu.txt
> >>
> >> So what's wrong implementing that at the moment. As we discussed we
> >> would use the PCIe config space integration for non DT.
> >>
> >> If I use the MMIO based device, I am forced to lock an MMIO region for
> >> it in the machvirt memory map:
> >> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/virtio/mmio.txt
> >>
> >> I guess Peter (Maydell) will not be happy with this situation either.
> >>
> >> Thanks
> >>
> >> Eric
> > 
> > I see. Can't we limit this to DT platforms for now then?
> That is the case. virtio-iommu does not work with ACPI.


Right. But nothing seems to prevent users from creating it
on systems without dt, and there won't be an easy way for
management to detect it when qemu added  this support down the road.



> For non DT the plan is to use what you suggested, ie. pass the binding
> info through the PCIe config space.
> 
> In that prospect I am waiting for Jean-Philippe's [RFC 00/13]
> virtio-iommu on non-devicetree platforms respin
> (https://lore.kernel.org/linux-iommu/address@hidden/T/)
> and especially patches 12 and 13 of the series, ie. binding info through
> the PCIe config space.
> 
> Thanks
> 
> Eric
> > 
> > 
> > 
> >>>
> >>>> ---
> >>>>
> >>>> v11 -> v12:
> >>>> - added Jean's R-b
> >>>> - remove the array of intervals. Will be introduced later?
> >>>>
> >>>> v10 -> v11:
> >>>> - add the reserved_regions array property
> >>>>
> >>>> v9 -> v10:
> >>>> - include "hw/qdev-properties.h" header
> >>>>
> >>>> 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 2fd9da7410..4e4d39a0a4 100644
> >>>> --- a/hw/virtio/Makefile.objs
> >>>> +++ b/hw/virtio/Makefile.objs
> >>>> @@ -29,6 +29,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..4cfae1f9df
> >>>> --- /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"
> >>>> +#include "hw/qdev-properties.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_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 2acd8321af..cfedf5a995 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 2a2c2ecf83..f39aa0fbb4 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 3465a1e2d0..97f4022b51 100644
> >>>> --- a/qdev-monitor.c
> >>>> +++ b/qdev-monitor.c
> >>>> @@ -66,6 +66,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
> >>>
> >>>
> >