Re: [Qemu-devel] [PATCH v2] ppc: initialize GPRs as per epapr

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, April 29, 2013 11:21 PM
> To: Bhushan Bharat-R65777
> Cc: address@hidden; address@hidden; address@hidden; Yoder Stuart-
> B08248; address@hidden; Bhushan Bharat-R65777; Bhushan Bharat-
> R65777
> Subject: Re: [PATCH v2] ppc: initialize GPRs as per epapr
> 
> On 04/29/2013 09:40:56 AM, Bharat Bhushan wrote:
> > ePAPR defines the initial values of cpu registers.
> > This patch initialize the GPRs as per ePAPR specification.
> >
> > This resolves the issue of guest reboot/reset (guest hang on reboot).
> >
> > Signed-off-by: Bharat Bhushan <address@hidden>
> > ---
> > v1-->v2
> >  - Dynemic seting of initial map size in gpr[7]
> >  - For this the tlb size calculation is moved into a function.
> >
> >  hw/ppc/e500.c |   29 ++++++++++++++++++++++++++---
> >  1 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index c1bdb6b..decd86c
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -37,6 +37,7 @@
> >  #include "qemu/host-utils.h"
> >  #include "hw/pci-host/ppce500.h"
> >
> > +#define EPAPR_MAGIC                (0x45504150)
> >  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> >  #define UIMAGE_LOAD_BASE           0
> >  #define DTC_LOAD_PAD               0x1800000
> > @@ -393,11 +394,10 @@ static inline hwaddr
> > booke206_page_size_to_tlb(uint64_t size)
> >      return 63 - clz64(size >> 10);
> >  }
> >
> > -static void mmubooke_create_initial_mapping(CPUPPCState *env)
> > +static int booke206_initial_map_tsize(CPUPPCState *env)
> >  {
> >      struct boot_info *bi = env->load_info;
> > -    ppcmas_tlb_t *tlb = booke206_get_tlbm(env, 1, 0, 0);
> > -    hwaddr size, dt_end;
> > +    hwaddr dt_end;
> >      int ps;
> >
> >      /* Our initial TLB entry needs to cover everything from 0 to @@
> > -408,6 +408,23 @@ static void
> > mmubooke_create_initial_mapping(CPUPPCState *env)
> >          /* e500v2 can only do even TLB size bits */
> >          ps++;
> >      }
> > +    return ps;
> > +}
> > +static uint64_t mmubooke_initial_mapsize(CPUPPCState *env)
> 
> Please leave a blank line after each of those } lines.
> 
> You change the function name to look like it's simply returning
> information, but it still creates the TLB entry as far as I can see.

This is just returning the tlb size . not creating the tlb entry.



> Then you go calling it multiple times (why?).  This may not be harmful, but it
> is ugly.
> 
> > +{
> > +    int tsize;
> > +
> > +    tsize = booke206_initial_map_tsize(env);
> > +    return (1ULL << 10 << tsize);
> > +}
> 
> What value does this wrapper have?

This returning the size on initial tlb entry in bytes.

>  The caller needs both the "tsize"
> and the size in bytes, and you only call this wrapper once, so let the caller 
> do
> the conversion instead.

Caller does not need to know both. It only need to know the size in bytes.

I think git diff make a mess of this. just trying to put the code for clarity 

----
static int booke206_initial_map_tsize(CPUPPCState *env)
{
    struct boot_info *bi = env->load_info;
    hwaddr dt_end;
    int ps;

    /* Our initial TLB entry needs to cover everything from 0 to
       the device tree top */
    dt_end = bi->dt_base + bi->dt_size;
    ps = booke206_page_size_to_tlb(dt_end) + 1;
    if (ps & 1) {
        /* e500v2 can only do even TLB size bits */
        ps++;
    }
    return ps;
}
static uint64_t mmubooke_initial_mapsize(CPUPPCState *env)
{
    int tsize;

    tsize = booke206_initial_map_tsize(env);
    return (1ULL << 10 << tsize);
}

static void mmubooke_create_initial_mapping(CPUPPCState *env)
{
    ppcmas_tlb_t *tlb = booke206_get_tlbm(env, 1, 0, 0);
    hwaddr size;
    int ps;

    ps = booke206_initial_map_tsize(env);
    size = (ps << MAS1_TSIZE_SHIFT);
    tlb->mas1 = MAS1_VALID | size;
    tlb->mas2 = 0;
    tlb->mas7_3 = 0;
    tlb->mas7_3 |= MAS3_UR | MAS3_UW | MAS3_UX | MAS3_SR | MAS3_SW | MAS3_SX;

    env->tlb_dirty = true;
}

--------------

Thanks
-Bharat
> 
> -Scott