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:c06::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::241]) (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 AD7CF210C42AC for ; Thu, 21 Jun 2018 07:11:17 -0700 (PDT) Received: by mail-io0-x241.google.com with SMTP id l25-v6so3081284ioh.12 for ; Thu, 21 Jun 2018 07:11:17 -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=63NCDFRNeLljrCfkqSR3s5lM2LAB26zbKdJ535ex2pM=; b=AYWP+5BwO6Up4GQU8rMxXIzdfYseMUg4jPWqrAiBRkOZl2ChSIRsj1wjiQhtu4CtMN mruifSHjmUfcejmAWHPINpXrbLqqTMi+VlGMHBe4fpv+vz7241KvzCiSWJfC5glhCoqD 3l3TRFktrmACwWwNtw6Z0pObTJi8Kdnw8e13A= 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=63NCDFRNeLljrCfkqSR3s5lM2LAB26zbKdJ535ex2pM=; b=rTtk3pQ5nWsntdLSPBkMdKcND0P+PXMzLlNkvpvc/qeWWnUdGQKwAcdc4V0eeO96qv F8M8sVJClH7Pevecl1u5qf+a/IG9JAv+8OyFSE2Ap4SStxhqxL8IFDHBOgJYr3FlFT0n aVrG3p1p455jyb0tesgzl2Kuiq+6qMFD10A+5OKU6dXxhULeFmcAD5vupn78387/dNy1 9mhYcMNLC/nEpsEw7IwRHDjqZq0NAa9qTodp+Mzgq3jxhCudr/iqmiYwbRBxcNEVw6E1 qUM5pgN5ZM8+2ghAWiAWJF1W/MOCj0ksevQXeaX4MyUIwYfgPJI1QfsbChRrpPLSy73l oB+Q== X-Gm-Message-State: APt69E1q64VZIAJbHkrRNLSSL/G7jrP2SOm3L//aVnx8dyrg4OQMZH6z AniNsTGi2frvlj6PfhttEasx/+a1P6noieqAjUI99w== X-Google-Smtp-Source: ADUXVKLP+9+zr4E1QRUCTHsT17spOspder1LiwBSxprASxmnYbsS7dajsRChaXEmeCfswJZYfCKxeM9tOla4HobIqC8= X-Received: by 2002:a6b:6709:: with SMTP id b9-v6mr20250495ioc.170.1529590276857; Thu, 21 Jun 2018 07:11:16 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Thu, 21 Jun 2018 07:11:16 -0700 (PDT) In-Reply-To: References: <20180416104412.npzwcvl6zlrh426k@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 21 Jun 2018 16:11:16 +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: Thu, 21 Jun 2018 14:11:18 -0000 Content-Type: text/plain; charset="UTF-8" On 20 June 2018 at 21:09, Ard Biesheuvel wrote: > 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. > Now pushed as 8e586296c114f630188cfe4c76df91a1e2b7a5b2 >>>> -----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