Hi Igor,
Thanks for the review.
Igor Mammedov <imamm...@redhat.com> writes:
> On Thu, 5 Mar 2015 14:36:10 +0530
> Nikunj A Dadhania <nik...@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 <nik...@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
