[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH -V5] target-ppc: Fix page table lookup with kvm
|
From: |
Alexander Graf |
|
Subject: |
Re: [Qemu-devel] [PATCH -V5] target-ppc: Fix page table lookup with kvm enabled |
|
Date: |
Fri, 11 Oct 2013 15:36:48 +0200 |
On 11.10.2013, at 13:13, 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 V4:
> * Rewrite to avoid two code paths for doing hash lookups
>
> hw/ppc/spapr_hcall.c | 44 ++++++++++++++++++++-----------
> target-ppc/kvm.c | 47 +++++++++++++++++++++++++++++++++
> target-ppc/kvm_ppc.h | 16 +++++++++++
> target-ppc/mmu-hash64.c | 70 +++++++++++++++++++++++++++++++++++++++----------
> target-ppc/mmu-hash64.h | 33 ++++++++++++++++-------
> 5 files changed, 170 insertions(+), 40 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f10ba8a..96348e3 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -52,6 +52,7 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> sPAPREnvironment *spapr,
> target_ulong raddr;
> target_ulong i;
> hwaddr hpte;
> + struct ppc_hash64_hpte_token *token;
>
> /* only handle 4k and 16M pages for now */
> if (pteh & HPTE64_V_LARGE) {
> @@ -94,25 +95,32 @@ static target_ulong h_enter(PowerPCCPU *cpu,
> sPAPREnvironment *spapr,
> if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
> return H_PARAMETER;
> }
> +
> + i = 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) {
> + token = ppc_hash64_start_access(ppc_env_get_cpu(env), pte_index);
> + do {
Why convert this into a while loop?
> if (i == 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(token, i) & HPTE64_V_VALID) == 0) {
> break;
> }
> - hpte += HASH_PTE_SIZE_64;
> - }
> + } while (i++);
> + 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(ppc_env_get_cpu(env), pte_index);
> + if (ppc_hash64_load_hpte0(token, 0) & HPTE64_V_VALID) {
> + ppc_hash64_stop_access(token);
> return H_PTEG_FULL;
> }
> + ppc_hash64_stop_access(token);
> }
> + hpte += i * HASH_PTE_SIZE_64;
> +
> ppc_hash64_store_hpte1(env, hpte, ptel);
> /* eieio(); FIXME: need some sort of barrier for smp? */
> ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
> @@ -135,15 +143,16 @@ static RemoveResult remove_hpte(CPUPPCState *env,
> target_ulong ptex,
> {
> hwaddr hpte;
> target_ulong v, r, rb;
> + struct ppc_hash64_hpte_token *token;
>
> 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(token, 0);
> + r = ppc_hash64_load_hpte1(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);
> @@ -261,15 +271,16 @@ static target_ulong h_protect(PowerPCCPU *cpu,
> sPAPREnvironment *spapr,
> target_ulong avpn = args[2];
> hwaddr hpte;
> target_ulong v, r, rb;
> + struct ppc_hash64_hpte_token *token;
>
> 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(ppc_env_get_cpu(env), pte_index);
> + v = ppc_hash64_load_hpte0(token, 0);
> + r = ppc_hash64_load_hpte1(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 ac70efbf..3ad3283 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 = {
> @@ -1888,3 +1893,45 @@ int kvm_arch_on_sigbus(int code, void *addr)
> void kvm_arch_init_irq_routing(KVMState *s)
> {
> }
> +
> +int kvm_ppc_hash64_start_access(PowerPCCPU *cpu, unsigned long pte_index,
> + struct ppc_hash64_hpte_token *token)
> +{
> + int htab_fd;
> + int hpte_group_size;
> + struct kvm_get_htab_fd ghf;
> + struct kvm_get_htab_buf {
> + struct kvm_get_htab_header header;
> + /*
> + * We required one extra byte for read
> + */
> + unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
> + } hpte_buf;;
Double semicolon?
> +
> + ghf.flags = 0;
> + ghf.start_index = pte_index;
> + htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
> + if (htab_fd < 0) {
> + goto error_out;
> + }
> + memset(&hpte_buf, 0, sizeof(hpte_buf));
> + /*
> + * Read the hpte group
> + */
> + if (read(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
> + goto out_close;
> + }
> +
> + hpte_group_size = sizeof(unsigned long) * 2 * HPTES_PER_GROUP;
> + token->hpte_group = g_malloc(hpte_group_size);
> + token->free_hpte_group = 1;
> + token->external = 1;
> + memcpy(token->hpte_group, hpte_buf.hpte, hpte_group_size);
> + close(htab_fd);
> + return 0;
> +
> +out_close:
> + close(htab_fd);
> +error_out:
> + return -1;
> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4ae7bf2..31df330 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -12,6 +12,7 @@
> #define TYPE_HOST_POWERPC_CPU "host-" TYPE_POWERPC_CPU
>
> void kvmppc_init(void);
> +struct ppc_hash64_hpte_token;
>
> #ifdef CONFIG_KVM
>
> @@ -38,10 +39,13 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned
> int hash_shift);
> #endif /* !CONFIG_USER_ONLY */
> int kvmppc_fixup_cpu(PowerPCCPU *cpu);
> bool kvmppc_has_cap_epr(void);
> +bool kvmppc_has_cap_htab_fd(void);
> 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);
> +int kvm_ppc_hash64_start_access(PowerPCCPU *cpu, hwaddr hash,
> + struct ppc_hash64_hpte_token *token);
>
> #else
>
> @@ -164,6 +168,11 @@ static inline bool kvmppc_has_cap_epr(void)
> return false;
> }
>
> +static inline bool kvmppc_has_cap_htab_fd(void)
> +{
> + return false;
> +}
> +
> static inline int kvmppc_get_htab_fd(bool write)
> {
> return -1;
> @@ -181,6 +190,13 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f,
> int fd, uint32_t index,
> abort();
> }
>
> +static inline int kvm_ppc_hash64_start_access(PowerPCCPU *cpu,
> + unsigned long pte_index,
> + struct ppc_hash64_hpte_token *token)
> +{
> + abort();
> +}
> +
> #endif
>
> #ifndef CONFIG_KVM
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 67fc1b5..aeb4593 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -302,29 +302,73 @@ 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,
> +struct ppc_hash64_hpte_token *ppc_hash64_start_access(PowerPCCPU *cpu,
> + unsigned long
> pte_index)
How about you also pass in the number of PTEs you want to access? Let's call it
"pte_num" for now. Then if you only care about one PTE you can indicate so,
otherwise it's clear that you want to access 8 PTEs beginning from the one
you're pointing at.
> +{
> + hwaddr pte_offset;
> + struct ppc_hash64_hpte_token *token;
void *token = NULL;
if (kvmppc_uses_htab_fd(cpu)) {
/* HTAB is controlled by KVM. Fetch the PTEG into a new buffer. */
int hpte_group_size = sizeof(unsigned long) * 2 * pte_num;
token = g_malloc(hpte_group_size);
if (kvm_ppc_hash64_read_pteg(cpu, pte_index, token)) {
free(token);
return NULL;
}
} else {
/* HTAB is controlled by QEMU. Just point to the internally accessible
PTEG. */
hwaddr pte_offset;
pte_offset = pte_index * HASH_PTE_SIZE_64;
if (cpu->env.external_htab) {
token = cpu->env.external_htab + pte_offset;
} else {
token = (uint8_t *) cpu->env.htab_base + pte_offset;
}
}
return token;
This way it's more obvious which path the "normal code flow" would be. We also
only clearly choose what to do depending on in-kernel HTAB or now. As a big
plus we don't need a struct that we need to dynamically allocate even in the
TCG case (malloc is slow).
You also want to cache kvmppc_uses_htab_fd(cpu) so it doesn't have to call an
ioctl every time we try to read a PTE. I guess the best spot for it would be to
put it into env.
> +
> + token = g_malloc(sizeof(struct ppc_hash64_hpte_token));
> +
> + pte_offset = pte_index * HASH_PTE_SIZE_64;
> + if (!kvmppc_has_cap_htab_fd()) {
> + token->free_hpte_group = 0;
> + token->external = !!cpu->env.external_htab;
> + if (token->external) {
> + token->hpte_group = cpu->env.external_htab + pte_offset;
> + } else {
> + token->hpte_group = (uint8_t *) cpu->env.htab_base + pte_offset;
> + }
> + return token;
> + }
> +
> + if (!kvm_ppc_hash64_start_access(cpu, pte_index, token)) {
> + return token;
> + }
> + free(token);
> + return NULL;
> +}
> +
> +void ppc_hash64_stop_access(struct ppc_hash64_hpte_token *token)
> +{
Pass in cpu as well, then simply only do
if (kvmppc_uses_htab_fd(cpu)) {
g_free(token);
}
> + if (token->free_hpte_group) {
> + free(token->hpte_group);
> + }
> + free(token);
> + return;
> +}
> +
> +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;
> - target_ulong pte0, pte1;
> int i;
> + target_ulong pte0, pte1;
> + unsigned long pte_index;
> + struct ppc_hash64_hpte_token *token;
>
> + pte_index = (hash * HPTES_PER_GROUP) & env->htab_mask;
> + token = ppc_hash64_start_access(ppc_env_get_cpu(env), pte_index);
> + if (!token) {
> + return -1;
> + }
> for (i = 0; i < HPTES_PER_GROUP; i++) {
> - pte0 = ppc_hash64_load_hpte0(env, pte_offset);
> - pte1 = ppc_hash64_load_hpte1(env, pte_offset);
> + pte0 = ppc_hash64_load_hpte0(token, i);
> + pte1 = ppc_hash64_load_hpte1(token, i);
>
> if ((pte0 & HPTE64_V_VALID)
> && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
> && HPTE64_V_COMPARE(pte0, ptem)) {
> pte->pte0 = pte0;
> pte->pte1 = pte1;
> - return pte_offset;
> + ppc_hash64_stop_access(token);
> + return (pte_index + i) * HASH_PTE_SIZE_64;
> }
> -
> - pte_offset += HASH_PTE_SIZE_64;
> }
> -
> + ppc_hash64_stop_access(token);
> + /*
> + * We didn't find a valid entry.
> + */
> return -1;
> }
>
> @@ -332,7 +376,7 @@ 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 +411,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 +420,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;
> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
> index 55f5a23..0b44321 100644
> --- a/target-ppc/mmu-hash64.h
> +++ b/target-ppc/mmu-hash64.h
> @@ -75,23 +75,36 @@ int ppc_hash64_handle_mmu_fault(CPUPPCState *env,
> target_ulong address, int rw,
> #define HPTE64_V_1TB_SEG 0x4000000000000000ULL
> #define HPTE64_V_VRMA_MASK 0x4001ffffff000000ULL
>
> -static inline target_ulong ppc_hash64_load_hpte0(CPUPPCState *env,
> - hwaddr pte_offset)
> +
> +struct ppc_hash64_hpte_token {
> + bool free_hpte_group;
> + bool external;
> + uint8_t *hpte_group;
> +};
> +
> +struct ppc_hash64_hpte_token *ppc_hash64_start_access(PowerPCCPU *cpu,
> + unsigned long
> pte_index);
> +void ppc_hash64_stop_access(struct ppc_hash64_hpte_token *token);
> +
> +static inline target_ulong ppc_hash64_load_hpte0(struct
> ppc_hash64_hpte_token *token,
> + int index)
If you pass in env or cpu in here as well you can do the decision whether to
use ldq_p or ldq_phys based on cpu->env.external_htab. Whenever the HTAB is
controlled by KVM, external_htab is true as well.
Alex
> {
> - if (env->external_htab) {
> - return ldq_p(env->external_htab + pte_offset);
> + index *= HASH_PTE_SIZE_64;
> + if (token->external) {
> + return ldq_p(token->hpte_group + index);
> } else {
> - return ldq_phys(env->htab_base + pte_offset);
> + return ldq_phys((uint64_t)(token->hpte_group + index));
> }
> }
>
> -static inline target_ulong ppc_hash64_load_hpte1(CPUPPCState *env,
> - hwaddr pte_offset)
> +static inline target_ulong ppc_hash64_load_hpte1(struct
> ppc_hash64_hpte_token *token,
> + int index)
> {
> - if (env->external_htab) {
> - return ldq_p(env->external_htab + pte_offset + HASH_PTE_SIZE_64/2);
> + index = index * HASH_PTE_SIZE_64;
> + if (token->external) {
> + return ldq_p(token->hpte_group + index + HASH_PTE_SIZE_64/2);
> } else {
> - return ldq_phys(env->htab_base + pte_offset + HASH_PTE_SIZE_64/2);
> + return ldq_phys((uint64_t)(token->hpte_group + index +
> HASH_PTE_SIZE_64/2));
> }
> }
>
> --
> 1.8.1.2
>