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
pgpcgPuRyavvC.pgp
Description: PGP signature
