Re: [Qemu-devel] [PATCH 13/35] pc: initialize memory hotplug address space

[Hu Tao]

On Wed, Apr 16, 2014 at 04:23:44PM +0200, Igor Mammedov wrote:
> On Wed, 16 Apr 2014 16:59:25 +0800
> Hu Tao <address@hidden> wrote:
> 
> > On Fri, Apr 04, 2014 at 03:36:38PM +0200, Igor Mammedov wrote:
> > > initialize and map hotplug memory address space container
> > > into guest's RAM address space.
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > >  hw/i386/pc.c         | 19 +++++++++++++++++--
> > >  include/hw/i386/pc.h | 10 ++++++++++
> > >  2 files changed, 27 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 32b4003..69e4225 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1171,6 +1171,9 @@ FWCfgState *pc_memory_init(MemoryRegion 
> > > *system_memory,
> > >      MemoryRegion *ram, *option_rom_mr;
> > >      MemoryRegion *ram_below_4g, *ram_above_4g;
> > >      FWCfgState *fw_cfg;
> > > +    ram_addr_t ram_size = below_4g_mem_size + above_4g_mem_size;
> > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > +    PCMachineState *pcms = PC_MACHINE(machine);
> > >  
> > >      linux_boot = (kernel_filename != NULL);
> > >  
> > > @@ -1179,8 +1182,7 @@ FWCfgState *pc_memory_init(MemoryRegion 
> > > *system_memory,
> > >       * with older qemus that used qemu_ram_alloc().
> > >       */
> > >      ram = g_malloc(sizeof(*ram));
> > > -    memory_region_init_ram(ram, NULL, "pc.ram",
> > > -                           below_4g_mem_size + above_4g_mem_size);
> > > +    memory_region_init_ram(ram, NULL, "pc.ram", ram_size);
> > >      vmstate_register_ram_global(ram);
> > >      *ram_memory = ram;
> > >      ram_below_4g = g_malloc(sizeof(*ram_below_4g));
> > > @@ -1197,6 +1199,19 @@ FWCfgState *pc_memory_init(MemoryRegion 
> > > *system_memory,
> > >          e820_add_entry(0x100000000ULL, above_4g_mem_size, E820_RAM);
> > >      }
> > >  
> > > +    /* initialize hotplug memory address space */
> > > +    if (ram_size < machine->init_args.maxram_size) {
> > > +        ram_addr_t hotplug_mem_size =
> > > +            machine->init_args.maxram_size - ram_size;
> > > +
> > > +        pcms->hotplug_memory_base =
> > > +            ROUND_UP(0x100000000ULL + above_4g_mem_size, 1ULL << 30);
> > 
> > -m maxmem should be limited otherwise hotplug_memory_base + maxmem can
> > overflow(in dimm_get_free_addr()).
> If overflow happens than dimm_get_free_addr() will return error,
> if you look its end.
> 
> dimm_get_free_addr() {
> ...
>  if (new_start < address_space_start) {
>         error_setg(...
> 
> 
> 
> that of cause doesn't mean that maxmem limit shouldn't be set and verified
> with proper error reporting in case of one.

Yeah. With command line -object memory-ram,id=foo,size=512M -m 
512M,slots=4,maxmem=17179869183G
It reports an error "can't add memory [0x100000000:0x20000000] beyond 
0xa0000000" when hotplugging
memory-ram foo but shouldn't.

> Is there any suggestion as to what max supported RAM amount should be?

max ram shouldn't exceed unused memory range above 4g
(UINT64_MAX - 0x100000000 - above_4g_mem_size), that is:

maxram_size - ram_size <= UINT64_MAX - 0x100000000 - above_4g_mem_size

Following patch does the check:

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1329a50..f55e8c6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -68,6 +68,20 @@ static bool smbios_type1_defaults = true;
 static bool gigabyte_align = true;
 static bool has_reserved_memory = true;
 
+ram_addr_t get_above_4g_mem_size(ram_addr_t ram_size)
+{
+    ram_addr_t above_4g_mem_size;
+
+    if (ram_size >= 0xe0000000) {
+        ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
+        above_4g_mem_size = ram_size - lowmem;
+    } else {
+        above_4g_mem_size = 0;
+    }
+
+    return above_4g_mem_size;
+}
+
 /* PC hardware initialisation */
 static void pc_init1(QEMUMachineInitArgs *args,
                      int pci_enabled,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 88efdaa..b03fb64 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -273,6 +273,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory,
 
 /* pvpanic.c */
 uint16_t pvpanic_port(void);
+ram_addr_t get_above_4g_mem_size(ram_addr_t ram_size);
 
 /* e820 types */
 #define E820_RAM        1
diff --git a/vl.c b/vl.c
index 8f7b04e..095068e 100644
--- a/vl.c
+++ b/vl.c
@@ -3378,6 +3378,14 @@ int main(int argc, char **argv, char **envp)
                         exit(EXIT_FAILURE);
                     }
 
+                    ram_addr_t above_4g_mem = get_above_4g_mem_size(ram_size);
+                    if (sz - ram_size > UINT64_MAX - 0x100000000 - 
above_4g_mem) {
+                        fprintf(stderr, "qemu: invalid -m option value: maxmem 
"
+                                "(%" PRIu64 ") out of range.\n", sz);
+                        fprintf(stderr, "maxmem: %" PRIu64 "\n", UINT64_MAX - 
0x100000000 - above_4g_mem + ram_size);
+                        exit(EXIT_FAILURE);
+                    }
+
                     slots = qemu_opt_get_number(opts, "slots", 0);
                     if ((sz > ram_size) && !slots) {
                         fprintf(stderr, "qemu: invalid -m option value: maxmem 
"