| Message ID | 20200125171955.12825-8-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU device | expand |
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 <eric.auger@redhat.com> > Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> 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? > --- > > 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
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 <eric.auger@redhat.com> >> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > 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 > >> --- >> >> 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 > >
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 <eric.auger@redhat.com> > >> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > > > 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? > > > >> --- > >> > >> 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 > > > >
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 <eric.auger@redhat.com> >>>> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> >>> >>> 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. 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/20191203190136.00007171@intel.com/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 >>> >>> >
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 <eric.auger@redhat.com> > >>>> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > >>> > >>> 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/20191203190136.00007171@intel.com/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 > >>> > >>> > >
Hi Michael, On 2/3/20 2:46 PM, Michael S. Tsirkin wrote: > 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 <eric.auger@redhat.com> >>>>>> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> >>>>> >>>>> 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. OK I will add add a check Thanks Eric > > > >> 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/20191203190136.00007171@intel.com/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 >>>>> >>>>> >>> > >
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 },