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

[Eric Auger <eric.auger@linaro.org>]

Hi Peter,
On 02/16/2016 07:21 PM, Peter Maydell wrote:
> 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 :-)
Yes hopefully. The platform bus has its own MMIO allocation scheme.
> 
>> 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?
yes that's correct. currently the PCIe controller is the object that
tracks the reserved region's ref count. I proceeded that way because the
reserved IOVA provision currently is related to PCIe/MSI functionality.

 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.
OK

Another issue is that we currently book an arbitrary 1MB IOVA window
whatever the needs. We are also thinking about extending the VFIO user
API to return the number of reserved iova pages that are requested and
possibly some alignment constraints. Then it becomes more complex since
VFIO devices are instantiated after the machine creation, all the needs
must be collected and consolidated and eventually the reserved iova
region can be created. So I think that code will end up somewhere in a
machine init done notifier ...

Thanks for the review!

Best Regards

Eric
> 
> thanks
> -- PMM
>