diff mbox series

[PULL,006/136] vl.c: move -m parsing after memory backends has been processed

Message ID 1582631466-13880-6-git-send-email-pbonzini@redhat.com
State New
Headers show
Series None | expand

Commit Message

Paolo Bonzini Feb. 25, 2020, 11:48 a.m. UTC 
From: Igor Mammedov <imammedo@redhat.com>

It will be possible for main RAM to come from memory-backend
and we should check that size specified in -m matches the size
of the backend and [MachineState::]ram_size also matches
backend's size.

However -m parsing (set_memory_options()) happens before backends
are intialized (object_create_delayed()) which complicates it.
Consolidate set_memory_options() and assigning parsed results to
current_machine after backends are initialized, so it would be
possible access the initialized backend instance to compare
sizes.

This patch only consolidates scattered places touching ram_size
within vl.c. And follow up patch will integrate backend handling
to set_memory_options().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20200219160953.13771-7-imammedo@redhat.com>
---
 vl.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Comments

Eric Auger March 26, 2020, 9:20 a.m. UTC | #1 
Hi

On 2/25/20 12:48 PM, Paolo Bonzini wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> It will be possible for main RAM to come from memory-backend
> and we should check that size specified in -m matches the size
> of the backend and [MachineState::]ram_size also matches
> backend's size.
> 
> However -m parsing (set_memory_options()) happens before backends
> are intialized (object_create_delayed()) which complicates it.
> Consolidate set_memory_options() and assigning parsed results to
> current_machine after backends are initialized, so it would be
> possible access the initialized backend instance to compare
> sizes.
> 
> This patch only consolidates scattered places touching ram_size
> within vl.c. And follow up patch will integrate backend handling
> to set_memory_options().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Message-Id: <20200219160953.13771-7-imammedo@redhat.com>

Unfortunately this patch breaks ARM virt memory map setting in KVM mode.
If you launch ARM virt with PCDIMM you now get

qemu-system-aarch64: -device
pc-dimm,memdev=mem1,id=dimm1,driver=pc-dimm: memory devices (e.g. for
memory hotplug) are not supported by the machine

With machvirt/KVM
configure_accelerators() calls ARM virt_kvm_type(). This function
freezes the machine memory map and computes the size of the IPA to be
supported by KVM. See:
c9650222b8 ("hw/arm/virt: Implement kvm_type function for 4.0 machine")

virt_kvm_type() uses machine memory settings. Igor's patch moves those
after the mem backend init, so the virt memory map does not properly
take into account the machine memory settings anymore.

So we need to find a way to rework this.

Thanks

Eric


> ---
>  vl.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 2103804..72ffc06 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2655,6 +2655,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>          exit(EXIT_FAILURE);
>      }
>  
> +    if (!xen_enabled()) {
> +        /* On 32-bit hosts, QEMU is limited by virtual address space */
> +        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> +            error_report("at most 2047 MB RAM can be simulated");
> +            exit(1);
> +        }
> +    }
> +
>      loc_pop(&loc);
>  }
>  
> @@ -3819,8 +3827,6 @@ int main(int argc, char **argv, char **envp)
>      machine_class = select_machine();
>      object_set_machine_compat_props(machine_class->compat_props);
>  
> -    set_memory_options(&ram_slots, &maxram_size, machine_class);
> -
>      os_daemonize();
>      rcu_disable_atfork();
>  
> @@ -4122,9 +4128,6 @@ int main(int argc, char **argv, char **envp)
>      machine_opts = qemu_get_machine_opts();
>      qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
>                       &error_fatal);
> -    current_machine->ram_size = ram_size;
> -    current_machine->maxram_size = maxram_size;
> -    current_machine->ram_slots = ram_slots;
>  
>      /*
>       * Note: uses machine properties such as kernel-irqchip, must run
> @@ -4235,14 +4238,6 @@ int main(int argc, char **argv, char **envp)
>  
>      tpm_init();
>  
> -    if (!xen_enabled()) {
> -        /* On 32-bit hosts, QEMU is limited by virtual address space */
> -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> -            error_report("at most 2047 MB RAM can be simulated");
> -            exit(1);
> -        }
> -    }
> -
>      blk_mig_init();
>      ram_mig_init();
>      dirty_bitmap_mig_init();
> @@ -4287,6 +4282,12 @@ int main(int argc, char **argv, char **envp)
>      if (cpu_option) {
>          current_machine->cpu_type = parse_cpu_option(cpu_option);
>      }
> +
> +    set_memory_options(&ram_slots, &maxram_size, machine_class);
> +    current_machine->ram_size = ram_size;
> +    current_machine->maxram_size = maxram_size;
> +    current_machine->ram_slots = ram_slots;
> +
>      parse_numa_opts(current_machine);
>  
>      if (machine_class->default_ram_id && current_machine->ram_size &&
>
Igor Mammedov March 26, 2020, 10:43 a.m. UTC | #2 
On Thu, 26 Mar 2020 10:20:31 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi
> 
> On 2/25/20 12:48 PM, Paolo Bonzini wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > It will be possible for main RAM to come from memory-backend
> > and we should check that size specified in -m matches the size
> > of the backend and [MachineState::]ram_size also matches
> > backend's size.
> > 
> > However -m parsing (set_memory_options()) happens before backends
> > are intialized (object_create_delayed()) which complicates it.
> > Consolidate set_memory_options() and assigning parsed results to
> > current_machine after backends are initialized, so it would be
> > possible access the initialized backend instance to compare
> > sizes.
> > 
> > This patch only consolidates scattered places touching ram_size
> > within vl.c. And follow up patch will integrate backend handling
> > to set_memory_options().
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20200219160953.13771-7-imammedo@redhat.com>  
> 
> Unfortunately this patch breaks ARM virt memory map setting in KVM mode.
> If you launch ARM virt with PCDIMM you now get
> 
> qemu-system-aarch64: -device
> pc-dimm,memdev=mem1,id=dimm1,driver=pc-dimm: memory devices (e.g. for
> memory hotplug) are not supported by the machine
> 
> With machvirt/KVM
> configure_accelerators() calls ARM virt_kvm_type(). This function
> freezes the machine memory map and computes the size of the IPA to be
> supported by KVM. See:
> c9650222b8 ("hw/arm/virt: Implement kvm_type function for 4.0 machine")
> 
> virt_kvm_type() uses machine memory settings. Igor's patch moves those
> after the mem backend init, so the virt memory map does not properly
> take into account the machine memory settings anymore.
> 
> So we need to find a way to rework this.
set_memory_options() was already reverted, so  safest way to fix it during
freeze is to revert back lines

-    current_machine->ram_size = ram_size;
-    current_machine->maxram_size = maxram_size;
-    current_machine->ram_slots = ram_slots;

I'll post a patch

> 
> Thanks
> 
> Eric
> 
> 
> > ---
> >  vl.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 2103804..72ffc06 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2655,6 +2655,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
> >          exit(EXIT_FAILURE);
> >      }
> >  
> > +    if (!xen_enabled()) {
> > +        /* On 32-bit hosts, QEMU is limited by virtual address space */
> > +        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> > +            error_report("at most 2047 MB RAM can be simulated");
> > +            exit(1);
> > +        }
> > +    }
> > +
> >      loc_pop(&loc);
> >  }
> >  
> > @@ -3819,8 +3827,6 @@ int main(int argc, char **argv, char **envp)
> >      machine_class = select_machine();
> >      object_set_machine_compat_props(machine_class->compat_props);
> >  
> > -    set_memory_options(&ram_slots, &maxram_size, machine_class);
> > -
> >      os_daemonize();
> >      rcu_disable_atfork();
> >  
> > @@ -4122,9 +4128,6 @@ int main(int argc, char **argv, char **envp)
> >      machine_opts = qemu_get_machine_opts();
> >      qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
> >                       &error_fatal);
> > -    current_machine->ram_size = ram_size;
> > -    current_machine->maxram_size = maxram_size;
> > -    current_machine->ram_slots = ram_slots;
> >  
> >      /*
> >       * Note: uses machine properties such as kernel-irqchip, must run
> > @@ -4235,14 +4238,6 @@ int main(int argc, char **argv, char **envp)
> >  
> >      tpm_init();
> >  
> > -    if (!xen_enabled()) {
> > -        /* On 32-bit hosts, QEMU is limited by virtual address space */
> > -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> > -            error_report("at most 2047 MB RAM can be simulated");
> > -            exit(1);
> > -        }
> > -    }
> > -
> >      blk_mig_init();
> >      ram_mig_init();
> >      dirty_bitmap_mig_init();
> > @@ -4287,6 +4282,12 @@ int main(int argc, char **argv, char **envp)
> >      if (cpu_option) {
> >          current_machine->cpu_type = parse_cpu_option(cpu_option);
> >      }
> > +
> > +    set_memory_options(&ram_slots, &maxram_size, machine_class);
> > +    current_machine->ram_size = ram_size;
> > +    current_machine->maxram_size = maxram_size;
> > +    current_machine->ram_slots = ram_slots;
> > +
> >      parse_numa_opts(current_machine);
> >  
> >      if (machine_class->default_ram_id && current_machine->ram_size &&
> >
diff mbox series

Patch

diff --git a/vl.c b/vl.c
index 2103804..72ffc06 100644
--- a/vl.c
+++ b/vl.c
@@ -2655,6 +2655,14 @@  static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
         exit(EXIT_FAILURE);
     }
 
+    if (!xen_enabled()) {
+        /* On 32-bit hosts, QEMU is limited by virtual address space */
+        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
+            error_report("at most 2047 MB RAM can be simulated");
+            exit(1);
+        }
+    }
+
     loc_pop(&loc);
 }
 
@@ -3819,8 +3827,6 @@  int main(int argc, char **argv, char **envp)
     machine_class = select_machine();
     object_set_machine_compat_props(machine_class->compat_props);
 
-    set_memory_options(&ram_slots, &maxram_size, machine_class);
-
     os_daemonize();
     rcu_disable_atfork();
 
@@ -4122,9 +4128,6 @@  int main(int argc, char **argv, char **envp)
     machine_opts = qemu_get_machine_opts();
     qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
                      &error_fatal);
-    current_machine->ram_size = ram_size;
-    current_machine->maxram_size = maxram_size;
-    current_machine->ram_slots = ram_slots;
 
     /*
      * Note: uses machine properties such as kernel-irqchip, must run
@@ -4235,14 +4238,6 @@  int main(int argc, char **argv, char **envp)
 
     tpm_init();
 
-    if (!xen_enabled()) {
-        /* On 32-bit hosts, QEMU is limited by virtual address space */
-        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
-            error_report("at most 2047 MB RAM can be simulated");
-            exit(1);
-        }
-    }
-
     blk_mig_init();
     ram_mig_init();
     dirty_bitmap_mig_init();
@@ -4287,6 +4282,12 @@  int main(int argc, char **argv, char **envp)
     if (cpu_option) {
         current_machine->cpu_type = parse_cpu_option(cpu_option);
     }
+
+    set_memory_options(&ram_slots, &maxram_size, machine_class);
+    current_machine->ram_size = ram_size;
+    current_machine->maxram_size = maxram_size;
+    current_machine->ram_slots = ram_slots;
+
     parse_numa_opts(current_machine);
 
     if (machine_class->default_ram_id && current_machine->ram_size &&