| Message ID | 20200208120022.1920-9-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU device | expand |
On Sat, 8 Feb 2020 at 12:01, Eric Auger <eric.auger@redhat.com> wrote: > > Adds the "virtio,pci-iommu" node in the host bridge node and > the RID mapping, excluding the IOMMU RID. > > This is done in the virtio-iommu-pci hotplug handler which > gets called only if no firmware is loaded or if -no-acpi is > passed on the command line. As non DT integration is > not yet supported by the kernel we must make sure we > are in DT mode. This limitation will be removed as soon > as the topology description feature gets supported. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > +static void create_virtio_iommu(VirtMachineState *vms, Error **errp) > +{ > + const char compat[] = "virtio,pci-iommu"; > + uint16_t bdf = vms->virtio_iommu_bdf; > + char *node; > + > + vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt); > + > + node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf); > + qemu_fdt_add_subnode(vms->fdt, node); > + qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat)); > + qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg", > + 1, bdf << 8, 1, 0, 1, 0, > + 1, 0, 1, 0); > + > + qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1); > + qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle); > + g_free(node); > + > + qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-map", > + 0x0, vms->iommu_phandle, 0x0, bdf, > + bdf + 1, vms->iommu_phandle, bdf + 1, 0xffff - bdf); > +} This function name implies that we're creating the IOMMU device here (which would be a weird thing to do in a hotplug callback for some other device), but it looks like we're only adding device tree nodes ? Given that we write the FDT blob into the guest RAM on bootup, how does making changes to it here on hotplug (which I assume to be 'after boot, whenever the user hot-plugs something') work? thanks -- PMM
Hi Peter, On 2/11/20 4:00 PM, Peter Maydell wrote: > On Sat, 8 Feb 2020 at 12:01, Eric Auger <eric.auger@redhat.com> wrote: >> >> Adds the "virtio,pci-iommu" node in the host bridge node and >> the RID mapping, excluding the IOMMU RID. >> >> This is done in the virtio-iommu-pci hotplug handler which >> gets called only if no firmware is loaded or if -no-acpi is >> passed on the command line. As non DT integration is >> not yet supported by the kernel we must make sure we >> are in DT mode. This limitation will be removed as soon >> as the topology description feature gets supported. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> +static void create_virtio_iommu(VirtMachineState *vms, Error **errp) >> +{ >> + const char compat[] = "virtio,pci-iommu"; >> + uint16_t bdf = vms->virtio_iommu_bdf; >> + char *node; >> + >> + vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt); >> + >> + node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf); >> + qemu_fdt_add_subnode(vms->fdt, node); >> + qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat)); >> + qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg", >> + 1, bdf << 8, 1, 0, 1, 0, >> + 1, 0, 1, 0); >> + >> + qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1); >> + qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle); >> + g_free(node); >> + >> + qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-map", >> + 0x0, vms->iommu_phandle, 0x0, bdf, >> + bdf + 1, vms->iommu_phandle, bdf + 1, 0xffff - bdf); >> +} > > This function name implies that we're creating the IOMMU device > here (which would be a weird thing to do in a hotplug callback > for some other device), but it looks like we're only adding > device tree nodes ? yes the actual iommu device is created through the -device option. I can rename into create_iommu_dt_bindings > > Given that we write the FDT blob into the guest RAM on bootup, > how does making changes to it here on hotplug (which I assume > to be 'after boot, whenever the user hot-plugs something') work? the virtio-iommu is not supposed to be hotplugged but rather cold-plugged. I use this hotplug mechanism to detect its presence and add the related dt mappings. Maybe I can add a check to detect if the bootup is over? Thoughts? Eric > > thanks > -- PMM >
Hi Peter, Michael, On 2/11/20 6:31 PM, Auger Eric wrote: > Hi Peter, > > On 2/11/20 4:00 PM, Peter Maydell wrote: >> On Sat, 8 Feb 2020 at 12:01, Eric Auger <eric.auger@redhat.com> wrote: >>> >>> Adds the "virtio,pci-iommu" node in the host bridge node and >>> the RID mapping, excluding the IOMMU RID. >>> >>> This is done in the virtio-iommu-pci hotplug handler which >>> gets called only if no firmware is loaded or if -no-acpi is >>> passed on the command line. As non DT integration is >>> not yet supported by the kernel we must make sure we >>> are in DT mode. This limitation will be removed as soon >>> as the topology description feature gets supported. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> >>> +static void create_virtio_iommu(VirtMachineState *vms, Error **errp) >>> +{ >>> + const char compat[] = "virtio,pci-iommu"; >>> + uint16_t bdf = vms->virtio_iommu_bdf; >>> + char *node; >>> + >>> + vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt); >>> + >>> + node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf); >>> + qemu_fdt_add_subnode(vms->fdt, node); >>> + qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat)); >>> + qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg", >>> + 1, bdf << 8, 1, 0, 1, 0, >>> + 1, 0, 1, 0); >>> + >>> + qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1); >>> + qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle); >>> + g_free(node); >>> + >>> + qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-map", >>> + 0x0, vms->iommu_phandle, 0x0, bdf, >>> + bdf + 1, vms->iommu_phandle, bdf + 1, 0xffff - bdf); >>> +} >> >> This function name implies that we're creating the IOMMU device >> here (which would be a weird thing to do in a hotplug callback >> for some other device), but it looks like we're only adding >> device tree nodes ? > yes the actual iommu device is created through the -device option. I can > rename into create_iommu_dt_bindings So I renamed it into create_virtio_iommu_dt_bindings() >> >> Given that we write the FDT blob into the guest RAM on bootup, >> how does making changes to it here on hotplug (which I assume >> to be 'after boot, whenever the user hot-plugs something') work? > > the virtio-iommu is not supposed to be hotplugged but rather > cold-plugged. I use this hotplug mechanism to detect its presence and > add the related dt mappings. Maybe I can add a check to detect if the > bootup is over? I added in virtio-iommu-pci virtio_iommu_pci_class_init() dc->hotpluggable = false; As far as I understand this makes the virtio-iommu-pci device not hotpluggable (same is used for intel-iommu): (QEMU) device_add id=hot0 driver=virtio-iommu-pci bus=head.0 addr=4 {"error": {"class": "GenericError", "desc": "Parameter 'driver' expects pluggable device type"}} Is that OK? Thanks Eric > > Thoughts? > > Eric >> >> thanks >> -- PMM >> > >
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index f788fe27d6..784e70c89d 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -32,6 +32,7 @@ #include "qemu-common.h" #include "qemu/units.h" #include "qemu/option.h" +#include "monitor/qdev.h" #include "qapi/error.h" #include "hw/sysbus.h" #include "hw/boards.h" @@ -54,6 +55,7 @@ #include "qemu/error-report.h" #include "qemu/module.h" #include "hw/pci-host/gpex.h" +#include "hw/virtio/virtio-pci.h" #include "hw/arm/sysbus-fdt.h" #include "hw/platform-bus.h" #include "hw/qdev-properties.h" @@ -71,6 +73,7 @@ #include "hw/mem/pc-dimm.h" #include "hw/mem/nvdimm.h" #include "hw/acpi/generic_event_device.h" +#include "hw/virtio/virtio-iommu.h" #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ @@ -1180,6 +1183,30 @@ static void create_smmu(const VirtMachineState *vms, g_free(node); } +static void create_virtio_iommu(VirtMachineState *vms, Error **errp) +{ + const char compat[] = "virtio,pci-iommu"; + uint16_t bdf = vms->virtio_iommu_bdf; + char *node; + + vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt); + + node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf); + qemu_fdt_add_subnode(vms->fdt, node); + qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat)); + qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg", + 1, bdf << 8, 1, 0, 1, 0, + 1, 0, 1, 0); + + qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1); + qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle); + g_free(node); + + qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-map", + 0x0, vms->iommu_phandle, 0x0, bdf, + bdf + 1, vms->iommu_phandle, bdf + 1, 0xffff - bdf); +} + static void create_pcie(VirtMachineState *vms) { hwaddr base_mmio = vms->memmap[VIRT_PCIE_MMIO].base; @@ -1258,7 +1285,7 @@ static void create_pcie(VirtMachineState *vms) } } - nodename = g_strdup_printf("/pcie@%" PRIx64, base); + nodename = vms->pciehb_nodename = g_strdup_printf("/pcie@%" PRIx64, base); qemu_fdt_add_subnode(vms->fdt, nodename); qemu_fdt_setprop_string(vms->fdt, nodename, "compatible", "pci-host-ecam-generic"); @@ -1301,13 +1328,16 @@ static void create_pcie(VirtMachineState *vms) if (vms->iommu) { vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt); - create_smmu(vms, pci->bus); - - qemu_fdt_setprop_cells(vms->fdt, nodename, "iommu-map", - 0x0, vms->iommu_phandle, 0x0, 0x10000); + switch (vms->iommu) { + case VIRT_IOMMU_SMMUV3: + create_smmu(vms, pci->bus); + qemu_fdt_setprop_cells(vms->fdt, nodename, "iommu-map", + 0x0, vms->iommu_phandle, 0x0, 0x10000); + break; + default: + g_assert_not_reached(); + } } - - g_free(nodename); } static void create_platform_bus(VirtMachineState *vms) @@ -1976,6 +2006,13 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev, if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { virt_memory_plug(hotplug_dev, dev, errp); } + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { + PCIDevice *pdev = PCI_DEVICE(dev); + + vms->iommu = VIRT_IOMMU_VIRTIO; + vms->virtio_iommu_bdf = pci_get_bdf(pdev); + create_virtio_iommu(vms, errp); + } } static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev, @@ -1992,7 +2029,13 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine, (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) { return HOTPLUG_HANDLER(machine); } + if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { + VirtMachineState *vms = VIRT_MACHINE(machine); + if (!vms->bootinfo.firmware_loaded || !acpi_enabled) { + return HOTPLUG_HANDLER(machine); + } + } return NULL; } diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index 71508bf40c..02f500cb8e 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -125,8 +125,10 @@ typedef struct { bool virt; int32_t gic_version; VirtIOMMUType iommu; + uint16_t virtio_iommu_bdf; struct arm_boot_info bootinfo; MemMapEntry *memmap; + char *pciehb_nodename; const int *irqmap; int smp_cpus; void *fdt;
Adds the "virtio,pci-iommu" node in the host bridge node and the RID mapping, excluding the IOMMU RID. This is done in the virtio-iommu-pci hotplug handler which gets called only if no firmware is loaded or if -no-acpi is passed on the command line. As non DT integration is not yet supported by the kernel we must make sure we are in DT mode. This limitation will be removed as soon as the topology description feature gets supported. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v14 -> v15: - only support the hotplug handler for virtio-iommu-pci if dt can be guaranteed v11 -> v12: - Added Jean's R-b v10 -> v11: - remove msi_bypass v8 -> v9: - disable msi-bypass property - addition of the subnode is handled is the hotplug handler and IOMMU RID is notimposed anymore v6 -> v7: - align to the smmu instantiation code v4 -> v5: - VirtMachineClass no_iommu added in this patch - Use object_resolve_path_type --- hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++------ include/hw/arm/virt.h | 2 ++ 2 files changed, 52 insertions(+), 7 deletions(-)