[Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Nikunj A Dadhania]

Current DEFAULT_RAM_SIZE(128MB) enforced by QEMU would not work for
all machines. Introduce a default_ram_size as part of MachineClass.

The below patches has following behaviour:

1) If the user does not provide "-m" option, machine's default ram
   size will be picked.

2) In case the user provides memory that is lesser than machine's
   default ram size, we upscale the ram_size to machine's
   default_ram_size. A warning is displayed for the change that qemu
   has done.

On the side note, there are other cleanup of removing ram_size, slots
and maxmem from vl.c. All these are being parsed by generic code. This
can be moved to machine specific property. I will take a stab at it
next.

Nikunj A Dadhania (2):
  machine: add default_ram_size to machine class
  spapr: override default ram size 1GB

 hw/core/machine.c   |  8 ++++++++
 hw/ppc/spapr.c      |  2 ++
 include/hw/boards.h |  4 ++++
 vl.c                | 29 +++++++++++++++++------------
 4 files changed, 31 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/2] machine: add default_ram_size to machine class

[Nikunj A Dadhania]

Machines types can have different requirement for default ram
size. Introduce a member in the machine class and set the current
default_ram_size to 128MB.

For QEMUMachine types override the value during the registration of
the machine and for MachineClass introduce the generic class init
setting the default_ram_size.

In case the user passes memory that is lesser that the default ram
size, upscale the value to the machine's default ram size with a
warning.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/core/machine.c   |  8 ++++++++
 include/hw/boards.h |  4 ++++
 vl.c                | 29 +++++++++++++++++------------
 3 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index fbd91be..29c48de 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -403,11 +403,19 @@ bool machine_usb(MachineState *machine)
     return machine->usb;
 }
 
+static void machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->default_ram_size = MACHINE_DEFAULT_RAM_SIZE;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
     .abstract = true,
     .class_size = sizeof(MachineClass),
+    .class_init    = machine_class_init,
     .instance_size = sizeof(MachineState),
     .instance_init = machine_initfn,
     .instance_finalize = machine_finalize,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3ddc449..3fca4e0 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -62,6 +62,9 @@ int qemu_register_machine(QEMUMachine *m);
 #define MACHINE_CLASS(klass) \
     OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
 
+/* Default 128 MB as guest ram size */
+#define MACHINE_DEFAULT_RAM_SIZE (1UL << 27)
+
 MachineClass *find_default_machine(void);
 extern MachineState *current_machine;
 
@@ -108,6 +111,7 @@ struct MachineClass {
     const char *default_display;
     GlobalProperty *compat_props;
     const char *hw_version;
+    ram_addr_t default_ram_size;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
diff --git a/vl.c b/vl.c
index 801d487..7af1c0b 100644
--- a/vl.c
+++ b/vl.c
@@ -120,8 +120,6 @@ int main(int argc, char **argv)
 #include "qom/object_interfaces.h"
 #include "qapi-event.h"
 
-#define DEFAULT_RAM_SIZE 128
-
 #define MAX_VIRTIO_CONSOLES 1
 #define MAX_SCLP_CONSOLES 1
 
@@ -1333,6 +1331,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
     mc->is_default = qm->is_default;
     mc->default_machine_opts = qm->default_machine_opts;
     mc->default_boot_order = qm->default_boot_order;
+    mc->default_ram_size = MACHINE_DEFAULT_RAM_SIZE;
     mc->default_display = qm->default_display;
     mc->compat_props = qm->compat_props;
     mc->hw_version = qm->hw_version;
@@ -2641,13 +2640,13 @@ out:
     return 0;
 }
 
-static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
+static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
+                               MachineClass *mc)
 {
     uint64_t sz;
     const char *mem_str;
     const char *maxmem_str, *slots_str;
-    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
-                                        1024 * 1024;
+    const ram_addr_t default_ram_size = mc->default_ram_size;
     QemuOpts *opts = qemu_find_opts_singleton("memory");
 
     sz = 0;
@@ -2684,6 +2683,12 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
         exit(EXIT_FAILURE);
     }
 
+    if (ram_size < default_ram_size) {
+        fprintf(stderr, "WARNING: qemu: %s guest ram size defaulting to %ld MB\n",
+                mc->name, default_ram_size / (1024 * 1024));
+        ram_size = default_ram_size;
+    }
+
     /* store value for the future use */
     qemu_opt_set_number(opts, "size", ram_size, &error_abort);
     *maxram_size = ram_size;
@@ -3763,7 +3768,13 @@ int main(int argc, char **argv, char **envp)
         machine_class = machine_parse(optarg);
     }
 
-    set_memory_options(&ram_slots, &maxram_size);
+    if (machine_class == NULL) {
+        fprintf(stderr, "No machine specified, and there is no default.\n"
+                "Use -machine help to list supported machines!\n");
+        exit(1);
+    }
+
+    set_memory_options(&ram_slots, &maxram_size, machine_class);
 
     loc_set_none();
 
@@ -3792,12 +3803,6 @@ int main(int argc, char **argv, char **envp)
     }
 #endif
 
-    if (machine_class == NULL) {
-        fprintf(stderr, "No machine specified, and there is no default.\n"
-                "Use -machine help to list supported machines!\n");
-        exit(1);
-    }
-
     current_machine = MACHINE(object_new(object_class_get_name(
                           OBJECT_CLASS(machine_class))));
     if (machine_help_func(qemu_get_machine_opts(), current_machine)) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/2] spapr: override default ram size 1GB

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 23cde20..c71ee4b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -96,6 +96,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
 #define SPAPR_MACHINE(obj) \
     OBJECT_CHECK(sPAPRMachineState, (obj), TYPE_SPAPR_MACHINE)
 
+#define SPAPR_DEFAULT_RAM_SIZE   (1UL << 30)
 /**
  * sPAPRMachineState:
  */
@@ -1738,6 +1739,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = MAX_CPUS;
     mc->no_parallel = 1;
     mc->default_boot_order = NULL;
+    mc->default_ram_size = SPAPR_DEFAULT_RAM_SIZE;
     mc->kvm_type = spapr_kvm_type;
     mc->has_dynamic_sysbus = true;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Markus Armbruster]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> Current DEFAULT_RAM_SIZE(128MB) enforced by QEMU would not work for
> all machines. Introduce a default_ram_size as part of MachineClass.
>
> The below patches has following behaviour:
>
> 1) If the user does not provide "-m" option, machine's default ram
>    size will be picked.
>
> 2) In case the user provides memory that is lesser than machine's
>    default ram size, we upscale the ram_size to machine's
>    default_ram_size. A warning is displayed for the change that qemu
>    has done.

Please do not "improve" the user's explicit order that way.  Either
execute the order as is, or reject it as invalid.

So far, we've always executed this order.

[...]

Re: [Qemu-devel] [PATCH v3 1/2] machine: add default_ram_size to machine class

[Igor Mammedov]

On Thu,  5 Mar 2015 14:36:10 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Machines types can have different requirement for default ram
> size. Introduce a member in the machine class and set the current
> default_ram_size to 128MB.
> 
> For QEMUMachine types override the value during the registration of
> the machine and for MachineClass introduce the generic class init
> setting the default_ram_size.
> 
> In case the user passes memory that is lesser that the default ram
> size, upscale the value to the machine's default ram size with a
> warning.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/core/machine.c   |  8 ++++++++
>  include/hw/boards.h |  4 ++++
>  vl.c                | 29 +++++++++++++++++------------
>  3 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fbd91be..29c48de 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -403,11 +403,19 @@ bool machine_usb(MachineState *machine)
>      return machine->usb;
>  }
>  
> +static void machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->default_ram_size = MACHINE_DEFAULT_RAM_SIZE;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,
>      .abstract = true,
>      .class_size = sizeof(MachineClass),
> +    .class_init    = machine_class_init,
>      .instance_size = sizeof(MachineState),
>      .instance_init = machine_initfn,
>      .instance_finalize = machine_finalize,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ddc449..3fca4e0 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -62,6 +62,9 @@ int qemu_register_machine(QEMUMachine *m);
>  #define MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
>  
> +/* Default 128 MB as guest ram size */
> +#define MACHINE_DEFAULT_RAM_SIZE (1UL << 27)
no need for it to be global, bury it inside hw/core/machine.c

> +
>  MachineClass *find_default_machine(void);
>  extern MachineState *current_machine;
>  
> @@ -108,6 +111,7 @@ struct MachineClass {
>      const char *default_display;
>      GlobalProperty *compat_props;
>      const char *hw_version;
> +    ram_addr_t default_ram_size;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> diff --git a/vl.c b/vl.c
> index 801d487..7af1c0b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -120,8 +120,6 @@ int main(int argc, char **argv)
>  #include "qom/object_interfaces.h"
>  #include "qapi-event.h"
>  
> -#define DEFAULT_RAM_SIZE 128
> -
>  #define MAX_VIRTIO_CONSOLES 1
>  #define MAX_SCLP_CONSOLES 1
>  
> @@ -1333,6 +1331,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
Now it's confusing, we have 2 machine_class_init() one for TYPE_MACHINE
in hw/core/machine.c and another one here for subclasses.

Maybe rename this one to qemu_machine_class_init() with comment that it's
transitional class registration used for converting from legacy QEMUMachine
to MachineClass.

>      mc->is_default = qm->is_default;
>      mc->default_machine_opts = qm->default_machine_opts;
>      mc->default_boot_order = qm->default_boot_order;
> +    mc->default_ram_size = MACHINE_DEFAULT_RAM_SIZE;
this looks wrong, default_ram_size  is initialized when TYPE_MACHINE
class is initialized. No need to override the same default in immediate subclass
 

>      mc->default_display = qm->default_display;
>      mc->compat_props = qm->compat_props;
>      mc->hw_version = qm->hw_version;
> @@ -2641,13 +2640,13 @@ out:
>      return 0;
>  }
>  
> -static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
> +static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
> +                               MachineClass *mc)
>  {
>      uint64_t sz;
>      const char *mem_str;
>      const char *maxmem_str, *slots_str;
> -    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> -                                        1024 * 1024;
> +    const ram_addr_t default_ram_size = mc->default_ram_size;
>      QemuOpts *opts = qemu_find_opts_singleton("memory");
>  
>      sz = 0;
> @@ -2684,6 +2683,12 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>          exit(EXIT_FAILURE);
>      }
>  
> +    if (ram_size < default_ram_size) {
> +        fprintf(stderr, "WARNING: qemu: %s guest ram size defaulting to %ld MB\n",
> +                mc->name, default_ram_size / (1024 * 1024));
> +        ram_size = default_ram_size;
> +    }
In previous review someone explicitly asked not to override lower ram_size
if it was requested by user on command line.

> +
>      /* store value for the future use */
>      qemu_opt_set_number(opts, "size", ram_size, &error_abort);
>      *maxram_size = ram_size;
> @@ -3763,7 +3768,13 @@ int main(int argc, char **argv, char **envp)
>          machine_class = machine_parse(optarg);
>      }
>  
> -    set_memory_options(&ram_slots, &maxram_size);
> +    if (machine_class == NULL) {
> +        fprintf(stderr, "No machine specified, and there is no default.\n"
> +                "Use -machine help to list supported machines!\n");
> +        exit(1);
> +    }
> +
> +    set_memory_options(&ram_slots, &maxram_size, machine_class);
>  
>      loc_set_none();
>  
> @@ -3792,12 +3803,6 @@ int main(int argc, char **argv, char **envp)
>      }
>  #endif
>  
> -    if (machine_class == NULL) {
> -        fprintf(stderr, "No machine specified, and there is no default.\n"
> -                "Use -machine help to list supported machines!\n");
> -        exit(1);
> -    }
> -
>      current_machine = MACHINE(object_new(object_class_get_name(
>                            OBJECT_CLASS(machine_class))));
>      if (machine_help_func(qemu_get_machine_opts(), current_machine)) {

Re: [Qemu-devel] [PATCH v3 2/2] spapr: override default ram size 1GB

[Igor Mammedov]

On Thu,  5 Mar 2015 14:36:11 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/ppc/spapr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 23cde20..c71ee4b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -96,6 +96,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
>  #define SPAPR_MACHINE(obj) \
>      OBJECT_CHECK(sPAPRMachineState, (obj), TYPE_SPAPR_MACHINE)
>  
> +#define SPAPR_DEFAULT_RAM_SIZE   (1UL << 30)
>  /**
>   * sPAPRMachineState:
>   */
> @@ -1738,6 +1739,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = MAX_CPUS;
>      mc->no_parallel = 1;
>      mc->default_boot_order = NULL;
> +    mc->default_ram_size = SPAPR_DEFAULT_RAM_SIZE;
>      mc->kvm_type = spapr_kvm_type;
>      mc->has_dynamic_sysbus = true;
>  

Re: [Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Nikunj A Dadhania]

Markus Armbruster <armbru@redhat.com> writes:

> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>
>> Current DEFAULT_RAM_SIZE(128MB) enforced by QEMU would not work for
>> all machines. Introduce a default_ram_size as part of MachineClass.
>>
>> The below patches has following behaviour:
>>
>> 1) If the user does not provide "-m" option, machine's default ram
>>    size will be picked.
>>
>> 2) In case the user provides memory that is lesser than machine's
>>    default ram size, we upscale the ram_size to machine's
>>    default_ram_size. A warning is displayed for the change that qemu
>>    has done.
>
> Please do not "improve" the user's explicit order that way.  Either
> execute the order as is, or reject it as invalid.

If there is consensus for doing this, I can change the patches
accordingly.

Rejection is also change of behaviour. Because till now, a VM would
start with any memory size, even if it's less that 128MB
(default_ram_size). With rejection, all those VMs would fail booting
displaying the warning. Is this OK?

>
> So far, we've always executed this order.
>
> [...]

Regards,
Nikunj

Re: [Qemu-devel] [PATCH v3 1/2] machine: add default_ram_size to machine class

[Nikunj A Dadhania]

Hi Igor,

Thanks for the review.

Igor Mammedov <imammedo@redhat.com> writes:
> On Thu,  5 Mar 2015 14:36:10 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> Machines types can have different requirement for default ram
>> size. Introduce a member in the machine class and set the current
>> default_ram_size to 128MB.
>> 
>> For QEMUMachine types override the value during the registration of
>> the machine and for MachineClass introduce the generic class init
>> setting the default_ram_size.
>> 
>> In case the user passes memory that is lesser that the default ram
>> size, upscale the value to the machine's default ram size with a
>> warning.
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/core/machine.c   |  8 ++++++++
>>  include/hw/boards.h |  4 ++++
>>  vl.c                | 29 +++++++++++++++++------------
>>  3 files changed, 29 insertions(+), 12 deletions(-)
>> 
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index fbd91be..29c48de 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -403,11 +403,19 @@ bool machine_usb(MachineState *machine)
>>      return machine->usb;
>>  }
>>  
>> +static void machine_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>> +    mc->default_ram_size = MACHINE_DEFAULT_RAM_SIZE;
>> +}
>> +
>>  static const TypeInfo machine_info = {
>>      .name = TYPE_MACHINE,
>>      .parent = TYPE_OBJECT,
>>      .abstract = true,
>>      .class_size = sizeof(MachineClass),
>> +    .class_init    = machine_class_init,
>>      .instance_size = sizeof(MachineState),
>>      .instance_init = machine_initfn,
>>      .instance_finalize = machine_finalize,
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 3ddc449..3fca4e0 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -62,6 +62,9 @@ int qemu_register_machine(QEMUMachine *m);
>>  #define MACHINE_CLASS(klass) \
>>      OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
>>  
>> +/* Default 128 MB as guest ram size */
>> +#define MACHINE_DEFAULT_RAM_SIZE (1UL << 27)
> no need for it to be global, bury it inside hw/core/machine.c

Sure.

>
>> +
>>  MachineClass *find_default_machine(void);
>>  extern MachineState *current_machine;
>>  
>> @@ -108,6 +111,7 @@ struct MachineClass {
>>      const char *default_display;
>>      GlobalProperty *compat_props;
>>      const char *hw_version;
>> +    ram_addr_t default_ram_size;
>>  
>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>                                             DeviceState *dev);
>> diff --git a/vl.c b/vl.c
>> index 801d487..7af1c0b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -120,8 +120,6 @@ int main(int argc, char **argv)
>>  #include "qom/object_interfaces.h"
>>  #include "qapi-event.h"
>>  
>> -#define DEFAULT_RAM_SIZE 128
>> -
>>  #define MAX_VIRTIO_CONSOLES 1
>>  #define MAX_SCLP_CONSOLES 1
>>  
>> @@ -1333,6 +1331,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
> Now it's confusing, we have 2 machine_class_init() one for TYPE_MACHINE
> in hw/core/machine.c and another one here for subclasses.

Both are static, but we can rename one of them for better readablitiy.

> Maybe rename this one to qemu_machine_class_init() with comment that it's
> transitional class registration used for converting from legacy QEMUMachine
> to MachineClass.

Sure.

>
>>      mc->is_default = qm->is_default;
>>      mc->default_machine_opts = qm->default_machine_opts;
>>      mc->default_boot_order = qm->default_boot_order;
>> +    mc->default_ram_size = MACHINE_DEFAULT_RAM_SIZE;
> this looks wrong, default_ram_size  is initialized when TYPE_MACHINE
> class is initialized. No need to override the same default in
> immediate subclass

Oh yes, you are right.

>
>
>>      mc->default_display = qm->default_display;
>>      mc->compat_props = qm->compat_props;
>>      mc->hw_version = qm->hw_version;
>> @@ -2641,13 +2640,13 @@ out:
>>      return 0;
>>  }
>>  
>> -static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>> +static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>> +                               MachineClass *mc)
>>  {
>>      uint64_t sz;
>>      const char *mem_str;
>>      const char *maxmem_str, *slots_str;
>> -    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>> -                                        1024 * 1024;
>> +    const ram_addr_t default_ram_size = mc->default_ram_size;
>>      QemuOpts *opts = qemu_find_opts_singleton("memory");
>>  
>>      sz = 0;
>> @@ -2684,6 +2683,12 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>>          exit(EXIT_FAILURE);
>>      }
>>  
>> +    if (ram_size < default_ram_size) {
>> +        fprintf(stderr, "WARNING: qemu: %s guest ram size defaulting to %ld MB\n",
>> +                mc->name, default_ram_size / (1024 * 1024));
>> +        ram_size = default_ram_size;
>> +    }
> In previous review someone explicitly asked not to override lower ram_size
> if it was requested by user on command line.

We would get to a state where the VM is not bootable. I understand that
user has provided a value, but what if the value is not correct?

>
>> +
>>      /* store value for the future use */
>>      qemu_opt_set_number(opts, "size", ram_size, &error_abort);
>>      *maxram_size = ram_size;
>> @@ -3763,7 +3768,13 @@ int main(int argc, char **argv, char **envp)
>>          machine_class = machine_parse(optarg);
>>      }
>>  
>> -    set_memory_options(&ram_slots, &maxram_size);
>> +    if (machine_class == NULL) {
>> +        fprintf(stderr, "No machine specified, and there is no default.\n"
>> +                "Use -machine help to list supported machines!\n");
>> +        exit(1);
>> +    }
>> +
>> +    set_memory_options(&ram_slots, &maxram_size, machine_class);
>>  
>>      loc_set_none();
>>  
>> @@ -3792,12 +3803,6 @@ int main(int argc, char **argv, char **envp)
>>      }
>>  #endif
>>  
>> -    if (machine_class == NULL) {
>> -        fprintf(stderr, "No machine specified, and there is no default.\n"
>> -                "Use -machine help to list supported machines!\n");
>> -        exit(1);
>> -    }
>> -
>>      current_machine = MACHINE(object_new(object_class_get_name(
>>                            OBJECT_CLASS(machine_class))));
>>      if (machine_help_func(qemu_get_machine_opts(), current_machine)) {

Regards
Nikunj

Re: [Qemu-devel] [PATCH v3 1/2] machine: add default_ram_size to machine class

[Thomas Huth]

On Thu, 05 Mar 2015 16:01:40 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Hi Igor,
> 
> Thanks for the review.
> 
> Igor Mammedov <imammedo@redhat.com> writes:
> > On Thu,  5 Mar 2015 14:36:10 +0530
> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> >
> >> Machines types can have different requirement for default ram
> >> size. Introduce a member in the machine class and set the current
> >> default_ram_size to 128MB.
> >> 
> >> For QEMUMachine types override the value during the registration of
> >> the machine and for MachineClass introduce the generic class init
> >> setting the default_ram_size.
> >> 
> >> In case the user passes memory that is lesser that the default ram
> >> size, upscale the value to the machine's default ram size with a
> >> warning.
...
> >> @@ -2684,6 +2683,12 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
> >>          exit(EXIT_FAILURE);
> >>      }
> >>  
> >> +    if (ram_size < default_ram_size) {
> >> +        fprintf(stderr, "WARNING: qemu: %s guest ram size defaulting to %ld MB\n",
> >> +                mc->name, default_ram_size / (1024 * 1024));
> >> +        ram_size = default_ram_size;
> >> +    }
> > In previous review someone explicitly asked not to override lower ram_size
> > if it was requested by user on command line.
> 
> We would get to a state where the VM is not bootable. I understand that
> user has provided a value, but what if the value is not correct?

Well, as I said before: There are older versions of Linux which run fine
with 128 MB or even 64 MB of memory. Do you really want to block this
just because newer Linux distros now need more RAM now by default?
IMHO if the user specified the amount of RAM at the command line, you
can assume that they know what they are doing.

 Thomas

Re: [Qemu-devel] [PATCH v3 1/2] machine: add default_ram_size to machine class

[Nikunj A Dadhania]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:

> On Thu, 05 Mar 2015 16:01:40 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> Hi Igor,
>> 
>> Thanks for the review.
>> 
>> Igor Mammedov <imammedo@redhat.com> writes:
>> > On Thu,  5 Mar 2015 14:36:10 +0530
>> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>> >
>> >> Machines types can have different requirement for default ram
>> >> size. Introduce a member in the machine class and set the current
>> >> default_ram_size to 128MB.
>> >> 
>> >> For QEMUMachine types override the value during the registration of
>> >> the machine and for MachineClass introduce the generic class init
>> >> setting the default_ram_size.
>> >> 
>> >> In case the user passes memory that is lesser that the default ram
>> >> size, upscale the value to the machine's default ram size with a
>> >> warning.
> ...
>> >> @@ -2684,6 +2683,12 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>> >>          exit(EXIT_FAILURE);
>> >>      }
>> >>  
>> >> +    if (ram_size < default_ram_size) {
>> >> +        fprintf(stderr, "WARNING: qemu: %s guest ram size defaulting to %ld MB\n",
>> >> +                mc->name, default_ram_size / (1024 * 1024));
>> >> +        ram_size = default_ram_size;
>> >> +    }
>> > In previous review someone explicitly asked not to override lower ram_size
>> > if it was requested by user on command line.
>> 
>> We would get to a state where the VM is not bootable. I understand that
>> user has provided a value, but what if the value is not correct?
>
> Well, as I said before: There are older versions of Linux which run fine
> with 128 MB or even 64 MB of memory. Do you really want to block this
> just because newer Linux distros now need more RAM now by default?
> IMHO if the user specified the amount of RAM at the command line, you
> can assume that they know what they are doing.

Sure, I can then just use that input without warning/rejection.

Regards
Nikunj

Re: [Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Peter Maydell]

On 5 March 2015 at 19:24, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> Rejection is also change of behaviour. Because till now, a VM would
> start with any memory size, even if it's less that 128MB
> (default_ram_size). With rejection, all those VMs would fail booting
> displaying the warning. Is this OK?

No. Not all of the machines we emulate are modern machines with
gigabytes of memory -- some are very small boards which might
really only have 64K of RAM. If the user asks for 64K you should
do what they ask.

If what you want is to reject user specified memory sizes which
are too small, this is a "minimum RAM size", which is different
from "default RAM size". It would also be nice to have a
"maximum RAM size", so we can avoid weird failures if the user
asks for 1GB on a board which only has 256MB of space for RAM
in its address map.

Somebody may be along shortly to complain that this doesn't account
for machines where you can only add RAM one DRAM stick at a time
and so 64MB, 128MB and 256MB might all be valid but 100MB not.
At least, that's what happened a few years ago when I tried to
suggest something like these per-board properties...

-- PMM

Re: [Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Markus Armbruster]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>>
>>> Current DEFAULT_RAM_SIZE(128MB) enforced by QEMU would not work for
>>> all machines. Introduce a default_ram_size as part of MachineClass.
>>>
>>> The below patches has following behaviour:
>>>
>>> 1) If the user does not provide "-m" option, machine's default ram
>>>    size will be picked.
>>>
>>> 2) In case the user provides memory that is lesser than machine's
>>>    default ram size, we upscale the ram_size to machine's
>>>    default_ram_size. A warning is displayed for the change that qemu
>>>    has done.
>>
>> Please do not "improve" the user's explicit order that way.  Either
>> execute the order as is, or reject it as invalid.
>
> If there is consensus for doing this, I can change the patches
> accordingly.
>
> Rejection is also change of behaviour. Because till now, a VM would
> start with any memory size, even if it's less that 128MB
> (default_ram_size). With rejection, all those VMs would fail booting
> displaying the warning. Is this OK?

I'd stick to "don't reject".  Yes, the failure mode is ugly.  But
protecting the user from it is also somewhat problematic, because we
don't generally know how much RAM the actual guest requires, and it's an
incompatible change.  Seems not worth it.

Back in 2012, we discussed rejecting RAM size less than 1MiB for PC
machines, because SeaBIOS requires at least that much, and decided
against it.  See
http://www.seabios.org/pipermail/seabios/2012-August/004343.html

If you want to pursue "reject" anyway, please make sure to split this
into two separate patches: one patch to make the default ram size
machine-specific, and another patch to reject user requests for less.

[...]

Re: [Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Nikunj A Dadhania]

Markus Armbruster <armbru@redhat.com> writes:

> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>>>
>>>> Current DEFAULT_RAM_SIZE(128MB) enforced by QEMU would not work for
>>>> all machines. Introduce a default_ram_size as part of MachineClass.
>>>>
>>>> The below patches has following behaviour:
>>>>
>>>> 1) If the user does not provide "-m" option, machine's default ram
>>>>    size will be picked.
>>>>
>>>> 2) In case the user provides memory that is lesser than machine's
>>>>    default ram size, we upscale the ram_size to machine's
>>>>    default_ram_size. A warning is displayed for the change that qemu
>>>>    has done.
>>>
>>> Please do not "improve" the user's explicit order that way.  Either
>>> execute the order as is, or reject it as invalid.
>>
>> If there is consensus for doing this, I can change the patches
>> accordingly.
>>
>> Rejection is also change of behaviour. Because till now, a VM would
>> start with any memory size, even if it's less that 128MB
>> (default_ram_size). With rejection, all those VMs would fail booting
>> displaying the warning. Is this OK?
>
> I'd stick to "don't reject".  

Agree, i have already sent v4 with those changes, as there were multiple
opinions against changing the behaviour.

> Yes, the failure mode is ugly.  But protecting the user from it is
> also somewhat problematic, because we don't generally know how much
> RAM the actual guest requires, and it's an incompatible change.  Seems
> not worth it.
>
> Back in 2012, we discussed rejecting RAM size less than 1MiB for PC
> machines, because SeaBIOS requires at least that much, and decided
> against it.  See
> http://www.seabios.org/pipermail/seabios/2012-August/004343.html
>
> If you want to pursue "reject" anyway, please make sure to split this
> into two separate patches: one patch to make the default ram size
> machine-specific, and another patch to reject user requests for less.
>
> [...]

Thanks,
Nikunj

Re: [Qemu-devel] [PATCH v3 0/2] Introduce default ram size in MachineClass

[Nikunj A Dadhania]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 March 2015 at 19:24, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>> Rejection is also change of behaviour. Because till now, a VM would
>> start with any memory size, even if it's less that 128MB
>> (default_ram_size). With rejection, all those VMs would fail booting
>> displaying the warning. Is this OK?
>
> No. Not all of the machines we emulate are modern machines with
> gigabytes of memory -- some are very small boards which might
> really only have 64K of RAM. If the user asks for 64K you should
> do what they ask.
>
> If what you want is to reject user specified memory sizes which
> are too small, this is a "minimum RAM size", which is different
> from "default RAM size". It would also be nice to have a
> "maximum RAM size", so we can avoid weird failures if the user
> asks for 1GB on a board which only has 256MB of space for RAM
> in its address map.

Yes, [min,max]_ram_size is more appropriate. At present, I have sent a
v4 without changing the default behaviour when user has provided an
option.


> Somebody may be along shortly to complain that this doesn't account
> for machines where you can only add RAM one DRAM stick at a time
> and so 64MB, 128MB and 256MB might all be valid but 100MB not.

Yes, and these would help memory hotplug as well.

> At least, that's what happened a few years ago when I tried to
> suggest something like these per-board properties...
>
> -- PMM

Regards
Nikunj