From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::242; helo=mail-it0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2C6A2211B7F7A for ; Wed, 20 Jun 2018 12:09:39 -0700 (PDT) Received: by mail-it0-x242.google.com with SMTP id n7-v6so1202032itn.1 for ; Wed, 20 Jun 2018 12:09:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=yYzpW6CRnD+o/uqzypVn1guxZS9JXdjQ5FlYnzjAS1Y=; b=gryUIdqEsXhmnU0S/u59/qzsqmgU+L2THzNrFbBpAkJusxFFoEuKqqb3w4SnTFHxO8 ZLvk11QRYM7Ekzi8axSqQQasLHcf9Do8i72O4fbEMtHp7QDG9ZNOIEHHjHvqngRpHx7D YLvhMOyQaVY+ihq7SnK4wWhoPL2EEnb2MvI8M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=yYzpW6CRnD+o/uqzypVn1guxZS9JXdjQ5FlYnzjAS1Y=; b=OgT6e4p7+yhJg51YMbbbRS1cqc4HgviZJ+3whU6q9z0SEoMkE8wtZQKpTaN7sMxt0k JuNTBjtAh/KDYcrIpY/nFjSkYZtXXEUHHV79riZW7zuGhDuYXVJ04pUDydY/iQX559Pd Ni7Qe30RxazHz3Yp384YDB1Fpcdtntdf37stHgE5MPru/jx5mBPDFVEG8fvsRCBckWAZ cCANIaXBe5SX4xQbmQOPz/EfWGYZuOHT2W68zgv1u1ptdkb9ZZPk11y0/DpsT6OxVrxg PPo3fXNhwAzcXOE/Dc7N61/X4yXk/nBAUYbATnXowuFhdX94KFBPDjtRxusoxPImaSIS xLwA== X-Gm-Message-State: APt69E2gW9Bm5DFp4S4cMq3f3FFqMEPDC9Ns1FVdvQqP7wuBE/kteniG +M19vQ1Zj86WrKDx+cYhlU+kkoB4yc8rC2T5re04uQ== X-Google-Smtp-Source: ADUXVKJ+HiMZuqBMrJjc6DnaN2VNd2a/Ap8v5wCuLO3qlwvFuTj6wwGsylD6cyP030xIXjHPJ/iSPmtnp75H0FiEh0s= X-Received: by 2002:a24:1d0e:: with SMTP id 14-v6mr2598021itj.50.1529521779184; Wed, 20 Jun 2018 12:09:39 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Wed, 20 Jun 2018 12:09:38 -0700 (PDT) In-Reply-To: References: <20180416104412.npzwcvl6zlrh426k@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 20 Jun 2018 21:09:38 +0200 Message-ID: To: Chris Co Cc: Leif Lindholm , "edk2-devel@lists.01.org" Subject: Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jun 2018 19:09:40 -0000 Content-Type: text/plain; charset="UTF-8" On 20 June 2018 at 18:39, Ard Biesheuvel wrote: > On 19 June 2018 at 22:52, Chris Co wrote: >> Hi, >> >> Just checking if there is anything needed on my end to get this patch merged in. >> > > Well, the patch looks obviously correct, but I just tested it and it > breaks ArmVirtQemu running in 32-bit mode. > > I will investigate > OK, I found the issue, it is not a hang but a very long stall. Patch incoming. >>> -----Original Message----- >>> From: Ard Biesheuvel >>> Sent: Thursday, April 19, 2018 5:30 AM >>> To: Chris Co >>> Cc: Leif Lindholm ; edk2-devel@lists.01.org >>> Subject: Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead >>> of table base >>> >>> On 16 April 2018 at 21:45, Chris Co wrote: >>> > Hi Leif, >>> > >>> >> -----Original Message----- >>> >> From: Leif Lindholm >>> >> Sent: Monday, April 16, 2018 3:44 AM >>> >> To: Chris Co >>> >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel >>> >> >>> >> Subject: Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx >>> instead >>> >> of table base >>> >> >>> >> On Fri, Apr 13, 2018 at 11:43:27PM +0000, Chris Co wrote: >>> >> > Mva address calculation should use the left-shifted current section >>> >> > index instead of the left-shifted table base address. >>> >> > >>> >> > Using the table base address here has the side-effect of >>> >> > potentially causing an access violation depending on the base address >>> value. >>> >> > >>> >> > Cc: Leif Lindholm >>> >> > Cc: Ard Biesheuvel >>> >> > Contributed-under: TianoCore Contribution Agreement 1.1 >>> >> > Signed-off-by: Christopher Co >>> >> > --- >>> >> > ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 2 +- >>> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> > >>> >> > diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >>> >> > b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >>> >> > index 774a7ccf59..9bf4ba03fd 100644 >>> >> > --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >>> >> > +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >>> >> > @@ -716,7 +716,7 @@ UpdateSectionEntries ( >>> >> > Descriptor |= EntryValue; >>> >> > >>> >> > if (CurrentDescriptor != Descriptor) { >>> >> > - Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << >>> >> TT_DESCRIPTOR_SECTION_BASE_SHIFT); >>> >> > + Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + i) << >>> >> > + TT_DESCRIPTOR_SECTION_BASE_SHIFT); >>> >> >>> >> So, this clearly looks like you've found a bug - thanks! >>> >> >>> >> But I am a little bit confused about the patch - should this not need >>> >> to incorporate the descriptor size in some way? >>> >> I.e. something like >>> >> Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + (i * sizeof(UINTN))) >>> >> << TT_DESCRIPTOR_SECTION_BASE_SHIFT); >>> >> or >>> >> ... &FirstLevelTable[FirstLevelIndex + i] ... >>> >> >>> >> ? >>> >> >>> >> Regards, >>> >> >>> >> Leif >>> >> >>> > I don't think descriptor size is needed here. >>> > >>> > My understanding is that Mva is the base address of the current section. >>> > >>> > FirstLevelidx is derived by the first section's BaseAddress >> 20. >>> > The current section index is then (FirstLevelIdx + i), which makes the >>> > base address of the current section (FirstLeveLidx + i) << 20. >>> > >>> >>> Indeed. 'Index' is a bit misleading here, given that it is the top level index into >>> the entire VA space, and so it is congruent with the virtual base address >>> itself. The use of 'FirstLevelTable' in this context is obviously incorrect, given >>> that it refers to the [physical] address of the page tables itself, not to the >>> virtual region they describe. >>> >>> Reviewed-by: Ard Biesheuvel