Re: [Qemu-devel] [PATCHv2 2/4] spapr: Remove obsolete ram_limit field from sPAPRMachineState

[Thomas Huth <thuth@redhat.com>]

On Tue, 26 May 2015 12:22:57 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> The ram_limit field was imported from sPAPREnvironment where it predates
> the machine's ram size being available generically from machine->ram_size.
> 
> Worse, the existing code was inconsistent about where it got the ram size
> from.  Sometimes it used spapr->ram_limit, sometimes the global 'ram_size'
> and sometimes a local 'ram_size' masking the global.
> 
> This cleans up the code to consistently use machine->ram_size, eliminating
> spapr->ram_limit in the process.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c         | 22 ++++++++++++----------
>  hw/ppc/spapr_hcall.c   |  3 ++-
>  include/hw/ppc/spapr.h |  1 -
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0016f25..31b29d6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
[...]
> @@ -649,6 +652,7 @@ static void spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
>  
>  static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
>  {
> +    MachineState *machine = &spapr->parent_obj;

Here you use &spapr->parent_obj ... below you use MACHINE(spapr) ...
looks somewhat inconsequent ==> maybe also use MACHINE(spapr) here?

...
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 1a20884..652ddf6 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -87,6 +87,7 @@ static inline bool valid_pte_index(CPUPPCState *env, target_ulong pte_index)
>  static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                              target_ulong opcode, target_ulong *args)
>  {
> +    MachineState *machine = MACHINE(spapr);
>      CPUPPCState *env = &cpu->env;
>      target_ulong flags = args[0];
>      target_ulong pte_index = args[1];

Apart from the above nit, patch looks fine to me, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Another question out of curiosity: Do you know if the global "ram_size"
variable is scheduled to be removed soon in the future?

 Thomas