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

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

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