| Message ID | 1367246456-22382-1-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New |
| Headers | show |
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.bhushan@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. 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? 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. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Monday, April 29, 2013 11:21 PM > To: Bhushan Bharat-R65777 > Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; agraf@suse.de; Yoder Stuart- > B08248; david@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.bhushan@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
On 04/29/2013 02:10:14 PM, Bhushan Bharat-R65777 wrote: > > 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 OK, sorry... I misread the diff. -Scott
On 29.04.2013, at 16:40, 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.bhushan@freescale.com> Thanks, applied to ppc-next, adding a white space line between functions and fixing up the commit subject. Alex > --- > 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) > +{ > + 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; > @@ -444,6 +461,12 @@ static void ppce500_cpu_reset(void *opaque) > cs->halted = 0; > env->gpr[1] = (16<<20) - 8; > env->gpr[3] = bi->dt_base; > + env->gpr[4] = 0; > + env->gpr[5] = 0; > + env->gpr[6] = EPAPR_MAGIC; > + env->gpr[7] = mmubooke_initial_mapsize(env); > + env->gpr[8] = 0; > + env->gpr[9] = 0; > env->nip = bi->entry; > mmubooke_create_initial_mapping(env); > } > -- > 1.7.0.4 > >
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) +{ + 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; @@ -444,6 +461,12 @@ static void ppce500_cpu_reset(void *opaque) cs->halted = 0; env->gpr[1] = (16<<20) - 8; env->gpr[3] = bi->dt_base; + env->gpr[4] = 0; + env->gpr[5] = 0; + env->gpr[6] = EPAPR_MAGIC; + env->gpr[7] = mmubooke_initial_mapsize(env); + env->gpr[8] = 0; + env->gpr[9] = 0; env->nip = bi->entry; mmubooke_create_initial_mapping(env); }
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.bhushan@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(-)