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

[Igor Mammedov <imammedo@redhat.com>]

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;
> >
> >
>