From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zen1z-00025s-UM for qemu-devel@nongnu.org; Wed, 23 Sep 2015 12:36:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zen1v-0000TR-Qt for qemu-devel@nongnu.org; Wed, 23 Sep 2015 12:36:31 -0400 Received: from mail-vk0-f53.google.com ([209.85.213.53]:36480) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zen1v-0000TM-J1 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 12:36:27 -0400 Received: by vkfp126 with SMTP id p126so31200375vkf.3 for ; Wed, 23 Sep 2015 09:36:27 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1442672127-26223-3-git-send-email-edgar.iglesias@gmail.com> References: <1442672127-26223-1-git-send-email-edgar.iglesias@gmail.com> <1442672127-26223-3-git-send-email-edgar.iglesias@gmail.com> From: Peter Maydell Date: Wed, 23 Sep 2015 09:36:06 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH RFC 2/8] target-arm: Add computation of starting level for S2 PTW List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: Edgar Iglesias , Sergey Fedorov , =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Alexander Graf On 19 September 2015 at 07:15, 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 | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index b709582..33be8c2 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -6542,18 +6542,26 @@ 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 (granule_sz 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 'va_size - tsz', 'grainsize' is > - * our 'granule_sz + 3' and 'stride' is our 'granule_sz'. > - * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying: > - * = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz > - * = 4 - (va_size - tsz - 4) / granule_sz; > - */ > - level = 4 - (va_size - tsz - 4) / granule_sz; > + if (mmu_idx == ARMMMUIdx_S2NS) { > + unsigned int startlevel = extract32(tcr->raw_tcr, 6, 2); > + level = 3 - startlevel; > + if (granule_sz == 9) { > + level = 2 - startlevel; > + } I think this is right code-wise but we could make it read a little more nicely: if you make the condition be "if (mmu_idx != ARMMUIdx_S2NS)" then the common case comes first and its long comment works as a description of what we're doing here. Then the else clause can just say /* For stage 2 translations the starting level is specified by the * VCTR_EL2.SL0 field (whose interpretation depends on the page size) */ I was pondering whether writing it as if (granule_sz == 9) { /* 4K pages */ level = 2 - startlevel; } else { /* 16K or 64K pages */ level = 3 - startlevel; } would be slightly better, but it's marginal. Do add a "4K pages" comment in somewhere, though. > + } else { > + /* 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 (granule_sz 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 'va_size - tsz', 'grainsize' is > + * our 'granule_sz + 3' and 'stride' is our 'granule_sz'. > + * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying: > + * = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz > + * = 4 - (va_size - tsz - 4) / granule_sz; > + */ > + level = 4 - (va_size - tsz - 4) / granule_sz; > + } > > /* 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 > -- > 1.9.1 > thanks -- PMM