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

[Scott Wood]

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.   
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