From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54264) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ve6bE-00029I-Es for qemu-devel@nongnu.org; Wed, 06 Nov 2013 12:09:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ve6ay-0007Tz-N2 for qemu-devel@nongnu.org; Wed, 06 Nov 2013 12:08:58 -0500 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:48878) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ve6ay-0007SZ-2S for qemu-devel@nongnu.org; Wed, 06 Nov 2013 12:08:44 -0500 Received: from /spool/local by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 7 Nov 2013 03:08:39 +1000 From: "Aneesh Kumar K.V" In-Reply-To: <33725426-3675-4C1B-B82C-0063B835F565@suse.de> References: <1381490016-13557-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <122B2EF8-01BF-4B89-B5AE-C9A1662A7BA1@suse.de> <87li1zn3rf.fsf@linux.vnet.ibm.com> <33725426-3675-4C1B-B82C-0063B835F565@suse.de> Date: Wed, 06 Nov 2013 22:38:22 +0530 Message-ID: <87zjphsbjt.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH -V5] 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: "open list:PReP" , Paul Mackerras , "qemu-devel@nongnu.org Developers" Alexander Graf writes: > On 11.10.2013, at 09:58, Aneesh Kumar K.V wrote: > >> Alexander Graf writes: >> >>> On 11.10.2013, at 13:13, 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 V4: >>>> * Rewrite to avoid two code paths for doing hash lookups >> >> .... >> >>>> + >>>> + 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? >> >> I am moving i = 0 outside the loop. Hence found while () better than for(;;++i) > > Outside of what loop? You're only moving it outside of the if(). I meant moving i = 0 outside of for loop initialization part here is how it would look like with for i = 0; if () for(;;i++) { if () { break; } } } else { } hpte += i * HASH_PTE_SIZE_64; I found do { } while (i++) better than for 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); >> >> .... .... >>> 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. >> >> didn't get that. We already cache that value as >> >> bool kvmppc_has_cap_htab_fd(void) >> { >> return cap_htab_fd; >> } >> >> Looking at the kernel capability check I do find an issue, So with both >> hv and pr loaded as module, that would always return true. Now that is >> not we want here. Ideally we should have all these as VM ioctl and not >> device ioctl. I had asked that as a fixme in the HV PR patchset. > > As you've realized we only cache the ability for an htab fd, not whether we are actually using one. That needs to be a separate variable. > As mentioned earlier we can't really cache the htab fd, because the interface doesn't support random reads of hash page table. -aneesh