Re: [Qemu-devel] [PATCH -V4 2/4] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf <agraf@suse.de>]

On 09/05/2013 10:16 AM, Aneesh Kumar K.V wrote:
> From: "Aneesh Kumar K.V"<aneesh.kumar@linux.vnet.ibm.com>
>
> 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<aneesh.kumar@linux.vnet.ibm.com>
> ---
>   target-ppc/kvm.c        | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
>   target-ppc/kvm_ppc.h    | 12 +++++++++-
>   target-ppc/mmu-hash64.c | 57 ++++++++++++++++++++++++++++-------------------
>   3 files changed, 104 insertions(+), 24 deletions(-)
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 1838465..05b066c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1888,3 +1888,62 @@ int kvm_arch_on_sigbus(int code, void *addr)
>   void kvm_arch_init_irq_routing(KVMState *s)
>   {
>   }
> +
> +hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> +                                 bool secondary, target_ulong ptem,
> +                                 target_ulong *hpte0, target_ulong *hpte1)
> +{
> +    int htab_fd;
> +    uint64_t index;
> +    hwaddr pte_offset;
> +    target_ulong pte0, pte1;
> +    struct kvm_get_htab_fd ghf;
> +    struct kvm_get_htab_buf {
> +        struct kvm_get_htab_header header;
> +        /*
> +         * Older kernel required one extra byte.

Older than what?

> +         */
> +        unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
> +    } hpte_buf;
> +
> +    index = (hash * HPTES_PER_GROUP)&  cpu->env.htab_mask;
> +    *hpte0 = 0;
> +    *hpte1 = 0;
> +    if (!cap_htab_fd) {
> +        return 0;
> +    }
> +
> +    ghf.flags = 0;
> +    ghf.start_index = index;
> +    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD,&ghf);
> +    if (htab_fd<  0) {
> +        goto error_out;
> +    }
> +    /*
> +     * Read the hpte group
> +     */
> +    if (read(htab_fd,&hpte_buf, sizeof(hpte_buf))<  0) {
> +        goto out;
> +    }
> +
> +    index = 0;
> +    pte_offset = (hash * HASH_PTEG_SIZE_64)&  cpu->env.htab_mask;;
> +    while (index<  hpte_buf.header.n_valid) {
> +        pte0 = hpte_buf.hpte[(index * 2)];
> +        pte1 = hpte_buf.hpte[(index * 2) + 1];
> +        if ((pte0&  HPTE64_V_VALID)
> +&&  (secondary == !!(pte0&  HPTE64_V_SECONDARY))
> +&&  HPTE64_V_COMPARE(pte0, ptem)) {
> +            *hpte0 = pte0;
> +            *hpte1 = pte1;
> +            close(htab_fd);
> +            return pte_offset;
> +        }
> +        index++;
> +        pte_offset += HASH_PTE_SIZE_64;
> +    }
> +out:
> +    close(htab_fd);
> +error_out:
> +    return -1;
> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4ae7bf2..dad0e57 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -42,7 +42,9 @@ int kvmppc_get_htab_fd(bool write);
>   int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
>   int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>                              uint16_t n_valid, uint16_t n_invalid);
> -
> +hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> +                                 bool secondary, target_ulong ptem,
> +                                 target_ulong *hpte0, target_ulong *hpte1);
>   #else
>
>   static inline uint32_t kvmppc_get_tbfreq(void)
> @@ -181,6 +183,14 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>       abort();
>   }
>
> +static inline hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> +                                               bool secondary,
> +                                               target_ulong ptem,
> +                                               target_ulong *hpte0,
> +                                               target_ulong *hpte1)
> +{
> +    abort();
> +}
>   #endif
>
>   #ifndef CONFIG_KVM
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 67fc1b5..2288fe8 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -302,37 +302,50 @@ static int ppc_hash64_amr_prot(CPUPPCState *env, ppc_hash_pte64_t pte)
>       return prot;
>   }
>
> -static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr pteg_off,
> +static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr hash,
>                                        bool secondary, target_ulong ptem,
>                                        ppc_hash_pte64_t *pte)
>   {
> -    hwaddr pte_offset = pteg_off;
> +    hwaddr pte_offset;
>       target_ulong pte0, pte1;
> -    int i;
> -
> -    for (i = 0; i<  HPTES_PER_GROUP; i++) {
> -        pte0 = ppc_hash64_load_hpte0(env, pte_offset);
> -        pte1 = ppc_hash64_load_hpte1(env, pte_offset);
> -
> -        if ((pte0&  HPTE64_V_VALID)
> -&&  (secondary == !!(pte0&  HPTE64_V_SECONDARY))
> -&&  HPTE64_V_COMPARE(pte0, ptem)) {
> -            pte->pte0 = pte0;
> -            pte->pte1 = pte1;
> -            return pte_offset;
> +    int i, ret = 0;
> +
> +    if (kvm_enabled()) {
> +        ret = kvmppc_hash64_pteg_search(ppc_env_get_cpu(env), hash,
> +                                        secondary, ptem,
> +&pte->pte0,&pte->pte1);

Instead of duplicating the search, couldn't you just hook yourself into 
ppc_hash64_load_hpte0/1 and return the respective ptes from there? Just 
cache the current pteg to ensure things don't become dog slow.


Alex

> +    }
> +    /*
> +     * We don't support htab fd, check whether we have a copy of htab
> +     */
> +    if (!ret) {
> +        pte_offset = (hash * HASH_PTEG_SIZE_64)&  env->htab_mask;;
> +        for (i = 0; i<  HPTES_PER_GROUP; i++) {
> +            pte0 = ppc_hash64_load_hpte0(env, pte_offset);
> +            pte1 = ppc_hash64_load_hpte1(env, pte_offset);
> +
> +            if ((pte0&  HPTE64_V_VALID)
> +&&  (secondary == !!(pte0&  HPTE64_V_SECONDARY))
> +&&  HPTE64_V_COMPARE(pte0, ptem)) {
> +                pte->pte0 = pte0;
> +                pte->pte1 = pte1;
> +                return pte_offset;
> +            }
> +            pte_offset += HASH_PTE_SIZE_64;
>           }
> -
> -        pte_offset += HASH_PTE_SIZE_64;
> +        /*
> +         * We didn't find a valid entry.
> +         */
> +        ret = -1;
>       }
> -
> -    return -1;
> +    return ret;
>   }
>
>   static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
>                                        ppc_slb_t *slb, target_ulong eaddr,
>                                        ppc_hash_pte64_t *pte)
>   {
> -    hwaddr pteg_off, pte_offset;
> +    hwaddr pte_offset;
>       hwaddr hash;
>       uint64_t vsid, epnshift, epnmask, epn, ptem;
>
> @@ -367,8 +380,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
>               " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
>               " hash=" TARGET_FMT_plx "\n",
>               env->htab_base, env->htab_mask, vsid, ptem,  hash);
> -    pteg_off = (hash * HASH_PTEG_SIZE_64)&  env->htab_mask;
> -    pte_offset = ppc_hash64_pteg_search(env, pteg_off, 0, ptem, pte);
> +    pte_offset = ppc_hash64_pteg_search(env, hash, 0, ptem, pte);
>
>       if (pte_offset == -1) {
>           /* Secondary PTEG lookup */
> @@ -377,8 +389,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
>                   " hash=" TARGET_FMT_plx "\n", env->htab_base,
>                   env->htab_mask, vsid, ptem, ~hash);
>
> -        pteg_off = (~hash * HASH_PTEG_SIZE_64)&  env->htab_mask;
> -        pte_offset = ppc_hash64_pteg_search(env, pteg_off, 1, ptem, pte);
> +        pte_offset = ppc_hash64_pteg_search(env, ~hash, 1, ptem, pte);
>       }
>
>       return pte_offset;