* Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size
2015-03-04 11:18 [Qemu-devel] [PATCH v2] Introduce machine specific default memory size Nikunj A Dadhania
@ 2015-03-04 12:21 ` Igor Mammedov
2015-03-04 12:32 ` Marcel Apfelbaum
2015-03-04 12:21 ` [Qemu-devel] [Qemu-ppc] " Alexander Graf
2015-03-04 12:26 ` [Qemu-devel] " Marcel Apfelbaum
2 siblings, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2015-03-04 12:21 UTC (permalink / raw)
To: Nikunj A Dadhania; +Cc: thuth, qemu-ppc, qemu-devel, armbru
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;
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size
2015-03-04 12:21 ` Igor Mammedov
@ 2015-03-04 12:32 ` Marcel Apfelbaum
2015-03-04 15:39 ` Igor Mammedov
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-03-04 12:32 UTC (permalink / raw)
To: Igor Mammedov, Nikunj A Dadhania; +Cc: armbru, qemu-ppc, thuth, qemu-devel
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;
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size
2015-03-04 12:32 ` Marcel Apfelbaum
@ 2015-03-04 15:39 ` Igor Mammedov
0 siblings, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2015-03-04 15:39 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: armbru, qemu-ppc, thuth, Nikunj A Dadhania, qemu-devel
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;
> >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] Introduce machine specific default memory size
2015-03-04 11:18 [Qemu-devel] [PATCH v2] Introduce machine specific default memory size Nikunj A Dadhania
2015-03-04 12:21 ` Igor Mammedov
@ 2015-03-04 12:21 ` Alexander Graf
2015-03-04 12:34 ` Marcel Apfelbaum
2015-03-04 12:26 ` [Qemu-devel] " Marcel Apfelbaum
2 siblings, 1 reply; 7+ messages in thread
From: Alexander Graf @ 2015-03-04 12:21 UTC (permalink / raw)
To: Nikunj A Dadhania, qemu-devel; +Cc: qemu-ppc, armbru, thuth
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;
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] Introduce machine specific default memory size
2015-03-04 12:21 ` [Qemu-devel] [Qemu-ppc] " Alexander Graf
@ 2015-03-04 12:34 ` Marcel Apfelbaum
0 siblings, 0 replies; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-03-04 12:34 UTC (permalink / raw)
To: Alexander Graf, Nikunj A Dadhania, qemu-devel; +Cc: qemu-ppc, armbru, thuth
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;
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Introduce machine specific default memory size
2015-03-04 11:18 [Qemu-devel] [PATCH v2] Introduce machine specific default memory size Nikunj A Dadhania
2015-03-04 12:21 ` Igor Mammedov
2015-03-04 12:21 ` [Qemu-devel] [Qemu-ppc] " Alexander Graf
@ 2015-03-04 12:26 ` Marcel Apfelbaum
2 siblings, 0 replies; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-03-04 12:26 UTC (permalink / raw)
To: Nikunj A Dadhania, qemu-devel; +Cc: qemu-ppc, armbru, thuth
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
^ permalink raw reply [flat|nested] 7+ messages in thread