Re: [Qemu-arm] [RFC v2 7/8] hw: arm: virt: register reserved IOVA region

[Peter Maydell <peter.maydell@linaro.org>]

On 29 January 2016 at 16:53, Eric Auger <eric.auger@linaro.org> wrote:
> Registers a 16x64kB reserved iova region. Currently this iova
> region is used by the host kernel to map host MSI controller frames
> (GICv2m, GITS_TRANSLATER). The host kernel needs this iova window
> since it cannot program the PCIe device with MSI frame physical
> address (as opposed to x86) since the MSI write transactions go
> through the IOMMU.
>
> The reserved region is mapped on the platform bus.

I guess that keeps it neatly out of the way of everybody else :-)

> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> RFC v1 -> RFC v2:
> - use the platform bus to map the reserved iova region
> ---
>  hw/arm/virt.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 3839c68..4b2a891 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -805,7 +805,7 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle,
>  }
>
>  static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
> -                        bool use_highmem)
> +                        bool use_highmem, MemoryRegion **reserved_reg)
>  {
>      hwaddr base_mmio = vbi->memmap[VIRT_PCIE_MMIO].base;
>      hwaddr size_mmio = vbi->memmap[VIRT_PCIE_MMIO].size;
> @@ -920,10 +920,16 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
>      create_pcie_irq_map(vbi, vbi->gic_phandle, irq, nodename);
>
> +    /* initialize the reserved iova region for MSI binding (16 x 64kb) */
> +    *reserved_reg = g_new0(MemoryRegion, 1);
> +    memory_region_init_reserved_iova(*reserved_reg, OBJECT(dev),
> +                                     "reserved-iova",
> +                                     0x100000, &error_fatal);

So the only reason this is here is because we need to have a pointer to
the PCIe controller DeviceState, right? I think it would be better to
make create_pcie() return the DeviceState* instead of NULL. Then you
can either (a) pass the pcie controller pointer into create_platform_bus()
and have that create and map the reserved iova region, or (b) have a
separate function to create the reserved iova region. In any case I
think it fits more naturally with the rest of the platform bus code
rather than in the PCIe controller creation function.

thanks
-- PMM