| Message ID | 20200703165405.17672-9-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/34] Add a phy-num property to the i.MX FEC emulator | expand |
Hi Eric, On 3/7/20 17:53, Peter Maydell wrote: > From: Eric Auger <eric.auger@redhat.com> > > At the moment the virtio-iommu translates MSI transactions. > This behavior is inherited from ARM SMMU. The virt machine > code knows where the guest MSI doorbells are so we can easily > declare those regions as VIRTIO_IOMMU_RESV_MEM_T_MSI. With that > setting the guest will not map MSIs through the IOMMU and those > transactions will be simply bypassed. > > Depending on which MSI controller is in use (ITS or GICV2M), > we declare either: > - the ITS interrupt translation space (ITS_base + 0x10000), > containing the GITS_TRANSLATOR or > - The GICV2M single frame, containing the MSI_SETSP_NS register. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Message-id: 20200629070404.10969-6-eric.auger@redhat.com > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/arm/virt.h | 7 +++++++ > hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 37 insertions(+) > static void create_gic(VirtMachineState *vms) > @@ -2198,8 +2200,36 @@ out: > static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); > + > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > virt_memory_pre_plug(hotplug_dev, dev, errp); > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { > + hwaddr db_start = 0, db_end = 0; > + char *resv_prop_str; > + > + switch (vms->msi_controller) { > + case VIRT_MSI_CTRL_NONE: > + return; > + case VIRT_MSI_CTRL_ITS: > + /* GITS_TRANSLATER page */ > + db_start = base_memmap[VIRT_GIC_ITS].base + 0x10000; > + db_end = base_memmap[VIRT_GIC_ITS].base + > + base_memmap[VIRT_GIC_ITS].size - 1; > + break; > + case VIRT_MSI_CTRL_GICV2M: > + /* MSI_SETSPI_NS page */ > + db_start = base_memmap[VIRT_GIC_V2M].base; > + db_end = db_start + base_memmap[VIRT_GIC_V2M].size - 1; > + break; > + } > + resv_prop_str = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u", > + db_start, db_end, > + VIRTIO_IOMMU_RESV_MEM_T_MSI); > + > + qdev_prop_set_uint32(dev, "len-reserved-regions", 1); Where is "len-reserved-regions" declared? Since qdev_prop_set_uint32() uses &error_abort, isn't this call aborting the process? I am confused how this code path is exercised, what am I missing? > + qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str); > + g_free(resv_prop_str); > } > } >
On 2/2/23 11:47, Philippe Mathieu-Daudé wrote: > Hi Eric, > > On 3/7/20 17:53, Peter Maydell wrote: >> From: Eric Auger <eric.auger@redhat.com> >> >> At the moment the virtio-iommu translates MSI transactions. >> This behavior is inherited from ARM SMMU. The virt machine >> code knows where the guest MSI doorbells are so we can easily >> declare those regions as VIRTIO_IOMMU_RESV_MEM_T_MSI. With that >> setting the guest will not map MSIs through the IOMMU and those >> transactions will be simply bypassed. >> >> Depending on which MSI controller is in use (ITS or GICV2M), >> we declare either: >> - the ITS interrupt translation space (ITS_base + 0x10000), >> containing the GITS_TRANSLATOR or >> - The GICV2M single frame, containing the MSI_SETSP_NS register. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Message-id: 20200629070404.10969-6-eric.auger@redhat.com >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> include/hw/arm/virt.h | 7 +++++++ >> hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+) > > >> static void create_gic(VirtMachineState *vms) >> @@ -2198,8 +2200,36 @@ out: >> static void virt_machine_device_pre_plug_cb(HotplugHandler >> *hotplug_dev, >> DeviceState *dev, Error >> **errp) >> { >> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); >> + >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { >> virt_memory_pre_plug(hotplug_dev, dev, errp); >> + } else if (object_dynamic_cast(OBJECT(dev), >> TYPE_VIRTIO_IOMMU_PCI)) { >> + hwaddr db_start = 0, db_end = 0; >> + char *resv_prop_str; >> + >> + switch (vms->msi_controller) { >> + case VIRT_MSI_CTRL_NONE: >> + return; >> + case VIRT_MSI_CTRL_ITS: >> + /* GITS_TRANSLATER page */ >> + db_start = base_memmap[VIRT_GIC_ITS].base + 0x10000; >> + db_end = base_memmap[VIRT_GIC_ITS].base + >> + base_memmap[VIRT_GIC_ITS].size - 1; >> + break; >> + case VIRT_MSI_CTRL_GICV2M: >> + /* MSI_SETSPI_NS page */ >> + db_start = base_memmap[VIRT_GIC_V2M].base; >> + db_end = db_start + base_memmap[VIRT_GIC_V2M].size - 1; >> + break; >> + } >> + resv_prop_str = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u", >> + db_start, db_end, >> + VIRTIO_IOMMU_RESV_MEM_T_MSI); >> + >> + qdev_prop_set_uint32(dev, "len-reserved-regions", 1); > > Where is "len-reserved-regions" declared? > > Since qdev_prop_set_uint32() uses &error_abort, isn't this call > aborting the process? I am confused how this code path is exercised, > what am I missing? The call path is: qdev_prop_set_uint32 -> object_property_set_int -> object_property_set_qobject -> object_property_set -> object_property_find_err So QEMU should abort displaying: "Property 'virtio-iommu-pci.len-reserved-regions' not found". >> + qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str); >> + g_free(resv_prop_str); >> } >> }
On Thu, 2 Feb 2023 at 10:47, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> Where is "len-reserved-regions" declared?
DEFINE_PROP_ARRAY("reserved-regions", ...)
does this. For an array property "foo" the machinery creates an integer
property "foo-len", which must be set first. Setting that
then creates properties "foo[0]", "foo[1]", ... which can be set.
thanks
-- PMM
On 2/2/23 11:58, Peter Maydell wrote: > On Thu, 2 Feb 2023 at 10:47, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> Where is "len-reserved-regions" declared? > > DEFINE_PROP_ARRAY("reserved-regions", ...) > > does this. For an array property "foo" the machinery creates an integer > property "foo-len", which must be set first. Setting that > then creates properties "foo[0]", "foo[1]", ... which can be set. Oh indeed now I see the array prefix: #define PROP_ARRAY_LEN_PREFIX "len-" Not obvious to realize while grepping. Thanks for the quick answer!
Hi, On 2/2/23 11:58, Peter Maydell wrote: > On Thu, 2 Feb 2023 at 10:47, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> Where is "len-reserved-regions" declared? > DEFINE_PROP_ARRAY("reserved-regions", ...) > > does this. For an array property "foo" the machinery creates an integer > property "foo-len", which must be set first. Setting that > then creates properties "foo[0]", "foo[1]", ... which can be set. Yes. Thank you Peter! Eric > > thanks > -- PMM >
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index 31878ddc722..a18b6b397b6 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -96,6 +96,12 @@ typedef enum VirtIOMMUType { VIRT_IOMMU_VIRTIO, } VirtIOMMUType; +typedef enum VirtMSIControllerType { + VIRT_MSI_CTRL_NONE, + VIRT_MSI_CTRL_GICV2M, + VIRT_MSI_CTRL_ITS, +} VirtMSIControllerType; + typedef enum VirtGICType { VIRT_GIC_VERSION_MAX, VIRT_GIC_VERSION_HOST, @@ -136,6 +142,7 @@ typedef struct { OnOffAuto acpi; VirtGICType gic_version; VirtIOMMUType iommu; + VirtMSIControllerType msi_controller; uint16_t virtio_iommu_bdf; struct arm_boot_info bootinfo; MemMapEntry *memmap; diff --git a/hw/arm/virt.c b/hw/arm/virt.c index af3050bc4bd..7922f3c23a5 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -600,6 +600,7 @@ static void create_its(VirtMachineState *vms) sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_ITS].base); fdt_add_its_gic_node(vms); + vms->msi_controller = VIRT_MSI_CTRL_ITS; } static void create_v2m(VirtMachineState *vms) @@ -620,6 +621,7 @@ static void create_v2m(VirtMachineState *vms) } fdt_add_v2m_gic_node(vms); + vms->msi_controller = VIRT_MSI_CTRL_GICV2M; } static void create_gic(VirtMachineState *vms) @@ -2198,8 +2200,36 @@ out: static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev); + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { virt_memory_pre_plug(hotplug_dev, dev, errp); + } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { + hwaddr db_start = 0, db_end = 0; + char *resv_prop_str; + + switch (vms->msi_controller) { + case VIRT_MSI_CTRL_NONE: + return; + case VIRT_MSI_CTRL_ITS: + /* GITS_TRANSLATER page */ + db_start = base_memmap[VIRT_GIC_ITS].base + 0x10000; + db_end = base_memmap[VIRT_GIC_ITS].base + + base_memmap[VIRT_GIC_ITS].size - 1; + break; + case VIRT_MSI_CTRL_GICV2M: + /* MSI_SETSPI_NS page */ + db_start = base_memmap[VIRT_GIC_V2M].base; + db_end = db_start + base_memmap[VIRT_GIC_V2M].size - 1; + break; + } + resv_prop_str = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u", + db_start, db_end, + VIRTIO_IOMMU_RESV_MEM_T_MSI); + + qdev_prop_set_uint32(dev, "len-reserved-regions", 1); + qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str); + g_free(resv_prop_str); } }