Re: [Qemu-devel] [PATCH -V5] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf]

On 06.11.2013, at 18:08, Aneesh Kumar K.V <address@hidden> wrote:

> Alexander Graf <address@hidden> writes:
> 
>> On 11.10.2013, at 09:58, Aneesh Kumar K.V <address@hidden> wrote:
>> 
>>> Alexander Graf <address@hidden> writes:
>>> 
>>>> 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
>>> 
>>> ....
>>> 
>>>>> +
>>>>> +    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

Just make it a different, separate variable. That way it's obvious that we're 
using it beyond the scope of the 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.

We don't need to cache the fd itself, only the fact that the cap is actually 
available and not just potentially available.


Alex