Re: [Qemu-ppc] [PATCH -V8 3/3] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf]

On 09.01.2014, at 16:46, Aneesh Kumar K.V <address@hidden> wrote:

> Alexander Graf <address@hidden> writes:
> 
>> On 23.12.2013, at 18:23, Aneesh Kumar K.V <address@hidden> wrote:
>> 
>>> From: "Aneesh Kumar K.V" <address@hidden>
>>> 
>>> With kvm enabled, we store the hash page table information in the 
>>> hypervisor.
>>> Use ioctl to read the htab contents. Without this we get the below error 
>>> when
>>> trying to read the guest address
>>> 
>>> (gdb) x/10 do_fork
>>> 0xc000000000098660 <do_fork>:   Cannot access memory at address 
>>> 0xc000000000098660
>>> (gdb)
>>> 
>>> Signed-off-by: Aneesh Kumar K.V <address@hidden>
>>> ---
>>> Changes from V7:
>>> * used uint64_t token
>>> 
>>> hw/ppc/spapr.c          |  1 +
>>> hw/ppc/spapr_hcall.c    | 50 +++++++++++++++++++------------
>>> target-ppc/kvm.c        | 53 +++++++++++++++++++++++++++++++++
>>> target-ppc/kvm_ppc.h    | 19 ++++++++++++
>>> target-ppc/mmu-hash64.c | 78 
>>> ++++++++++++++++++++++++++++++++++++++++---------
>>> target-ppc/mmu-hash64.h | 19 ++++++++----
>>> 6 files changed, 181 insertions(+), 39 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 3c0f29c99820..05244244301a 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -676,6 +676,7 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
>>>    if (shift > 0) {
>>>        /* Kernel handles htab, we don't need to allocate one */
>>>        spapr->htab_shift = shift;
>>> +        kvmppc_kern_htab = true;
>>>    } else {
>>>        if (!spapr->htab) {
>>>            /* Allocate an htab if we don't yet have one */
>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>> index f755a5392317..01cf6b05fee7 100644
>>> --- a/hw/ppc/spapr_hcall.c
>>> +++ b/hw/ppc/spapr_hcall.c
>>> @@ -50,8 +50,9 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
>>> sPAPREnvironment *spapr,
>>>    target_ulong ptel = args[3];
>>>    target_ulong page_shift = 12;
>>>    target_ulong raddr;
>>> -    target_ulong i;
>>> +    target_ulong index;
>>>    hwaddr hpte;
>>> +    uint64_t token;
>>> 
>>>    /* only handle 4k and 16M pages for now */
>>>    if (pteh & HPTE64_V_LARGE) {
>>> @@ -94,30 +95,37 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
>>> sPAPREnvironment *spapr,
>>>    if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>>>        return H_PARAMETER;
>>>    }
>>> +
>>> +    index = 0;
>>> +    hpte = pte_index * HASH_PTE_SIZE_64;
>>>    if (likely((flags & H_EXACT) == 0)) {
>>>        pte_index &= ~7ULL;
>>> -        hpte = pte_index * HASH_PTE_SIZE_64;
>>> -        for (i = 0; ; ++i) {
>>> -            if (i == 8) {
>>> +        token = ppc_hash64_start_access(cpu, pte_index);
>>> +        do {
>>> +            if (index == 8) {
>>> +                ppc_hash64_stop_access(token);
>>>                return H_PTEG_FULL;
>>>            }
>>> -            if ((ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) == 0) {
>>> +            if ((ppc_hash64_load_hpte0(env, token, index) & 
>>> HPTE64_V_VALID) == 0) {
>>>                break;
>>>            }
>>> -            hpte += HASH_PTE_SIZE_64;
>>> -        }
>>> +        } while (index++);
>>> +        ppc_hash64_stop_access(token);
>>>    } else {
>>> -        i = 0;
>>> -        hpte = pte_index * HASH_PTE_SIZE_64;
>>> -        if (ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) {
>>> +        token = ppc_hash64_start_access(cpu, pte_index);
>>> +        if (ppc_hash64_load_hpte0(env, token, 0) & HPTE64_V_VALID) {
>>> +            ppc_hash64_stop_access(token);
>>>            return H_PTEG_FULL;
>>>        }
>>> +        ppc_hash64_stop_access(token);
>>>    }
>>> +    hpte += index * HASH_PTE_SIZE_64;
>>> +
>>>    ppc_hash64_store_hpte1(env, hpte, ptel);
>> 
>> Didn't I mention that stores should be converted as well?
> 
> For the specific use case of x/10i we didn't needed store. And I though
> you agreed that just getting read alone now is useful if we can be
> sure store follow later. I was hoping to get to that later once this
> gets upstream

Ok, please mention these intentions in the cover letter :)


Alex