From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51545) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTBKd-0004pD-V3 for qemu-devel@nongnu.org; Mon, 07 Oct 2013 09:58:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VTBKV-0001ik-NZ for qemu-devel@nongnu.org; Mon, 07 Oct 2013 09:58:43 -0400 Received: from e28smtp06.in.ibm.com ([122.248.162.6]:44015) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTBKV-0001i2-20 for qemu-devel@nongnu.org; Mon, 07 Oct 2013 09:58:35 -0400 Received: from /spool/local by e28smtp06.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 7 Oct 2013 19:28:31 +0530 From: "Aneesh Kumar K.V" In-Reply-To: References: <1378369007-958-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1378369007-958-3-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <5249B9CD.2000401@suse.de> <87txh1dbl4.fsf@linux.vnet.ibm.com> Date: Mon, 07 Oct 2013 19:28:27 +0530 Message-ID: <87k3hpnpx8.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH -V4 2/4] target-ppc: Fix page table lookup with kvm enabled List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-ppc@nongnu.org, paulus@samba.org, "qemu-devel@nongnu.org" Alexander Graf writes: > On 01.10.2013, at 03:27, Aneesh Kumar K.V wrote: > >> Alexander Graf writes: >> >>> On 09/05/2013 10:16 AM, Aneesh Kumar K.V wrote: >>>> From: "Aneesh Kumar K.V" >>>> .... >> >> 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 { > > } > > 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