| Message ID | 1425546371-15909-2-git-send-email-nikunj@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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)) {
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
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
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
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)) {
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(-)