On 07.10.2013, at 15:58, Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com> wrote:
> Alexander Graf <ag...@suse.de> writes: > >> On 01.10.2013, at 03:27, Aneesh Kumar K.V wrote: >> >>> Alexander Graf <ag...@suse.de> writes: >>> >>>> On 09/05/2013 10:16 AM, Aneesh Kumar K.V wrote: >>>>> From: "Aneesh Kumar K.V"<aneesh.ku...@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. 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