[-V6,3/3] target-ppc: Fix htab_mask calculation

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

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 <aneesh.kumar@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c          | 3 ++-
 target-ppc/mmu-hash64.c | 2 +-
 target-ppc/mmu_helper.c | 4 +++-
 3 files changed, 6 insertions(+), 3 deletions(-)

On 15.10.2013, at 01:58, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> 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 <aneesh.kumar@linux.vnet.ibm.com>
> ---
> 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
>

Alexander Graf <agraf@suse.de> writes:

> On 15.10.2013, at 01:58, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> 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 <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> 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.

(HTAB_SIZE(spapr) - 1) / 128. I am not sure that is any better ?


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


That is how kernel does the masking. In kvmppc_alloc_hpt() we have

	/* 128 (2**7) bytes in each HPTEG */
	kvm->arch.hpt_mask = (1ul << (order - 7)) - 1;

If we don't have it same, we would end up with wrong hpte index 


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


I can verify the code, but i don't have setup

-aneesh