> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, April 29, 2013 11:21 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; ag...@suse.de; Yoder Stuart-
> B08248; da...@gibson.dropbear.id.au; 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 <bharat.bhus...@freescale.com>
> > ---
> > 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
