Re: [Qemu-devel] [PATCH -V6 3/3] target-ppc: Fix htab_mask calculation

[Alexander Graf]

On 15.10.2013, at 01:58, Aneesh Kumar K.V <address@hidden> wrote:

> From: "Aneesh Kumar K.V" <address@hidden>
> 
> Correctly update the htab_mask using the return value of
> KVM_PPC_ALLOCATE_HTAB ioctl. Also we don't update sdr1
> on GET_SREGS for HV. So don't update htab_mask if sdr1
> is found to be zero. Fix the pte index calculation to be
> same as that found in the kernel
> 
> Signed-off-by: Aneesh Kumar K.V <address@hidden>
> ---
> hw/ppc/spapr.c          | 3 ++-
> target-ppc/mmu-hash64.c | 2 +-
> target-ppc/mmu_helper.c | 4 +++-
> 3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 22f2a8a..d4f3502 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -724,7 +724,8 @@ static void spapr_cpu_reset(void *opaque)
>         env->external_htab = (void *)1;
>     }
>     env->htab_base = -1;
> -    env->htab_mask = HTAB_SIZE(spapr) - 1;
> +    /* 128 (2**7) bytes in each HPTEG */
> +    env->htab_mask = (1ULL << ((spapr)->htab_shift - 7)) - 1;

HTAB_SIZE(spapr) / 128? The compiler should be smart enough to produce the same 
code out of that.

However, could you please explain why it's better to have the mask be on the 
PTEG rather than the offset? Is this something you missed in the previous 
patch? If so, please change the semantics on what htab_mask means before you 
break the code as that makes bisecting hard.

Furthermore, since you are changing the semantics of htab_mask, have you 
checked all other users of it? Most notably the hash32 code.


Alex

>     env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab |
>         (spapr->htab_shift - 18);
> }
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 5c797c3..ddd8440 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -354,7 +354,7 @@ static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, 
> hwaddr hash,
>     target_ulong pte0, pte1;
>     unsigned long pte_index;
> 
> -    pte_index = (hash * HPTES_PER_GROUP) & env->htab_mask;
> +    pte_index = (hash & env->htab_mask) * HPTES_PER_GROUP;
>     token = ppc_hash64_start_access(ppc_env_get_cpu(env), pte_index, 
> &htab_fd);
>     if (!token) {
>         return -1;
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 04a840b..c39cb7b 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -2025,7 +2025,9 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong 
> value)
>                         " stored in SDR1\n", htabsize);
>                 htabsize = 28;
>             }
> -            env->htab_mask = (1ULL << (htabsize + 18)) - 1;
> +            if (htabsize) {
> +                env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
> +            }
>             env->htab_base = value & SDR_64_HTABORG;
>         } else
> #endif /* defined(TARGET_PPC64) */
> -- 
> 1.8.3.2
>