Re: [Qemu-devel] [RFC PATCH v1 11/13] spapr: Initialize hotplug memory address space

[David Gibson]

On Thu, Feb 12, 2015 at 11:09:14AM +0530, Bharata B Rao wrote:
> On Thu, Feb 12, 2015 at 04:19:36PM +1100, David Gibson wrote:
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 44405b2..9ff08ff 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -120,6 +120,8 @@ struct sPAPRMachineState {
> > >  
> > >      /*< public >*/
> > >      char *kvm_type;
> > > +    ram_addr_t hotplug_memory_base;
> > > +    MemoryRegion hotplug_memory;
> > 
> > We should really unify sPAPRMachineState with sPAPREnvironment at some
> > point (I realise that doesn't reasonably fit within the scope of this
> > series).
> 
> ok.
> 
> > 
> > >  };
> > >  
> > >  sPAPREnvironment *spapr;
> > > @@ -1403,6 +1405,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >      bool kernel_le = false;
> > >      char *filename;
> > >      int smt = kvmppc_smt_threads();
> > > +    sPAPRMachineState *ms = SPAPR_MACHINE(machine);
> > >  
> > >      msi_supported = true;
> > >  
> > > @@ -1492,6 +1495,29 @@ static void ppc_spapr_init(MachineState *machine)
> > >          memory_region_add_subregion(sysmem, 0, rma_region);
> > >      }
> > >  
> > > +    if (machine->ram_size < machine->maxram_size) {
> > > +        ram_addr_t hotplug_mem_size = machine->maxram_size - 
> > > machine->ram_size;
> > > +
> > > +        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> > > +            error_report("unsupported amount of memory slots: %"PRIu64,
> > > +                         machine->ram_slots);
> > > +            exit(EXIT_FAILURE);
> > > +        }
> > > +
> > > +        ms->hotplug_memory_base = ROUND_UP(machine->ram_size, 1ULL << 
> > > 30);
> > 
> > Is there a particular significance to the 1GiB alignment?  Is it just
> > a conveniently large alignment, or is that value specified in PAPR
> > somewhere?  Using a named constant would probably help to clarify that.
> 
> I am basing this on x86 memory hotplug and that's how 1GB is coming. It
> is not PAPR specified.

Ok, it should definitely be a #define.

> > > +        if ((ms->hotplug_memory_base + hotplug_mem_size) < 
> > > hotplug_mem_size) {
> > > +            error_report("unsupported amount of maximum memory: " 
> > > RAM_ADDR_FMT,
> > > +                         machine->maxram_size);
> > > +            exit(EXIT_FAILURE);
> > > +        }
> > > +
> > > +        memory_region_init(&ms->hotplug_memory, OBJECT(ms),
> > > +                           "hotplug-memory", hotplug_mem_size);
> > > +        memory_region_add_subregion(sysmem, ms->hotplug_memory_base,
> > > +                                    &ms->hotplug_memory);
> > > +    }
> > > +
> > >      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
> > >      spapr->rtas_size = get_image_size(filename);
> > >      spapr->rtas_blob = g_malloc(spapr->rtas_size);
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index ae8b4e1..64681c4 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -482,6 +482,9 @@ struct sPAPRTCETable {
> > >  #define TIMEBASE_FREQ           512000000ULL
> > >  #define SPAPR_MIN_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
> > >  
> > > +/* Support a min of 1TB hotplug memory assuming 256MB per slot */
> > > +#define SPAPR_MAX_RAM_SLOTS     (1ULL << 12)
> > 
> > Is this constraint arbitrary, or does it come from something in PAPR+?
> 
> Arbitrary max, not defined by PAPR.

Ok.  Why do we need a fixed maximum value?  I don't see this used to
size any arrays..

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

pgpE3NovdRjH_.pgp
Description: PGP signature