From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44119) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHNlr-0002qr-CQ for qemu-devel@nongnu.org; Thu, 07 Jan 2016 22:31:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHNlo-0007qC-5V for qemu-devel@nongnu.org; Thu, 07 Jan 2016 22:31:23 -0500 Received: from mail-pf0-x233.google.com ([2607:f8b0:400e:c00::233]:33074) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHNln-0007ob-Sh for qemu-devel@nongnu.org; Thu, 07 Jan 2016 22:31:20 -0500 Received: by mail-pf0-x233.google.com with SMTP id e65so3143149pfe.0 for ; Thu, 07 Jan 2016 19:31:19 -0800 (PST) References: <1450665670-18323-1-git-send-email-david@gibson.dropbear.id.au> <1450665670-18323-2-git-send-email-david@gibson.dropbear.id.au> From: Alexey Kardashevskiy Message-ID: <568F2D81.9090606@ozlabs.ru> Date: Fri, 8 Jan 2016 14:31:13 +1100 MIME-Version: 1.0 In-Reply-To: <1450665670-18323-2-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] ppc: Move HPTE size parsing code to target-ppc helper (and add 64kiB pages) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson , benh@kernel.crashing.org, mdroth@linux.vnet.ibm.com Cc: lvivier@redhat.com, thuth@redhat.com, qemu-ppc@nongnu.org, agraf@suse.de, qemu-devel@nongnu.org On 12/21/2015 01:41 PM, David Gibson wrote: > The h_enter() hypercall implementation in spapr_hcall.c contains some code > to determine the page size of the newly inserted hash PTE. This is > surprisingly complex in the general case, because POWER CPUs can have > different implementation dependent ways of encoding page sizes. The > current version assumes POWER7 or POWER8 and doesn't support all the s/and doesn't/doesn't/ ? > possibilities of even those (but this is advertised correctly in the device > tree and so works with guests). > > To support the possibility of future CPUs with different options, however, > this really belongs in the core ppc mmu code, rather than the spapr > ("pseries" machine type) specific code. > > So, move this code to a helper in target-ppc. > > At the same time, uncomment some code to allow 64kiB pages. At the time > that was added, I believe other parts of the ppc mmu code didn't handle > 64kiB pages, but that's since been fixed. > > Signed-off-by: David Gibson > --- > hw/ppc/spapr_hcall.c | 26 ++++++++------------------ > target-ppc/mmu-hash64.c | 31 +++++++++++++++++++++++++++++++ > target-ppc/mmu-hash64.h | 3 +++ > 3 files changed, 42 insertions(+), 18 deletions(-) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index cebceea..c346e97 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -93,28 +93,18 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPRMachineState *spapr, > target_ulong pte_index = args[1]; > target_ulong pteh = args[2]; > target_ulong ptel = args[3]; > - target_ulong page_shift = 12; > + int page_shift; > + uint64_t avpnm; > target_ulong raddr; > target_ulong index; > uint64_t token; > > - /* only handle 4k and 16M pages for now */ > - if (pteh & HPTE64_V_LARGE) { > -#if 0 /* We don't support 64k pages yet */ > - if ((ptel & 0xf000) == 0x1000) { > - /* 64k page */ > - } else > -#endif > - if ((ptel & 0xff000) == 0) { > - /* 16M page */ > - page_shift = 24; > - /* lowest AVA bit must be 0 for 16M pages */ > - if (pteh & 0x80) { > - return H_PARAMETER; > - } > - } else { > - return H_PARAMETER; > - } > + if (ppc_hash64_hpte_shift(pteh, ptel, &page_shift, &avpnm) < 0) { > + return H_PARAMETER; > + } > + > + if (HPTE64_V_AVPN_VAL(pteh) & avpnm) { HPTE64_V_AVPN_VAL(pteh) & ((1ULL << page_shift) >> 23) should do the same thing, no? > + return H_PARAMETER; > } > > raddr = (ptel & HPTE64_R_RPN) & ~((1ULL << page_shift) - 1); > diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c > index 34e20fa..d3b895d 100644 > --- a/target-ppc/mmu-hash64.c > +++ b/target-ppc/mmu-hash64.c > @@ -399,6 +399,37 @@ static uint64_t ppc_hash64_page_shift(ppc_slb_t *slb) > return epnshift; > } > > +/* Returns the page shift of the given hpte. This is the "base" > + * shift, used for hashing. Although we don't implement it yet, the > + * architecture allows the actuall mapped page to be larger, but s/actuall/actual/ > + * determining that size requires information from the slb. */ > +int ppc_hash64_hpte_shift(uint64_t pte0, uint64_t pte1, > + int *shift, uint64_t *avpnm) > +{ > + *shift = 12; /* 4kiB default */ > + > + /* only handle 4k and 16M pages for now */ > + if (pte0 & HPTE64_V_LARGE) { > + if ((pte1 & 0xf000) == 0x1000) { Nit: it would improve readability of this code if HPTE64_R_KEY()&co was used. What does define these "key" bits meaning? > + *shift = 16; /* 64 kiB */ > + } else if ((pte1 & 0xff000) == 0) { > + *shift = 24; /* 16MiB */ > + } else { > + return -EINVAL; > + } > + } > + > + if (avpnm) { > + if (*shift <= 23) { > + *avpnm = 0; > + } else { > + *avpnm = (1ULL << (*shift - 23)) - 1; > + } > + } What does "avpnm" stand for? Abbreviated virtual page mask? If you dropped "if(avpnm)", you could move "*avpnm = 0;" to the beginning and "*avpnm = (1ULL << (*shift - 23)) - 1;" next to "*shift = 24;". > + > + return 0; > +} > + > static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env, > ppc_slb_t *slb, target_ulong eaddr, > ppc_hash_pte64_t *pte) > diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h > index 291750f..71d2532 100644 > --- a/target-ppc/mmu-hash64.h > +++ b/target-ppc/mmu-hash64.h > @@ -117,6 +117,9 @@ typedef struct { > uint64_t pte0, pte1; > } ppc_hash_pte64_t; > > +int ppc_hash64_hpte_shift(uint64_t pte0, uint64_t pte1, > + int *shift, uint64_t *avpnm); > + > #endif /* CONFIG_USER_ONLY */ > > #endif /* !defined (__MMU_HASH64_H__) */ > -- Alexey