Re: [Qemu-devel] [RFC v2 0/6] hw/arm: Add support for non-contiguous iova regions

[Shameerali Kolothum Thodi]

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:address@hidden
> Sent: Monday, May 28, 2018 3:22 PM
> To: Shameerali Kolothum Thodi <address@hidden>;
> address@hidden; address@hidden
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; Zhaoshenglong <address@hidden>;
> Jonathan Cameron <address@hidden>; Linuxarm
> <address@hidden>
> Subject: Re: [RFC v2 0/6] hw/arm: Add support for non-contiguous iova regions
> 
> Hi Shameer,
> 
> On 05/16/2018 05:20 PM, Shameer Kolothum wrote:
> > When the kernel reports valid iova ranges as non-contiguous,
> > memory should be allocated to Guest in such a way that
> > reserved regions(holes) are not visible by Guest.
> >
> > This series retrieves the valid iova ranges based on the new
> > proposed VFIO interface for kernel [1]. It then populates the
> > first 1GB ram as a non-pluggable dimm and rest as a pc-dimm if
> > the valid iova ranges are non-contiguous.
> 
> Some general comments:

Thanks for going through this series.

> - what are your plans with respect to VFIO device hot-plug handling?
> IIUC, at the moment, any collision between reserved regions induces by
> hot-plugged devices are not detected/handled. I understand mem hotplug
> is not supported on aarch64. Would you simply reject the vfio device
> hotplug.

As per the new kernel changes, the _attach_group() will fail if the device 
reserved regions conflicts with any existing dma mappings of the VM. 
So my understanding is that, host kernel will in effect reject any hot-plug
device that has a reserved region conflicting with dma mappings.

> - IIUC any reserved window colliding with [4000000, 1GB] cold-plug RAM
> segment is show-stopper. How was this 1GB size chosen exactly?

Yes, any reserved window in that region might prevent the Guest from booting
with UEFI as the current solution will move the base start address . I think 
this is
because the EDK2 UEFI expects the base to start at 0x4000000[1]. I am not sure
why this is required by the UEFI as without the "-bios" option, the Guest can be
booted as long as base addr < 4GB.

> - Currently you create a single PC-DIMM node whereas several ones may be
> possible & necessary? Do you plan to support multiple PC-DIMMS node?

Yes, that is correct. This is mainly to keep it simple and with the expectation 
that
the valid host iova regions may not be that fragmented. I can look into extent 
this
to support multiple pc-dimms if there is an immediate requirement to do so.
 
> - I have started looking at RAM extension on machvirt. I think adding
> support of PC-DIMMS through the qemu cmd line is something that we need
> to work on, in paralell.

Ok. Do you have more info that you can share on this? Please let me know.

Thanks,
Shameer

[1] 
https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/ArmVirtQemu.dsc#L133

> Thanks
> 
> Eric
> >
> > Patch #3 of this series is loosely based on an earlier attempt
> > by Kwangwoo Lee to add hotplug/pc-dimm support to arm64[2]
> >
> > RFC v1[3] --> RFCv2
> >  -Based on new VFIO kernel interface
> >  -Part of Mem modelled as pc-dimm
> >  -Rebased to qemu 2.12.0
> >
> > [1] https://lkml.org/lkml/2018/4/18/293
> > [2] https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04600.html
> > [3] https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg02412.html
> >
> > Shameer Kolothum (6):
> >   hw/vfio: Retrieve valid iova ranges from kernel
> >   hw/arm/virt: Enable dynamic generation of guest RAM memory regions
> >   hw/arm/virt: Add pc-dimm mem hotplug framework
> >   hw/arm: Changes required to accommodate non-contiguous DT mem nodes
> >   hw/arm: ACPI SRAT changes to accommodate non-contiguous mem
> >   hw/arm: Populate non-contiguous memory regions
> >
> >  default-configs/aarch64-softmmu.mak |   1 +
> >  hw/arm/boot.c                       |  91 ++++++---
> >  hw/arm/virt-acpi-build.c            |  24 ++-
> >  hw/arm/virt.c                       | 367
> +++++++++++++++++++++++++++++++++++-
> >  hw/vfio/common.c                    | 108 ++++++++++-
> >  include/hw/arm/arm.h                |  12 ++
> >  include/hw/arm/virt.h               |   3 +
> >  include/hw/vfio/vfio-common.h       |   7 +
> >  linux-headers/linux/vfio.h          |  23 +++
> >  9 files changed, 589 insertions(+), 47 deletions(-)
> >