From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44512) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeXGf-00069B-CK for qemu-devel@nongnu.org; Fri, 11 Mar 2016 19:18:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aeXGe-0001Hd-BN for qemu-devel@nongnu.org; Fri, 11 Mar 2016 19:18:53 -0500 Received: from mail-vk0-x22f.google.com ([2607:f8b0:400c:c05::22f]:34039) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeXGe-0001HR-6h for qemu-devel@nongnu.org; Fri, 11 Mar 2016 19:18:52 -0500 Received: by mail-vk0-x22f.google.com with SMTP id e185so153202540vkb.1 for ; Fri, 11 Mar 2016 16:18:51 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1022901457739899@web28h.yandex.ru> References: <1457107473-26292-1-git-send-email-afarallax@yandex.ru> <1022901457739899@web28h.yandex.ru> From: Peter Maydell Date: Sat, 12 Mar 2016 07:18:30 +0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Sorokin Cc: qemu-arm , QEMU Developers On 12 March 2016 at 06:44, Sergey Sorokin wrote: > 11.03.2016, 11:41, "Peter Maydell" : >>On 4 March 2016 at 23:04, Sergey Sorokin wrote: >>> There is a bug in ARM address translation regime with a long-descriptor >>> format. On the descriptor reading its address is formed from an index >>> which is a part of the input address. And on the first iteration this index >>> is incorrectly masked with 'grainsize' mask. But it can be wider according >>> to pseudo-code. >>> On the other hand on the iterations other than first the descriptor address >>> is formed from the previous level descriptor by masking with 'descaddrmask' >>> value. It always clears just 12 lower bits, but it must clear 'grainsize' >>> lower bits instead according to pseudo-code. >>> The patch fixes both cases. >> >>This is pretty confusing to understand -- it might help if you >>could give an example. > > According to documentation (ARMv8 ARM DDI 0487A.i J1.1.5: > aarch64/translation/walk/AArch64.TranslationTableWalk): > > bits(48) index = ZeroExtend(inputaddr:'000'); > descaddr.paddress.physicaladdress = baseaddress OR index; > > For a first iteration of the descriptor reading: > > addrselecttop = inputsize - 1; > addrselectbottom = (3-level)*stride + grainsize; > > Let's assume grainsize == 12 (so stride == 9), level == 1, inputsize == 43. > Then index is > inputaddr<42:30>:'000'; ...which is more than 9 bits, so when does this happen? I think this can only happen for the Stage-2 only concatenated translation-tables case... (I agree we have a bug here, I'm just trying to work out when it can trigger; if it's only possible for S2 page tables then it's not a visible bug yet because no CPUs have EL2 support enabled.) >>> - /* The address field in the descriptor goes up to bit 39 for ARMv7 >>> - * but up to bit 47 for ARMv8. >>> + /* The address field in the descriptor goes up to bit 39 for AArch32 >>> + * but up to bit 47 for AArch64. >>> */ >> >>This is not correct -- the descriptor field widths are as the comment >>states before your patch: >> * up to bit 39 for ARMv7 >> * up to bit 47 for ARMv8 (whether AArch32 or AArch64) >> >>See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in >>particular note the width which it uses for AddressSizeFault checks. > > I see in ARMv8 ARM DDI 0487A.i J1.2.4 > aarch32/translation/walk/AArch32.TranslationTableWalkLD: > > Before 'repeat' cycle: > baseaddress = baseregister<39:baselowerbound>:Zeros(baselowerbound); > > Inside the cycle: > baseaddress = desc<39:grainsize>:Zeros(grainsize); Yes, but this happens only after we have done the check: if !IsZero(desc<47:40>) then [take the AddressSize fault] which tells us that the descriptor field really is up to bit 48. We just haven't yet implemented the check in QEMU which will generate the AddressSize fault if the top bits are nonzero. (In contrast, in ARMv7 there really are only 40 bits there.) If you want to implement the AddressSize checks that's fine, but otherwise please leave this bit of the code alone. thanks -- PMM