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

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Alexander Graf <agraf@suse.de> writes:

> On 01.10.2013, at 03:27, Aneesh Kumar K.V wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 09/05/2013 10:16 AM, Aneesh Kumar K.V wrote:
>>>> From: "Aneesh Kumar K.V"<aneesh.kumar@linux.vnet.ibm.com>
>>>> 

....

>> 
>> 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.

int pteg_group;
unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];

Also out serach can be much effective with the current code, 

    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.

-aneesh