Re: [Qemu-devel] [PATCH v7 07/17] hw/arm/virt: Dynamic memory map depending on RAM requirements

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 22 Feb 2019 15:06:14 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Igor,
> 
> On 2/22/19 1:57 PM, Igor Mammedov wrote:
> > On Wed, 20 Feb 2019 23:39:53 +0100
> > Eric Auger <eric.auger@redhat.com> wrote:
> >   
> >> Up to now the memory map has been static and the high IO region
> >> base has always been 256GiB.
> >>
> >> This patch modifies the virt_set_memmap() function, which freezes
> >> the memory map, so that the high IO range base becomes floating,
> >> located after the initial RAM and the device memory.
> >>
> >> The function computes
> >> - the base of the device memory,
> >> - the size of the device memory and
> >> - the highest GPA used in the memory map.
> >>
> >> The two former will be used when defining the device memory region
> >> while the latter will be used at VM creation to choose the requested
> >> IPA size.
> >>
> >> Setting all the existing highmem IO regions beyond the RAM
> >> allows to have a single contiguous RAM region (initial RAM and
> >> possible hotpluggable device memory). That way we do not need
> >> to do invasive changes in the EDK2 FW to support a dynamic
> >> RAM base.
> >>
> >> Still the user cannot request an initial RAM size greater than 255GB.
> >> Also we handle the case where maxmem or slots options are passed,
> >> although no device memory is usable at the moment. In this case, we
> >> just ignore those settings.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> ---
> >>  hw/arm/virt.c         | 47 ++++++++++++++++++++++++++++++++++---------
> >>  include/hw/arm/virt.h |  3 +++
> >>  2 files changed, 41 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index 12039a0367..9db602457b 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -107,8 +107,9 @@
> >>   * of a terabyte of RAM will be doing it on a host with more than a
> >>   * terabyte of physical address space.)
> >>   */
> >> -#define RAMLIMIT_GB 255
> >> -#define RAMLIMIT_BYTES (RAMLIMIT_GB * 1024ULL * 1024 * 1024)
> >> +#define RAMBASE GiB
> >> +#define LEGACY_RAMLIMIT_GB 255
> >> +#define LEGACY_RAMLIMIT_BYTES (LEGACY_RAMLIMIT_GB * GiB)
> >>  
> >>  /* Addresses and sizes of our components.
> >>   * 0..128MB is space for a flash device so we can run bootrom code such as UEFI.
> >> @@ -149,7 +150,7 @@ static const MemMapEntry base_memmap[] = {
> >>      [VIRT_PCIE_MMIO] =          { 0x10000000, 0x2eff0000 },
> >>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
> >>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
> >> -    [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
> >> +    [VIRT_MEM] =                { RAMBASE, LEGACY_RAMLIMIT_BYTES },
> >>  };
> >>  
> >>  /*
> >> @@ -1367,16 +1368,48 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
> >>  
> >>  static void virt_set_memmap(VirtMachineState *vms)
> >>  {
> >> +    MachineState *ms = MACHINE(vms);
> >>      hwaddr base;
> >>      int i;
> >>  
> >> +    if (ms->maxram_size > ms->ram_size || ms->ram_slots > 0) {
> >> +        error_report("mach-virt: does not support device memory: "
> >> +                     "ignore maxmem and slots options");
> >> +        ms->maxram_size = ms->ram_size;
> >> +        ms->ram_slots = 0;
> >> +    }
> >> +    if (ms->ram_size > (ram_addr_t)LEGACY_RAMLIMIT_BYTES) {
> >> +        error_report("mach-virt: cannot model more than %dGB RAM",
> >> +                     LEGACY_RAMLIMIT_GB);
> >> +        exit(1);
> >> +    }  
> > I'd drop these checks and amend below code so that it would init device_memory
> > logic when ram_slots > 0. It should simplify follow up patches by dropping
> > all machine version specific parts.  
> I don't have sufficient knowledge of virtio-mem/virtio-pmem. Do they
> also use slots?
if they don't then they will have to take care of it for every machine
anyway so I'd dismiss that from consideration.

> 
> 
> > It shouldn't break old machines as layout stays the same but would allow to
> > start old machine with pc-dimms which is fine from migration pov as target
> > also should be able to start QEMU with the same options for migration to start.
> > 
> >   
> >> +
> >>      vms->memmap = extended_memmap;
> >>  
> >>      for (i = 0; i < ARRAY_SIZE(base_memmap); i++) {
> >>          vms->memmap[i] = base_memmap[i];
> >>      }
> >>  
> >> -    vms->high_io_base = 256 * GiB; /* Top of the legacy initial RAM region */
> >> +    /*
> >> +     * We compute the base of the high IO region depending on the
> >> +     * amount of initial and device memory. The device memory start/size
> >> +     * is aligned on 1GiB. We never put the high IO region below 256GiB
> >> +     * so that if maxram_size is < 255GiB we keep the legacy memory map.
> >> +     * The device region size assumes 1GiB page max alignment per slot.
> >> +     */
> >> +    vms->device_memory_base = ROUND_UP(RAMBASE + ms->ram_size, GiB);
> >> +    vms->device_memory_size = ms->maxram_size - ms->ram_size +
> >> +                              ms->ram_slots * GiB;  
> So does everyone agree on this device memory size computation? I would
> like to make sure this is future proof. Do I need to add a machine
> option like on x86 to enforce slot alignment or is it OK?
computation looks fine to me,
Though, I'd try not duplicate data in vms->device_memory_base and vms->device_memory_size
as vms->device_memory is sufficient, see comment in 13/17

> 
> Thanks
> 
> Eric
> >> +
> >> +    vms->high_io_base = vms->device_memory_base +
> >> +                        ROUND_UP(vms->device_memory_size, GiB);
> >> +    if (vms->high_io_base < vms->device_memory_base) {
> >> +        error_report("maxmem/slots too huge");
> >> +        exit(EXIT_FAILURE);
> >> +    }
> >> +    if (vms->high_io_base < 256 * GiB) {
> >> +        vms->high_io_base = 256 * GiB;
> >> +    }
> >>      base = vms->high_io_base;
> >>  
> >>      for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
> >> @@ -1387,6 +1420,7 @@ static void virt_set_memmap(VirtMachineState *vms)
> >>          vms->memmap[i].size = size;
> >>          base += size;
> >>      }
> >> +    vms->highest_gpa = base - 1;
> >>  }
> >>  
> >>  static void machvirt_init(MachineState *machine)
> >> @@ -1470,11 +1504,6 @@ static void machvirt_init(MachineState *machine)
> >>  
> >>      vms->smp_cpus = smp_cpus;
> >>  
> >> -    if (machine->ram_size > vms->memmap[VIRT_MEM].size) {
> >> -        error_report("mach-virt: cannot model more than %dGB RAM", RAMLIMIT_GB);
> >> -        exit(1);
> >> -    }
> >> -
> >>      if (vms->virt && kvm_enabled()) {
> >>          error_report("mach-virt: KVM does not support providing "
> >>                       "Virtualization extensions to the guest CPU");
> >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> >> index 3dc7a6c5d5..acad0400d8 100644
> >> --- a/include/hw/arm/virt.h
> >> +++ b/include/hw/arm/virt.h
> >> @@ -132,6 +132,9 @@ typedef struct {
> >>      uint32_t iommu_phandle;
> >>      int psci_conduit;
> >>      hwaddr high_io_base;
> >> +    hwaddr highest_gpa;
> >> +    hwaddr device_memory_base;
> >> +    hwaddr device_memory_size;
> >>  } VirtMachineState;
> >>  
> >>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)  
> >