[Qemu-devel] [PATCH v2] Introduce machine specific default memory size

[Qemu-devel] [PATCH v2] Introduce machine specific default memory size

[Nikunj A Dadhania]

QEMU default memory of 128MB is not enough to boot sPAPR
guest. Introduce a member in the machine class to override the default
memory size enforced by QEMU.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 1 +
 include/hw/boards.h    | 1 +
 include/hw/ppc/spapr.h | 3 +++
 vl.c                   | 9 +++++++++
 4 files changed, 14 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 23cde20..f6b1137 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1738,6 +1738,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;
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3ddc449..b2b4698 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -108,6 +108,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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 716bff4..d401dd0 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -444,6 +444,9 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
 #define SPAPR_VIO_BASE_LIOBN    0x00000000
 #define SPAPR_PCI_BASE_LIOBN    0x80000000
 
+/* Default to 1GB guest ram_size */
+#define SPAPR_DEFAULT_RAM_SIZE  (1ULL << 30)
+
 #define RTAS_ERROR_LOG_MAX      2048
 
 typedef struct sPAPRTCETable sPAPRTCETable;
diff --git a/vl.c b/vl.c
index 801d487..447b993 100644
--- a/vl.c
+++ b/vl.c
@@ -2649,6 +2649,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
     const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
                                         1024 * 1024;
     QemuOpts *opts = qemu_find_opts_singleton("memory");
+    MachineClass *machine_class;
 
     sz = 0;
     mem_str = qemu_opt_get(opts, "size");
@@ -2684,6 +2685,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
         exit(EXIT_FAILURE);
     }
 
+    machine_class = find_default_machine();
+    if (machine_class->default_ram_size && ram_size < machine_class->default_ram_size) {
+        fprintf(stderr, "qemu: %s guest ram size defaulting to %ld MB\n",
+                machine_class->name,
+                machine_class->default_ram_size / (1024 * 1024));
+        ram_size = machine_class->default_ram_size;
+    }
+
     /* store value for the future use */
     qemu_opt_set_number(opts, "size", ram_size, &error_abort);
     *maxram_size = ram_size;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size

[Igor Mammedov]

On Wed,  4 Mar 2015 16:48:05 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> QEMU default memory of 128MB is not enough to boot sPAPR
> guest. Introduce a member in the machine class to override the default
> memory size enforced by QEMU.
> 
now we have DEFAULT_RAM_SIZE in vl.c and default_ram_size in Machine class.
it is confusing.

Could you make current ram_size in vl.c a property of Machine
with default DEFAULT_RAM_SIZE moved there for generic Machine class
and do what you are doing here in property setter. Then it will work
for all machines the same way.

I'd actually suggest to take set_memory_options() and make
ram_size, slots, maxmem a properties of Machine and remap
-m parsing to normal machine option parsing. Getting rid of
global ram_size along the way.


> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         | 1 +
>  include/hw/boards.h    | 1 +
>  include/hw/ppc/spapr.h | 3 +++
>  vl.c                   | 9 +++++++++
>  4 files changed, 14 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 23cde20..f6b1137 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1738,6 +1738,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;
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ddc449..b2b4698 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -108,6 +108,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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 716bff4..d401dd0 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -444,6 +444,9 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>  #define SPAPR_VIO_BASE_LIOBN    0x00000000
>  #define SPAPR_PCI_BASE_LIOBN    0x80000000
>  
> +/* Default to 1GB guest ram_size */
> +#define SPAPR_DEFAULT_RAM_SIZE  (1ULL << 30)
> +
>  #define RTAS_ERROR_LOG_MAX      2048
>  
>  typedef struct sPAPRTCETable sPAPRTCETable;
> diff --git a/vl.c b/vl.c
> index 801d487..447b993 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2649,6 +2649,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>                                          1024 * 1024;
>      QemuOpts *opts = qemu_find_opts_singleton("memory");
> +    MachineClass *machine_class;
>  
>      sz = 0;
>      mem_str = qemu_opt_get(opts, "size");
> @@ -2684,6 +2685,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>          exit(EXIT_FAILURE);
>      }
>  
> +    machine_class = find_default_machine();
> +    if (machine_class->default_ram_size && ram_size < machine_class->default_ram_size) {
> +        fprintf(stderr, "qemu: %s guest ram size defaulting to %ld MB\n",
> +                machine_class->name,
> +                machine_class->default_ram_size / (1024 * 1024));
> +        ram_size = machine_class->default_ram_size;
> +    }
> +
>      /* store value for the future use */
>      qemu_opt_set_number(opts, "size", ram_size, &error_abort);
>      *maxram_size = ram_size;

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] Introduce machine specific default memory size

[Alexander Graf]

On 04.03.15 12:18, Nikunj A Dadhania wrote:
> QEMU default memory of 128MB is not enough to boot sPAPR
> guest. Introduce a member in the machine class to override the default
> memory size enforced by QEMU.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         | 1 +
>  include/hw/boards.h    | 1 +
>  include/hw/ppc/spapr.h | 3 +++
>  vl.c                   | 9 +++++++++
>  4 files changed, 14 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 23cde20..f6b1137 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1738,6 +1738,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;
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ddc449..b2b4698 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -108,6 +108,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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 716bff4..d401dd0 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -444,6 +444,9 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>  #define SPAPR_VIO_BASE_LIOBN    0x00000000
>  #define SPAPR_PCI_BASE_LIOBN    0x80000000
>  
> +/* Default to 1GB guest ram_size */
> +#define SPAPR_DEFAULT_RAM_SIZE  (1ULL << 30)
> +
>  #define RTAS_ERROR_LOG_MAX      2048
>  
>  typedef struct sPAPRTCETable sPAPRTCETable;
> diff --git a/vl.c b/vl.c
> index 801d487..447b993 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2649,6 +2649,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>                                          1024 * 1024;
>      QemuOpts *opts = qemu_find_opts_singleton("memory");
> +    MachineClass *machine_class;
>  
>      sz = 0;
>      mem_str = qemu_opt_get(opts, "size");
> @@ -2684,6 +2685,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>          exit(EXIT_FAILURE);
>      }
>  
> +    machine_class = find_default_machine();
> +    if (machine_class->default_ram_size && ram_size < machine_class->default_ram_size) {
> +        fprintf(stderr, "qemu: %s guest ram size defaulting to %ld MB\n",
> +                machine_class->name,
> +                machine_class->default_ram_size / (1024 * 1024));
> +        ram_size = machine_class->default_ram_size;

Can't the check happen at the same spot that the current 128MB default
gets set?

Also, why so complicated? Just have a default value of 128MB in the
default machine class and just override it from the sPAPR one. Then
unconditionally set ram_size = mc->default_ram_size in the code that
today sets ram_size = 128MB.


Alex

> +    }
> +
>      /* store value for the future use */
>      qemu_opt_set_number(opts, "size", ram_size, &error_abort);
>      *maxram_size = ram_size;
> 

Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size

[Marcel Apfelbaum]

On 03/04/2015 01:18 PM, Nikunj A Dadhania wrote:
> QEMU default memory of 128MB is not enough to boot sPAPR
> guest. Introduce a member in the machine class to override the default
> memory size enforced by QEMU.
>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Hi,
Thank you for contributing to QEMU!

> ---
>   hw/ppc/spapr.c         | 1 +
>   include/hw/boards.h    | 1 +
>   include/hw/ppc/spapr.h | 3 +++
>   vl.c                   | 9 +++++++++
>   4 files changed, 14 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 23cde20..f6b1137 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1738,6 +1738,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;
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ddc449..b2b4698 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -108,6 +108,7 @@ struct MachineClass {
>       const char *default_display;
>       GlobalProperty *compat_props;
>       const char *hw_version;
> +    ram_addr_t default_ram_size;


You add a field to MachineClass that represents default_ram_size.
That's fair because different machine types may have a different "native" default.

But in this case you should set DEFAULT_RAM_SIZE as default for all machine types
for consistency. (as suggested also by others)

>
>       HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                              DeviceState *dev);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 716bff4..d401dd0 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -444,6 +444,9 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>   #define SPAPR_VIO_BASE_LIOBN    0x00000000
>   #define SPAPR_PCI_BASE_LIOBN    0x80000000
>
> +/* Default to 1GB guest ram_size */
> +#define SPAPR_DEFAULT_RAM_SIZE  (1ULL << 30)
> +
>   #define RTAS_ERROR_LOG_MAX      2048
>
>   typedef struct sPAPRTCETable sPAPRTCETable;
> diff --git a/vl.c b/vl.c
> index 801d487..447b993 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2649,6 +2649,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>       const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>                                           1024 * 1024;
>       QemuOpts *opts = qemu_find_opts_singleton("memory");
> +    MachineClass *machine_class;
>
>       sz = 0;
>       mem_str = qemu_opt_get(opts, "size");
> @@ -2684,6 +2685,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>           exit(EXIT_FAILURE);
>       }
>
> +    machine_class = find_default_machine();
I am not sure this is want you want to do here.
I think you want to use the *current* machine class and not the default one.
that can be passed as parameter to set_memory_options.

> +    if (machine_class->default_ram_size && ram_size < machine_class->default_ram_size) {
and here you can drop the  un-natural machine_class->default_ram_size check

> +        fprintf(stderr, "qemu: %s guest ram size defaulting to %ld MB\n",
> +                machine_class->name,
> +                machine_class->default_ram_size / (1024 * 1024));
> +        ram_size = machine_class->default_ram_size;
> +    }
> +
>       /* store value for the future use */
>       qemu_opt_set_number(opts, "size", ram_size, &error_abort);
>       *maxram_size = ram_size;
>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size

[Marcel Apfelbaum]

On 03/04/2015 02:21 PM, Igor Mammedov wrote:
> On Wed,  4 Mar 2015 16:48:05 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> QEMU default memory of 128MB is not enough to boot sPAPR
>> guest. Introduce a member in the machine class to override the default
>> memory size enforced by QEMU.
>>
> now we have DEFAULT_RAM_SIZE in vl.c and default_ram_size in Machine class.
> it is confusing.
Agree, but moving the define into hw/core/machine.c and setting it as
base class default would make this clean.
>
> Could you make current ram_size in vl.c a property of Machine
> with default DEFAULT_RAM_SIZE moved there for generic Machine class
> and do what you are doing here in property setter. Then it will work
> for all machines the same way.
Once is a property with a setter it can be set by command line!
I don't think this is the right think to do.

Thanks,
Marcel

>
> I'd actually suggest to take set_memory_options() and make
> ram_size, slots, maxmem a properties of Machine and remap
> -m parsing to normal machine option parsing. Getting rid of
> global ram_size along the way.
>
>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>   hw/ppc/spapr.c         | 1 +
>>   include/hw/boards.h    | 1 +
>>   include/hw/ppc/spapr.h | 3 +++
>>   vl.c                   | 9 +++++++++
>>   4 files changed, 14 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 23cde20..f6b1137 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1738,6 +1738,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;
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 3ddc449..b2b4698 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -108,6 +108,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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 716bff4..d401dd0 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -444,6 +444,9 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>   #define SPAPR_VIO_BASE_LIOBN    0x00000000
>>   #define SPAPR_PCI_BASE_LIOBN    0x80000000
>>
>> +/* Default to 1GB guest ram_size */
>> +#define SPAPR_DEFAULT_RAM_SIZE  (1ULL << 30)
>> +
>>   #define RTAS_ERROR_LOG_MAX      2048
>>
>>   typedef struct sPAPRTCETable sPAPRTCETable;
>> diff --git a/vl.c b/vl.c
>> index 801d487..447b993 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2649,6 +2649,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>>       const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>>                                           1024 * 1024;
>>       QemuOpts *opts = qemu_find_opts_singleton("memory");
>> +    MachineClass *machine_class;
>>
>>       sz = 0;
>>       mem_str = qemu_opt_get(opts, "size");
>> @@ -2684,6 +2685,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>>           exit(EXIT_FAILURE);
>>       }
>>
>> +    machine_class = find_default_machine();
>> +    if (machine_class->default_ram_size && ram_size < machine_class->default_ram_size) {
>> +        fprintf(stderr, "qemu: %s guest ram size defaulting to %ld MB\n",
>> +                machine_class->name,
>> +                machine_class->default_ram_size / (1024 * 1024));
>> +        ram_size = machine_class->default_ram_size;
>> +    }
>> +
>>       /* store value for the future use */
>>       qemu_opt_set_number(opts, "size", ram_size, &error_abort);
>>       *maxram_size = ram_size;
>
>

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] Introduce machine specific default memory size

[Marcel Apfelbaum]

On 03/04/2015 02:21 PM, Alexander Graf wrote:
>
>
> On 04.03.15 12:18, Nikunj A Dadhania wrote:
>> QEMU default memory of 128MB is not enough to boot sPAPR
>> guest. Introduce a member in the machine class to override the default
>> memory size enforced by QEMU.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>   hw/ppc/spapr.c         | 1 +
>>   include/hw/boards.h    | 1 +
>>   include/hw/ppc/spapr.h | 3 +++
>>   vl.c                   | 9 +++++++++
>>   4 files changed, 14 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 23cde20..f6b1137 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1738,6 +1738,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;
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 3ddc449..b2b4698 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -108,6 +108,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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 716bff4..d401dd0 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -444,6 +444,9 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>   #define SPAPR_VIO_BASE_LIOBN    0x00000000
>>   #define SPAPR_PCI_BASE_LIOBN    0x80000000
>>
>> +/* Default to 1GB guest ram_size */
>> +#define SPAPR_DEFAULT_RAM_SIZE  (1ULL << 30)
>> +
>>   #define RTAS_ERROR_LOG_MAX      2048
>>
>>   typedef struct sPAPRTCETable sPAPRTCETable;
>> diff --git a/vl.c b/vl.c
>> index 801d487..447b993 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2649,6 +2649,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>>       const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>>                                           1024 * 1024;
>>       QemuOpts *opts = qemu_find_opts_singleton("memory");
>> +    MachineClass *machine_class;
>>
>>       sz = 0;
>>       mem_str = qemu_opt_get(opts, "size");
>> @@ -2684,6 +2685,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
>>           exit(EXIT_FAILURE);
>>       }
>>
>> +    machine_class = find_default_machine();
>> +    if (machine_class->default_ram_size && ram_size < machine_class->default_ram_size) {
>> +        fprintf(stderr, "qemu: %s guest ram size defaulting to %ld MB\n",
>> +                machine_class->name,
>> +                machine_class->default_ram_size / (1024 * 1024));
>> +        ram_size = machine_class->default_ram_size;
>
> Can't the check happen at the same spot that the current 128MB default
> gets set?
>
> Also, why so complicated? Just have a default value of 128MB in the
> default machine class and just override it from the sPAPR one. Then
> unconditionally set ram_size = mc->default_ram_size in the code that
> today sets ram_size = 128MB.
+1

That's what I said :), but much more concise.
Thanks,
Marcel

>
>
> Alex
>
>> +    }
>> +
>>       /* store value for the future use */
>>       qemu_opt_set_number(opts, "size", ram_size, &error_abort);
>>       *maxram_size = ram_size;
>>
>

Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size

[Igor Mammedov]

On Wed, 04 Mar 2015 14:32:44 +0200
Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:

> On 03/04/2015 02:21 PM, Igor Mammedov wrote:
> > On Wed,  4 Mar 2015 16:48:05 +0530
> > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> >
> >> QEMU default memory of 128MB is not enough to boot sPAPR
> >> guest. Introduce a member in the machine class to override the default
> >> memory size enforced by QEMU.
> >>
> > now we have DEFAULT_RAM_SIZE in vl.c and default_ram_size in Machine class.
> > it is confusing.
> Agree, but moving the define into hw/core/machine.c and setting it as
> base class default would make this clean.
> >
> > Could you make current ram_size in vl.c a property of Machine
> > with default DEFAULT_RAM_SIZE moved there for generic Machine class
> > and do what you are doing here in property setter. Then it will work
> > for all machines the same way.
> Once is a property with a setter it can be set by command line!
> I don't think this is the right think to do.
Why is it not right?
We could some day abandon -m and just use -machine property parsing.
Another purpose of it is to move machine related code i.e. parsing/
setting/getting ram_size, slots, maxmem to machine class and drop adhoc
parsing and globals in vl.c.

> 
> Thanks,
> Marcel
> 
> >
> > I'd actually suggest to take set_memory_options() and make
> > ram_size, slots, maxmem a properties of Machine and remap
> > -m parsing to normal machine option parsing. Getting rid of
> > global ram_size along the way.
> >
> >
> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >> ---
> >>   hw/ppc/spapr.c         | 1 +
> >>   include/hw/boards.h    | 1 +
> >>   include/hw/ppc/spapr.h | 3 +++
> >>   vl.c                   | 9 +++++++++
> >>   4 files changed, 14 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 23cde20..f6b1137 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1738,6 +1738,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;
> >>
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index 3ddc449..b2b4698 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -108,6 +108,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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 716bff4..d401dd0 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -444,6 +444,9 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
> >>   #define SPAPR_VIO_BASE_LIOBN    0x00000000
> >>   #define SPAPR_PCI_BASE_LIOBN    0x80000000
> >>
> >> +/* Default to 1GB guest ram_size */
> >> +#define SPAPR_DEFAULT_RAM_SIZE  (1ULL << 30)
> >> +
> >>   #define RTAS_ERROR_LOG_MAX      2048
> >>
> >>   typedef struct sPAPRTCETable sPAPRTCETable;
> >> diff --git a/vl.c b/vl.c
> >> index 801d487..447b993 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -2649,6 +2649,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
> >>       const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> >>                                           1024 * 1024;
> >>       QemuOpts *opts = qemu_find_opts_singleton("memory");
> >> +    MachineClass *machine_class;
> >>
> >>       sz = 0;
> >>       mem_str = qemu_opt_get(opts, "size");
> >> @@ -2684,6 +2685,14 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
> >>           exit(EXIT_FAILURE);
> >>       }
> >>
> >> +    machine_class = find_default_machine();
> >> +    if (machine_class->default_ram_size && ram_size < machine_class->default_ram_size) {
> >> +        fprintf(stderr, "qemu: %s guest ram size defaulting to %ld MB\n",
> >> +                machine_class->name,
> >> +                machine_class->default_ram_size / (1024 * 1024));
> >> +        ram_size = machine_class->default_ram_size;
> >> +    }
> >> +
> >>       /* store value for the future use */
> >>       qemu_opt_set_number(opts, "size", ram_size, &error_abort);
> >>       *maxram_size = ram_size;
> >
> >
>