Re: [Qemu-devel] [PATCH -V4 2/4] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf]

On 07.10.2013, at 15:58, Aneesh Kumar K.V <address@hidden> wrote:

> Alexander Graf <address@hidden> writes:
> 
>> On 01.10.2013, at 03:27, Aneesh Kumar K.V wrote:
>> 
>>> Alexander Graf <address@hidden> writes:
>>> 
>>>> On 09/05/2013 10:16 AM, Aneesh Kumar K.V wrote:
>>>>> From: "Aneesh Kumar K.V"<address@hidden>
>>>>> 
> 
> ....
> 
>>> 
>>> Can you  explain this better ? 
>> 
>> You're basically doing
>> 
>> hwaddr ppc_hash64_pteg_search(...)
>> {
>>    if (kvm) {
>>        pteg = read_from_kvm();
>>        foreach pte in pteg {
>>            if (match) return offset;
>>        }
>>        return -1;
>>    } else {
>>        foreach pte in pteg {
>>            pte = read_pte_from_memory();
>>            if (match) return offset;
>>        }
>>        return -1;
>>    }
>> }
>> 
>> This is massive code duplication. The only difference between kvm and
>> tcg are the source for the pteg read. David already abstracted the
>> actual pte0/pte1 reads away in ppc_hash64_load_hpte0 and
>> ppc_hash64_load_hpte1 wrapper functions.
>> 
>> Now imagine we could add a temporary pteg store in env. Then have something 
>> like this in ppc_hash64_load_hpte0:
>> 
>> if (need_kvm_htab_access) {
>>    if (env->current_cached_pteg != this_pteg) (
>>        read_pteg(env->cached_pteg);
>>        return env->cached_pteg[x].pte0;
>>    }
>> } else {
>>    <do what was done before>
>> }
>> 
>> That way the actual resolver doesn't care where the PTEG comes from,
>> as it only ever checks pte0/pte1 and leaves all the magic on where
>> those come from to the load function.
> 
> I tried to do this and didn't like the end result. For one we
> unnecessarly bloat CPUPPCState struct to now carry a pteg information
> and associated array. ie, we need to have now the below in the CPUPPCState.

How about something like

token = ppc_hash64_start_access();
foreach (hpte entry) {
   pte0 = ppc_hash64_load_hpte0(token, ...);
   ...
}
ppc_hash64_stop_access(token);

That way you could put the buffer and pteg_group into the token struct and only 
allocate and use it when KVM with HV is in use.

> 
> int pteg_group;
> unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
> 
> Also out serach can be much effective with the current code, 

We're anything but performance critical at this point.

> 
>    while (index < hpte_buf.header.n_valid) {
> 
> against 
> 
>        for (i = 0; i < HPTES_PER_GROUP; i++) {
> 
> I guess the former is better when we can find invalid hpte entries.
> 
> We now also need to update kvm_cpu_synchronize_state to clear
> pte_group so that we would not look at the stale values. If we ever want
> to use reading pteg in any other place we could possibly look at doing
> this. But at this stage, IMHO it unnecessarily make it all complex and
> less efficient.

The point is to make it less complex. I don't like the idea of having 2 hash 
lookups in the same code base that do basically the same. And efficiency only 
ever counts in the TCG case here.


Alex