Re: [Qemu-devel] [RFC PATCH v1 05/13] spapr: Support ibm, lrdr-capacity device tree property

[Bharata B Rao]

On Thu, Jan 22, 2015 at 03:55:40PM -0600, Michael Roth wrote:
> Quoting Bharata B Rao (2015-01-08 00:10:12)
> > Add support for ibm,lrdr-capacity since this is needed by the guest
> > kernel to know about the possible hot-pluggable CPUs and Memory.
> > 
> > Define minimum hotpluggable memory size as 256MB and start storing maximum
> > possible memory for the guest in sPAPREnvironment.
> > 
> > Signed-off-by: Bharata B Rao <address@hidden>
> > ---
> >  hw/ppc/spapr.c         |  3 ++-
> >  hw/ppc/spapr_rtas.c    | 28 ++++++++++++++++++++++++++--
> >  include/hw/ppc/spapr.h |  6 ++++--
> >  3 files changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f49b0fa..515d770 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -775,7 +775,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
> >      }
> > 
> >      /* RTAS */
> > -    ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
> > +    ret = spapr_rtas_device_tree_setup(spapr, fdt, rtas_addr, rtas_size);
> >      if (ret < 0) {
> >          fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
> >      }
> > @@ -1473,6 +1473,7 @@ static void ppc_spapr_init(MachineState *machine)
> > 
> >      /* allocate RAM */
> >      spapr->ram_limit = ram_size;
> > +    spapr->maxram_limit = machine->maxram_size;
> >      memory_region_allocate_system_memory(ram, NULL, "ppc_spapr.ram",
> >                                           spapr->ram_limit);
> >      memory_region_add_subregion(sysmem, 0, ram);
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index d847f45..e8a0f21 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -29,6 +29,7 @@
> >  #include "sysemu/char.h"
> >  #include "hw/qdev.h"
> >  #include "sysemu/device_tree.h"
> > +#include "sysemu/cpus.h"
> > 
> >  #include "hw/ppc/spapr.h"
> >  #include "hw/ppc/spapr_vio.h"
> > @@ -551,11 +552,12 @@ void spapr_rtas_register(int token, const char *name, 
> > spapr_rtas_fn fn)
> >      rtas_table[token].fn = fn;
> >  }
> > 
> > -int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
> > -                                 hwaddr rtas_size)
> > +int spapr_rtas_device_tree_setup(sPAPREnvironment *spapr, void *fdt,
> > +                                 hwaddr rtas_addr, hwaddr rtas_size)
> >  {
> >      int ret;
> >      int i;
> > +    uint32_t lrdr_capacity[5];
> > 
> >      ret = fdt_add_mem_rsv(fdt, rtas_addr, rtas_size);
> >      if (ret < 0) {
> > @@ -604,6 +606,28 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr 
> > rtas_addr,
> >          }
> > 
> >      }
> > +
> > +    ret = qemu_fdt_setprop_cell(fdt, "/rtas", "#address-cells", 0x2);
> > +    if (ret < 0) {
> > +        fprintf(stderr, "Couldn't add #address-cells rtas property\n");
> > +    }
> > +
> > +    ret = qemu_fdt_setprop_cell(fdt, "/rtas", "#size-cells", 0x2);
> > +    if (ret < 0) {
> > +        fprintf(stderr, "Couldn't add #size-cells rtas property\n");
> > +    }
> > +
> > +    lrdr_capacity[0] = cpu_to_be32(spapr->maxram_limit >> 32);
> > +    lrdr_capacity[1] = cpu_to_be32(spapr->maxram_limit & 0xffffffff);
> > +    lrdr_capacity[2] = 0;
> > +    lrdr_capacity[3] = cpu_to_be32(SPAPR_MIN_MEMORY_BLOCK_SIZE);
> > +    lrdr_capacity[4] = cpu_to_be32(max_cpus/smp_threads);
> > +    ret = qemu_fdt_setprop(fdt, "/rtas", "ibm,lrdr-capacity", 
> > lrdr_capacity,
> > +                     sizeof(lrdr_capacity));
> > +    if (ret < 0) {
> > +        fprintf(stderr, "Couldn't add ibm,lrdr-capacity rtas property\n");
> > +    }
> > +
> 
> The property seems simple enough, but would be worthwhile to add a description
> of how/when it's used in a new section of docs/specs/ppc-spapr-hotplug.txt to
> keep the documentation complete

Sure.

> 
> >      return 0;
> >  }
> > 
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 831db6b..ae8b4e1 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -18,6 +18,7 @@ typedef struct sPAPREnvironment {
> >      XICSState *icp;
> > 
> >      hwaddr ram_limit;
> > +    hwaddr maxram_limit;
> >      void *htab;
> >      uint32_t htab_shift;
> >      hwaddr rma_size;
> > @@ -444,8 +445,8 @@ void spapr_rtas_register(int token, const char *name, 
> > spapr_rtas_fn fn);
> >  target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >                               uint32_t token, uint32_t nargs, target_ulong 
> > args,
> >                               uint32_t nret, target_ulong rets);
> > -int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
> > -                                 hwaddr rtas_size);
> > +int spapr_rtas_device_tree_setup(sPAPREnvironment *spapr, void *fdt,
> > +                                 hwaddr rtas_addr, hwaddr rtas_size);
> > 
> >  #define SPAPR_TCE_PAGE_SHIFT   12
> >  #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)
> > @@ -479,6 +480,7 @@ struct sPAPRTCETable {
> >  };
> > 
> >  #define TIMEBASE_FREQ           512000000ULL
> > +#define SPAPR_MIN_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
> 
> Is this actually the min, or a set increment size? Documentation suggests
> the latter, in which case the naming is a little confusing.

I should just call it SPAPR_MEMORY_BLOCK_SIZE.

Thanks for your review.

Regards,
Bharata.