From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqeIU-0002SC-M4 for qemu-devel@nongnu.org; Mon, 26 Oct 2015 05:42:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqeIQ-0000V0-Ha for qemu-devel@nongnu.org; Mon, 26 Oct 2015 05:42:34 -0400 Received: from mail-bl2on0054.outbound.protection.outlook.com ([65.55.169.54]:62898 helo=na01-bl2-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqeIQ-0000Uk-B8 for qemu-devel@nongnu.org; Mon, 26 Oct 2015 05:42:30 -0400 Date: Mon, 26 Oct 2015 10:42:24 +0100 From: "Edgar E. Iglesias" Message-ID: <20151026094224.GB3751@toto> References: <1444863346-9711-1-git-send-email-edgar.iglesias@gmail.com> <1444863346-9711-7-git-send-email-edgar.iglesias@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v4 06/13] target-arm: Add computation of starting level for S2 PTW List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , Alexander Graf , Sergey Fedorov , Laurent Desnogues , "Edgar E. Iglesias" , Alex =?iso-8859-1?Q?Benn=E9e?= On Fri, Oct 23, 2015 at 05:26:52PM +0100, Peter Maydell wrote: > On 14 October 2015 at 23:55, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" > > > > The starting level for S2 pagetable walks is computed > > differently from the S1 starting level. Implement the S2 > > variant. > > > > Signed-off-by: Edgar E. Iglesias > > --- > > target-arm/helper.c | 117 +++++++++++++++++++++++++++++++++++++++++++------ > > target-arm/internals.h | 25 +++++++++++ > > 2 files changed, 129 insertions(+), 13 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 79b4c03..8530f7e 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -6406,12 +6406,72 @@ typedef enum { > > permission_fault = 3, > > } MMUFaultType; > > > > +/* > > + * check_s2_startlevel > > + * @cpu: ARMCPU > > + * @is_aa64: True if the translation regime is in AArch64 state > > + * @startlevel: Suggested starting level > > + * @inputsize: Bitsize of IPAs > > + * @stride: Page-table stride (See the ARM ARM) > > + * > > + * Returns true if the suggested starting level is OK and false otherwise. > > + */ > > +static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int startlevel, > > + int inputsize, int stride) > > +{ > > + /* Negative levels are never allowed. */ > > + if (startlevel < 0) { > > + return false; > > + } > > + > > + if (is_aa64) { > > + unsigned int pamax = arm_pamax(cpu); > > + > > + switch (stride) { > > + case 13: /* 64KB Pages. */ > > + if (startlevel < 1 || (startlevel == 0 && pamax <= 42)) { > > + return false; > > + } > > I'm having trouble matching these up with the ARM ARM pseudocode, > which says for this case for instance > if (level == 0 || (level == 1 && PAMax() <= 42)) then basefound = FALSE; Not sure what I was thinking... I've rewritten all of it to match the specs (I hope). > > (as an aside, the pseudocode uses 'startlevel' for the raw SL0 > field value and 'level' for the 3 - startlevel (or 2 - startlevel) > value, so it's a bit confusing to use startlevel for both here.) Yes, I've changed the naming of to better match specs. Thanks! Edgar > > > + break; > > + case 11: /* 16KB Pages. */ > > + if (startlevel < 1 || (startlevel == 0 && pamax <= 40)) { > > + return false; > > + } > > + break; > > + case 9: /* 4KB Pages. */ > > + if (startlevel == 0 && pamax <= 42) { > > + return false; > > + } > > + break; > > + default: > > + g_assert_not_reached(); > > + } > > + } else { > > + const int grainsize = stride + 3; > > + int startsizecheck; > > + > > + /* AArch32 only supports 4KB pages. Assert on that. */ > > + assert(stride == 9); > > + > > + if (startlevel == 0) { > > + return false; > > + } > > + > > + startsizecheck = inputsize - ((3 - startlevel) * stride + grainsize); > > + if (startsizecheck < 1 || startsizecheck > stride + 4) { > > + return false; > > + } > > + } > > + return true; > > +} > > + > > static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > int access_type, ARMMMUIdx mmu_idx, > > hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot, > > target_ulong *page_size_ptr, uint32_t *fsr) > > { > > - CPUState *cs = CPU(arm_env_get_cpu(env)); > > + ARMCPU *cpu = arm_env_get_cpu(env); > > + CPUState *cs = CPU(cpu); > > /* Read an LPAE long-descriptor translation table. */ > > MMUFaultType fault_type = translation_fault; > > uint32_t level = 1; > > @@ -6560,18 +6620,49 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > > goto do_fault; > > } > > > > - /* The starting level depends on the virtual address size (which can be > > - * up to 48 bits) and the translation granule size. It indicates the number > > - * of strides (stride bits at a time) needed to consume the bits > > - * of the input address. In the pseudocode this is: > > - * level = 4 - RoundUp((inputsize - grainsize) / stride) > > - * where their 'inputsize' is our 'inputsize', 'grainsize' is > > - * our 'stride + 3' and 'stride' is our 'stride'. > > - * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying: > > - * = 4 - (inputsize - stride - 3 + stride - 1) / stride > > - * = 4 - (inputsize - 4) / stride; > > - */ > > - level = 4 - (inputsize - 4) / stride; > > + if (mmu_idx != ARMMMUIdx_S2NS) { > > + /* The starting level depends on the virtual address size (which can > > + * be up to 48 bits) and the translation granule size. It indicates > > + * the number of strides (stride bits at a time) needed to > > + * consume the bits of the input address. In the pseudocode this is: > > + * level = 4 - RoundUp((inputsize - grainsize) / stride) > > + * where their 'inputsize' is our 'inputsize', 'grainsize' is > > + * our 'stride + 3' and 'stride' is our 'stride'. > > + * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying: > > + * = 4 - (inputsize - stride - 3 + stride - 1) / stride > > + * = 4 - (inputsize - 4) / stride; > > + */ > > + level = 4 - (inputsize - 4) / stride; > > + } else { > > + /* For stage 2 translations the starting level is specified by the > > + * VCTR_EL2.SL0 field (whose interpretation depends on the page size) > > VTCR_EL2. > > > + */ > > + int startlevel = extract32(tcr->raw_tcr, 6, 2); > > + bool ok; > > + > > + if (va_size == 32 || stride == 9) { > > + /* AArch32 or 4KB pages */ > > + startlevel = 2 - startlevel; > > + } else { > > + /* 16KB or 64KB pages */ > > + startlevel = 3 - startlevel; > > + } > > + > > + /* Check that the starting level is valid. */ > > + ok = check_s2_startlevel(cpu, va_size == 64, startlevel, > > + inputsize, stride); > > + if (!ok) { > > + /* AArch64 reports these as level 0 faults. > > + * AArch32 reports these as level 1 faults. > > + */ > > + level = va_size == 64 ? 0 : 1; > > + fault_type = translation_fault; > > + goto do_fault; > > + } > > + > > + /* The starting level looks good, use it. */ > > + level = startlevel; > > + } > > > > /* Clear the vaddr bits which aren't part of the within-region address, > > * so that we don't have to special case things when calculating the > > diff --git a/target-arm/internals.h b/target-arm/internals.h > > index 36a56aa..8bd37eb 100644 > > --- a/target-arm/internals.h > > +++ b/target-arm/internals.h > > @@ -152,6 +152,31 @@ static inline void update_spsel(CPUARMState *env, uint32_t imm) > > aarch64_restore_sp(env, cur_el); > > } > > > > +/* > > + * arm_pamax > > + * @cpu: ARMCPU > > + * > > + * Returns the implementation defined bit-width of physical addresses. > > + * The ARMv8 reference manuals refer to this as PAMax(). > > + */ > > +static inline unsigned int arm_pamax(ARMCPU *cpu) > > +{ > > + static const unsigned int pamax_map[] = { > > + [0] = 32, > > + [1] = 36, > > + [2] = 40, > > + [3] = 42, > > + [4] = 44, > > + [5] = 48, > > + }; > > + unsigned int parange = extract32(cpu->id_aa64mmfr0, 0, 4); > > + > > + /* id_aa64mmfr0 is a read-only register so values outside of the > > + * supported mappings can be considered an implementation error. */ > > + assert(parange < ARRAY_SIZE(pamax_map)); > > + return pamax_map[parange]; > > +} > > + > > /* Return true if extended addresses are enabled. > > * This is always the case if our translation regime is 64 bit, > > * but depends on TTBCR.EAE for 32 bit. > > -- > > 1.9.1 > > > > looks ok otherwise > > thanks > -- PMM