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

[Aneesh Kumar K.V]

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
>
>>     /* eieio();  FIXME: need some sort of barrier for smp? */
>>     ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
>> 
>> -    args[0] = pte_index + i;
>> +    args[0] = pte_index + index;
>>     return H_SUCCESS;
>> }
>> 
>> @@ -134,16 +142,17 @@ static RemoveResult remove_hpte(CPUPPCState *env, 
>> target_ulong ptex,
>>                                 target_ulong *vp, target_ulong *rp)
>> {
>>     hwaddr hpte;
>> +    uint64_t token;
>>     target_ulong v, r, rb;
>> 
>>     if ((ptex * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>>         return REMOVE_PARM;
>>     }
>> 
>> -    hpte = ptex * HASH_PTE_SIZE_64;
>> -
>> -    v = ppc_hash64_load_hpte0(env, hpte);
>> -    r = ppc_hash64_load_hpte1(env, hpte);
>> +    token = ppc_hash64_start_access(ppc_env_get_cpu(env), ptex);
>> +    v = ppc_hash64_load_hpte0(env, token, 0);
>> +    r = ppc_hash64_load_hpte1(env, token, 0);
>> +    ppc_hash64_stop_access(token);
>> 
>>     if ((v & HPTE64_V_VALID) == 0 ||
>>         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
>> @@ -152,6 +161,7 @@ static RemoveResult remove_hpte(CPUPPCState *env, 
>> target_ulong ptex,
>>     }
>>     *vp = v;
>>     *rp = r;
>> +    hpte = ptex * HASH_PTE_SIZE_64;
>>     ppc_hash64_store_hpte0(env, hpte, HPTE64_V_HPTE_DIRTY);
>>     rb = compute_tlbie_rb(v, r, ptex);
>>     ppc_tlb_invalidate_one(env, rb);
>> @@ -260,16 +270,17 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
>> sPAPREnvironment *spapr,
>>     target_ulong pte_index = args[1];
>>     target_ulong avpn = args[2];
>>     hwaddr hpte;
>> +    uint64_t token;
>>     target_ulong v, r, rb;
>> 
>>     if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
>>         return H_PARAMETER;
>>     }
>> 
>> -    hpte = pte_index * HASH_PTE_SIZE_64;
>> -
>> -    v = ppc_hash64_load_hpte0(env, hpte);
>> -    r = ppc_hash64_load_hpte1(env, hpte);
>> +    token = ppc_hash64_start_access(cpu, pte_index);
>> +    v = ppc_hash64_load_hpte0(env, token, 0);
>> +    r = ppc_hash64_load_hpte1(env, token, 0);
>> +    ppc_hash64_stop_access(token);
>> 
>>     if ((v & HPTE64_V_VALID) == 0 ||
>>         ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
>> @@ -282,6 +293,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
>> sPAPREnvironment *spapr,
>>     r |= (flags << 48) & HPTE64_R_KEY_HI;
>>     r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
>>     rb = compute_tlbie_rb(v, r, pte_index);
>> +    hpte = pte_index * HASH_PTE_SIZE_64;
>>     ppc_hash64_store_hpte0(env, hpte, (v & ~HPTE64_V_VALID) | 
>> HPTE64_V_HPTE_DIRTY);
>>     ppc_tlb_invalidate_one(env, rb);
>>     ppc_hash64_store_hpte1(env, hpte, r);
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index b77ce5e94cbc..8f5c6b39eb38 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -1783,6 +1783,11 @@ bool kvmppc_has_cap_epr(void)
>>     return cap_epr;
>> }
>> 
>> +bool kvmppc_has_cap_htab_fd(void)
>> +{
>> +    return cap_htab_fd;
>> +}
>> +
>> static int kvm_ppc_register_host_cpu_type(void)
>> {
>>     TypeInfo type_info = {
>> @@ -1902,3 +1907,51 @@ int kvm_arch_on_sigbus(int code, void *addr)
>> void kvm_arch_init_irq_routing(KVMState *s)
>> {
>> }
>> +
>> +struct kvm_get_htab_buf {
>> +    struct kvm_get_htab_header header;
>> +    /*
>> +     * We required one extra byte for read
>
> Why past tense?

Wrong english, nothing else :). Will fix

-aneesh