[Qemu-devel] Re: [PATCH 02/15] Clean up PowerPC SLB handling code

[Alexander Graf]

On 12.02.2011, at 15:54, David Gibson wrote:

> Currently the SLB information when emulating a PowerPC 970 is
> storeed in a structure with the unhelpfully named fields 'tmp'
> and 'tmp64'.  While the layout in these fields does match the
> description of the SLB in the architecture document, it is not
> convenient either for looking up the SLB, or for emulating the
> slbmte instruction.
> 
> This patch, therefore, reorganizes the SLB entry structure to be
> divided in the the "ESID related" and "VSID related" fields as
> they are divided in instructions accessing the SLB.
> 
> In addition to making the code smaller and more readable, this will
> make it easier to implement for the 1TB segments used in more
> recent PowerPC chips.
> 
> Signed-off-by: David Gibson <address@hidden>

Acked-by: Alexander Graf <address@hidden>

> ---
> target-ppc/cpu.h       |   29 +++++++-
> target-ppc/helper.c    |  178 ++++++++++++++----------------------------------
> target-ppc/helper.h    |    1 -
> target-ppc/op_helper.c |    9 +--
> 4 files changed, 80 insertions(+), 137 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index deb8d7c..a20c132 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -43,6 +43,8 @@
> # define TARGET_VIRT_ADDR_SPACE_BITS 64
> #endif
> 
> +#define TARGET_PAGE_BITS_16M 24
> +
> #else /* defined (TARGET_PPC64) */
> /* PowerPC 32 definitions */
> #define TARGET_LONG_BITS 32
> @@ -359,10 +361,31 @@ union ppc_tlb_t {
> 
> typedef struct ppc_slb_t ppc_slb_t;
> struct ppc_slb_t {
> -    uint64_t tmp64;
> -    uint32_t tmp;
> +    uint64_t esid;
> +    uint64_t vsid;
> };
> 
> +/* Bits in the SLB ESID word */
> +#define SLB_ESID_ESID           0xFFFFFFFFF0000000ULL
> +#define SLB_ESID_V              0x0000000008000000ULL /* valid */
> +
> +/* Bits in the SLB VSID word */
> +#define SLB_VSID_SHIFT          12
> +#define SLB_VSID_SSIZE_SHIFT    62
> +#define SLB_VSID_B              0xc000000000000000ULL
> +#define SLB_VSID_B_256M         0x0000000000000000ULL
> +#define SLB_VSID_VSID           0x3FFFFFFFFFFFF000ULL
> +#define SLB_VSID_KS             0x0000000000000800ULL
> +#define SLB_VSID_KP             0x0000000000000400ULL
> +#define SLB_VSID_N              0x0000000000000200ULL /* no-execute */
> +#define SLB_VSID_L              0x0000000000000100ULL
> +#define SLB_VSID_C              0x0000000000000080ULL /* class */
> +#define SLB_VSID_LP             0x0000000000000030ULL
> +#define SLB_VSID_ATTR           0x0000000000000FFFULL
> +
> +#define SEGMENT_SHIFT_256M      28
> +#define SEGMENT_MASK_256M       ~((1ULL << SEGMENT_SHIFT_256M) - 1)
> +
> /*****************************************************************************/
> /* Machine state register bits definition                                    
> */
> #define MSR_SF   63 /* Sixty-four-bit mode                            hflags 
> */
> @@ -755,7 +778,7 @@ void ppc_store_sdr1 (CPUPPCState *env, target_ulong 
> value);
> void ppc_store_asr (CPUPPCState *env, target_ulong value);
> target_ulong ppc_load_slb (CPUPPCState *env, int slb_nr);
> target_ulong ppc_load_sr (CPUPPCState *env, int sr_nr);
> -void ppc_store_slb (CPUPPCState *env, target_ulong rb, target_ulong rs);
> +int ppc_store_slb (CPUPPCState *env, target_ulong rb, target_ulong rs);
> #endif /* defined(TARGET_PPC64) */
> void ppc_store_sr (CPUPPCState *env, int srnum, target_ulong value);
> #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 4b49101..2094ca3 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -672,85 +672,36 @@ static inline int find_pte(CPUState *env, mmu_ctx_t 
> *ctx, int h, int rw,
> }
> 
> #if defined(TARGET_PPC64)
> -static ppc_slb_t *slb_get_entry(CPUPPCState *env, int nr)
> -{
> -    ppc_slb_t *retval = &env->slb[nr];
> -
> -#if 0 // XXX implement bridge mode?
> -    if (env->spr[SPR_ASR] & 1) {
> -        target_phys_addr_t sr_base;
> -
> -        sr_base = env->spr[SPR_ASR] & 0xfffffffffffff000;
> -        sr_base += (12 * nr);
> -
> -        retval->tmp64 = ldq_phys(sr_base);
> -        retval->tmp = ldl_phys(sr_base + 8);
> -    }
> -#endif
> -
> -    return retval;
> -}
> -
> -static void slb_set_entry(CPUPPCState *env, int nr, ppc_slb_t *slb)
> -{
> -    ppc_slb_t *entry = &env->slb[nr];
> -
> -    if (slb == entry)
> -        return;
> -
> -    entry->tmp64 = slb->tmp64;
> -    entry->tmp = slb->tmp;
> -}
> -
> -static inline int slb_is_valid(ppc_slb_t *slb)
> -{
> -    return (int)(slb->tmp64 & 0x0000000008000000ULL);
> -}
> -
> -static inline void slb_invalidate(ppc_slb_t *slb)
> -{
> -    slb->tmp64 &= ~0x0000000008000000ULL;
> -}
> -
> static inline int slb_lookup(CPUPPCState *env, target_ulong eaddr,
>                              target_ulong *vsid, target_ulong *page_mask,
>                              int *attr, int *target_page_bits)
> {
> -    target_ulong mask;
> -    int n, ret;
> +    uint64_t esid;
> +    int n;
> 
> -    ret = -5;
>     LOG_SLB("%s: eaddr " TARGET_FMT_lx "\n", __func__, eaddr);
> -    mask = 0x0000000000000000ULL; /* Avoid gcc warning */
> +
> +    esid = (eaddr & SEGMENT_MASK_256M) | SLB_ESID_V;
> +
>     for (n = 0; n < env->slb_nr; n++) {
> -        ppc_slb_t *slb = slb_get_entry(env, n);
> -
> -        LOG_SLB("%s: seg %d %016" PRIx64 " %08"
> -                    PRIx32 "\n", __func__, n, slb->tmp64, slb->tmp);
> -        if (slb_is_valid(slb)) {
> -            /* SLB entry is valid */
> -            mask = 0xFFFFFFFFF0000000ULL;
> -            if (slb->tmp & 0x8) {
> -                /* 16 MB PTEs */
> -                if (target_page_bits)
> -                    *target_page_bits = 24;
> -            } else {
> -                /* 4 KB PTEs */
> -                if (target_page_bits)
> -                    *target_page_bits = TARGET_PAGE_BITS;
> -            }
> -            if ((eaddr & mask) == (slb->tmp64 & mask)) {
> -                /* SLB match */
> -                *vsid = ((slb->tmp64 << 24) | (slb->tmp >> 8)) & 
> 0x0003FFFFFFFFFFFFULL;
> -                *page_mask = ~mask;
> -                *attr = slb->tmp & 0xFF;
> -                ret = n;
> -                break;
> +        ppc_slb_t *slb = &env->slb[n];
> +
> +        LOG_SLB("%s: slot %d %016" PRIx64 " %016"
> +                    PRIx64 "\n", __func__, n, slb->esid, slb->vsid);
> +        if (slb->esid == esid) {
> +            *vsid = (slb->vsid & SLB_VSID_VSID) >> SLB_VSID_SHIFT;
> +            *page_mask = ~SEGMENT_MASK_256M;
> +            *attr = slb->vsid & SLB_VSID_ATTR;
> +            if (target_page_bits) {
> +                *target_page_bits = (slb->vsid & SLB_VSID_L)
> +                    ? TARGET_PAGE_BITS_16M
> +                    : TARGET_PAGE_BITS;
>             }
> +            return n;
>         }
>     }
> 
> -    return ret;
> +    return -5;

While at it, -5 really is not very verbose :). Doesn't have to be addressed in 
this patch though.


Alex