From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtsZu-0004yP-3O for qemu-devel@nongnu.org; Fri, 20 Dec 2013 00:24:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VtsZm-0000FE-A9 for qemu-devel@nongnu.org; Fri, 20 Dec 2013 00:24:50 -0500 Received: from e28smtp09.in.ibm.com ([122.248.162.9]:44604) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtsZl-0000Eo-Nk for qemu-devel@nongnu.org; Fri, 20 Dec 2013 00:24:42 -0500 Received: from /spool/local by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 20 Dec 2013 10:54:39 +0530 From: "Aneesh Kumar K.V" In-Reply-To: <8A1F1664-4165-41AF-9FAF-045E1B52C052@suse.de> References: <1383834662-12473-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1383834662-12473-3-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <472FFF53-4DE5-4138-A5D0-6466B884F358@suse.de> <87eh599vnm.fsf@linux.vnet.ibm.com> <8A1F1664-4165-41AF-9FAF-045E1B52C052@suse.de> Date: Fri, 20 Dec 2013 10:54:35 +0530 Message-ID: <87zjnw857w.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH -V7 3/3] target-ppc: Fix page table lookup with kvm enabled List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: "qemu-ppc@nongnu.org list:PowerPC" , Paul Mackerras , QEMU Developers Alexander Graf writes: > On 19.12.2013, at 07:55, Aneesh Kumar K.V wrote: > >> Alexander Graf writes: >> >>> On 07.11.2013, at 15:31, Aneesh Kumar K.V wrote: >>> >>>> From: "Aneesh Kumar K.V" >>>> >>>> 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 : Cannot access memory at address 0xc000000000098660 >>>> (gdb) >>>> >>>> Signed-off-by: Aneesh Kumar K.V >>>> --- >>>> Changes from V6: >>>> * drop htab_fd argument and use global variable kvmppc_kern_htab instead >>>> >>>> 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 | 23 ++++++++++----- >>>> 6 files changed, 183 insertions(+), 41 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>>> index d4f3502..8bf886e 100644 >>>> --- a/hw/ppc/spapr.c >>>> +++ b/hw/ppc/spapr.c >> >> ............ >> >>>> + hpte += index * HASH_PTE_SIZE_64; >>>> + >>>> ppc_hash64_store_hpte1(env, hpte, ptel); >>> >>> I'm not a big fan of fixing the read part, but leaving the write part >>> broken. However I can see value in read only already, so I'm fine if >>> the write part follows later. >> >> When do we want store support per hpte entry w.r.t to hash page table maintained by the >> hypervisor. I didn't find a use case while doing this, hence i left that >> part out. > > Well, it's mostly a matter of consistency. The only case where it ever > matters is if we wanted to emulate H_ENTER in QEMU - for tracing or > debugging purposes. But if we leave it halfway implemented like this, > you might get the impression it could work, but it doesn't. > >> >>> >>>> /* 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; >>>> } >>>> >> ..... >> >>>> #ifndef CONFIG_KVM >>>> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c >>>> index 67fc1b5..f59d199 100644 >>>> --- a/target-ppc/mmu-hash64.c >>>> +++ b/target-ppc/mmu-hash64.c >>>> @@ -41,6 +41,11 @@ >>>> #endif >>>> >>>> /* >>>> + * Used to indicate whether we have allocated htab in the >>>> + * host kernel >>>> + */ >>>> +bool kvmppc_kern_htab; >>>> +/* >>>> * SLB handling >>>> */ >>>> >>>> @@ -302,29 +307,76 @@ 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, >>>> +void *ppc_hash64_start_access(PowerPCCPU *cpu, unsigned long pte_index) >>>> +{ >>>> + void *token = NULL; >>>> + hwaddr pte_offset; >>>> + >>>> + pte_offset = pte_index * HASH_PTE_SIZE_64; >>>> + if (kvmppc_kern_htab) { >>>> + /* >>>> + * HTAB is controlled by KVM. Fetch the PTEG into a new buffer. >>>> + */ >>>> + token = kvmppc_hash64_read_pteg(cpu, pte_index); >>>> + if (token) { >>>> + return token; >>>> + } >>>> + /* >>>> + * pteg read failed, even though we have allocated htab via >>>> + * kvmppc_reset_htab. >>>> + */ >>>> + return NULL; >>>> + } >>>> + /* >>>> + * HTAB is controlled by QEMU. Just point to the internally >>>> + * accessible PTEG. >>>> + */ >>>> + if (cpu->env.external_htab) { >>>> + token = cpu->env.external_htab + pte_offset; >>>> + } else if (cpu->env.htab_base) { >>>> + token = (uint8_t *) cpu->env.htab_base + pte_offset; >>> >>> This breaks if you run a 64-bit guest on a 32-bit host trying to >>> access memory beyond 4GB. In that case htab_base is hwaddr (64bit) >>> while uint8_t is only 32bit wide. >> >> Wow!! didn't know that is possible. > > Well, with TCG you can run a 64-bit guest on a 32-bit host system, right? > >> >>> >>> Just pass a 64bit token around. That makes it safe and easy. >>> >> >> I converted that to uint64_t. How about Patch 1 and 2. Are they ok to go >> upstream ? > > Yup. It took me a while to figure out that the patches are doing "the > right thing". That code could really use some comments that explain > what's going on - like the semantics of htab_mask (mask of the PTEG, > will be shifted by 7 for physical address later), what htab_shift > really is (log2 of the HTAB size), etc :). How about /* * htab_mask is the mask used normalize hash value to PTEG index. * htab_shift is log2 of hash table size. * We have 8 hpte per group, and each hpte is 16 bytes. * ie have 128 bytes per hpte entry. */ -aneesh