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

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

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