Re: [OpenBIOS] [Qemu-devel] [PATCH 01/27] Clean up PowerPC SLB handling code

[Andreas Färber]

Am 25.03.2011 um 04:21 schrieb David Gibson:
> 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

According to my bisect, this patch broke ppc64 OpenBIOS.

David, would you please take a look?

Thanks,
Andreas

## bad: 3964f535c35c08470ac69bd553282af500bc8bb0 Merge remote-tracking  
branch 'mst/for_anthony' into staging
## bad: 8500e3a91292f253002783da267f3b08aead86c1 Clean up slb_lookup()  
function
# good: [a0843a68c4e79df801342ec37347dfebc591082e] vnc: fix build  
error from VNC_DIRTY_WORDS
git bisect good a0843a68c4e79df801342ec37347dfebc591082e
# good: [5256d8bfad9b0113dc2f9b57706eaad26b008987] pci: use devfn for  
pci_find_device() instead of (slot, fn) pair
git bisect good 5256d8bfad9b0113dc2f9b57706eaad26b008987
# good: [fcda98630b121a63c9de0705df02e59f4dc2fecc] lm32: rename raise  
opcode to scall
git bisect good fcda98630b121a63c9de0705df02e59f4dc2fecc
# bad: [d569956eaff4be808419f1f259a5c388d8789db4] Add a hook to allow  
hypercalls to be emulated on PowerPC
git bisect bad d569956eaff4be808419f1f259a5c388d8789db4
# good: [17d9b3af5b7f93e43d7fbdcb6f14cad54de9f1ae] target-ppc: ext32u  
instead of andi with constant
git bisect good 17d9b3af5b7f93e43d7fbdcb6f14cad54de9f1ae
# bad: [c48974903051ceb7cfbda23c22c159ea4b482d93] Allow  
qemu_devtree_setprop() to take arbitrary values
git bisect bad c48974903051ceb7cfbda23c22c159ea4b482d93
# bad: [81762d6dd0d430d87024f2c83e9c4dcc4329fb7d] Clean up PowerPC SLB  
handling code
git bisect bad 81762d6dd0d430d87024f2c83e9c4dcc4329fb7d
> 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..124bbbf 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;
>
> }
>
> void ppc_slb_invalidate_all (CPUPPCState *env)
> @@ -760,11 +711,10 @@ void ppc_slb_invalidate_all (CPUPPCState *env)
>     do_invalidate = 0;
>     /* XXX: Warning: slbia never invalidates the first segment */
>     for (n = 1; n < env->slb_nr; n++) {
>    ppc_slb_t *slb = slb_get_entry(env, n);
>    ppc_slb_t *slb = &env->slb[n];
>
>    if (slb_is_valid(slb)) {
>        slb_invalidate(slb);
>        slb_set_entry(env, n, slb);
>    if (slb->esid & SLB_ESID_V) {
>        slb->esid &= ~SLB_ESID_V;
>       /* XXX: given the fact that segment size is 256 MB or 1TB,
>        *      and we still don't have a tlb_flush_mask(env, n,  
>
> mask)
>              *      in Qemu, we just invalidate all TLBs
> @@ -781,68 +731,44 @@ void ppc_slb_invalidate_one (CPUPPCState *env,  
> uint64_t T0)
>     target_ulong vsid, page_mask;
>     int attr;
>     int n;
>
> ppc_slb_t *slb;
>
> n = slb_lookup(env, T0, &vsid, &page_mask, &attr, NULL);
> if (n >= 0) {
>    ppc_slb_t *slb = slb_get_entry(env, n);
>    if (slb_is_valid(slb)) {
>        slb_invalidate(slb);
>        slb_set_entry(env, n, slb);
>        /* XXX: given the fact that segment size is 256 MB or  
>
> 1TB,
>         *      and we still don't have a tlb_flush_mask(env,  
>
> n, mask)
>         *      in Qemu, we just invalidate all TLBs
>         */
>        tlb_flush(env, 1);
>    }
> if (n < 0) {
>    return;
> }
>
> -}
>
> -target_ulong ppc_load_slb (CPUPPCState *env, int slb_nr)
> -{
> target_ulong rt;
> ppc_slb_t *slb = slb_get_entry(env, slb_nr);
> slb = &env->slb[n];
> if (slb_is_valid(slb)) {
>    /* SLB entry is valid */
>    /* Copy SLB bits 62:88 to Rt 37:63 (VSID 23:49) */
>    rt = slb->tmp >> 8;             /* 65:88 => 40:63 */
>    rt |= (slb->tmp64 & 0x7) << 24; /* 62:64 => 37:39 */
>    /* Copy SLB bits 89:92 to Rt 33:36 (KsKpNL) */
>    rt |= ((slb->tmp >> 4) & 0xF) << 27;
> } else {
>    rt = 0;
> }
> LOG_SLB("%s: %016" PRIx64 " %08" PRIx32 " => %d "
>        TARGET_FMT_lx "\n", __func__, slb->tmp64, slb->tmp,  
>
> slb_nr, rt);
> if (slb->esid & SLB_ESID_V) {
>    slb->esid &= ~SLB_ESID_V;
>
> return rt;
>    /* XXX: given the fact that segment size is 256 MB or 1TB,
>     *      and we still don't have a tlb_flush_mask(env, n,  
>
> mask)
>     *      in Qemu, we just invalidate all TLBs
>     */
>    tlb_flush(env, 1);
> }
>
> }
>
> -void ppc_store_slb (CPUPPCState *env, target_ulong rb, target_ulong  
> rs)
> +int ppc_store_slb (CPUPPCState *env, target_ulong rb, target_ulong  
> rs)
> {
> ppc_slb_t *slb;
> uint64_t vsid;
> uint64_t esid;
> int flags, valid, slb_nr;
> vsid = rs >> 12;
> flags = ((rs >> 8) & 0xf);
> int slot = rb & 0xfff;
> uint64_t esid = rb & ~0xfff;
> ppc_slb_t *slb = &env->slb[slot];
> esid = rb >> 28;
> valid = (rb & (1 << 27));
> slb_nr = rb & 0xfff;
> if (slot >= env->slb_nr) {
>    return -1;
> }
> slb = slb_get_entry(env, slb_nr);
> slb->tmp64 = (esid << 28) | valid | (vsid >> 24);
> slb->tmp = (vsid << 8) | (flags << 3);
>
> slb->esid = esid;
>
> slb->vsid = rs;
>
> LOG_SLB("%s: %d " TARGET_FMT_lx " - " TARGET_FMT_lx " => %016"
>
> PRIx64
>        " %08" PRIx32 "\n", __func__, slb_nr, rb, rs, slb->tmp64,
>        slb->tmp);
>        " %016" PRIx64 "\n", __func__, slot, rb, rs,
>        slb->esid, slb->vsid);
>
> slb_set_entry(env, slb_nr, slb);
> return 0;
>
> }
> #endif /* defined(TARGET_PPC64) */
>
> @@ -860,24 +786,22 @@ static inline int get_segment(CPUState *env,  
> mmu_ctx_t *ctx,
> {
>     target_phys_addr_t sdr, hash, mask, sdr_mask, htab_mask;
>     target_ulong sr, vsid, vsid_mask, pgidx, page_mask;
> -#if defined(TARGET_PPC64)
> int attr;
>
> -#endif
>     int ds, vsid_sh, sdr_sh, pr, target_page_bits;
>     int ret, ret2;
> pr = msr_pr;
>
> #if defined(TARGET_PPC64)
>     if (env->mmu_model & POWERPC_MMU_64) {
>    int attr;
>   LOG_MMU("Check SLBs\n");
>   ret = slb_lookup(env, eaddr, &vsid, &page_mask, &attr,
>                    &target_page_bits);
>   if (ret < 0)
>       return ret;
>    ctx->key = ((attr & 0x40) && (pr != 0)) ||
>        ((attr & 0x80) && (pr == 0)) ? 1 : 0;
>    ctx->key = !!(pr ? (attr & SLB_VSID_KP) : (attr &  
>
> SLB_VSID_KS));
>         ds = 0;
>    ctx->nx = attr & 0x10 ? 1 : 0;
>    ctx->nx = !!(attr & SLB_VSID_N);
>   ctx->eaddr = eaddr;
>   vsid_mask = 0x00003FFFFFFFFF80ULL;
>   vsid_sh = 7;
>
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 2bf9283..d512cb0 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -340,7 +340,6 @@ DEF_HELPER_1(74xx_tlbi, void, tl)
> DEF_HELPER_FLAGS_0(tlbia, TCG_CALL_CONST, void)
> DEF_HELPER_FLAGS_1(tlbie, TCG_CALL_CONST, void, tl)
> #if defined(TARGET_PPC64)
> -DEF_HELPER_FLAGS_1(load_slb, TCG_CALL_CONST, tl, tl)
> DEF_HELPER_FLAGS_2(store_slb, TCG_CALL_CONST, void, tl, tl)
> DEF_HELPER_FLAGS_0(slbia, TCG_CALL_CONST, void)
> DEF_HELPER_FLAGS_1(slbie, TCG_CALL_CONST, void, tl)
> diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
> index 17e070a..bf41627 100644
> --- a/target-ppc/op_helper.c
> +++ b/target-ppc/op_helper.c
> @@ -3746,14 +3746,11 @@ void helper_store_sr (target_ulong sr_num,  
> target_ulong val)
>
> /* SLB management */
> #if defined(TARGET_PPC64)
> -target_ulong helper_load_slb (target_ulong slb_nr)
> -{
> return ppc_load_slb(env, slb_nr);
>
> -}
>
> void helper_store_slb (target_ulong rb, target_ulong rs)
> {
> ppc_store_slb(env, rb, rs);
> if (ppc_store_slb(env, rb, rs) < 0) {
>    helper_raise_exception_err(POWERPC_EXCP_PROGRAM,  
>
> POWERPC_EXCP_INVAL);
> }
>
> }
> void helper_slbia (void)
>
> 1.7.1