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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

On Tue, May 26, 2015 at 10:00:31AM +0200, Thomas Huth wrote:
> 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?

Ah, good catch.  Clearly not quite back into QOM headspace.

> ...
> > 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?

I don't know, I'm afraid.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]