Alexander Graf <ag...@suse.de> writes: > On 11.10.2013, at 09:58, Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com> > wrote: > >> Alexander Graf <ag...@suse.de> writes: >> >>> On 11.10.2013, at 13:13, Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com> >>> wrote: >>> >>>> From: "Aneesh Kumar K.V" <aneesh.ku...@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.ku...@linux.vnet.ibm.com> >>>> --- >>>> 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