On 19 June 2018 at 22:52, Chris Co <christopher...@microsoft.com> 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 >> -----Original Message----- >> From: Ard Biesheuvel <ard.biesheu...@linaro.org> >> Sent: Thursday, April 19, 2018 5:30 AM >> To: Chris Co <christopher...@microsoft.com> >> Cc: Leif Lindholm <leif.lindh...@linaro.org>; 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 <christopher...@microsoft.com> wrote: >> > Hi Leif, >> > >> >> -----Original Message----- >> >> From: Leif Lindholm <leif.lindh...@linaro.org> >> >> Sent: Monday, April 16, 2018 3:44 AM >> >> To: Chris Co <christopher...@microsoft.com> >> >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel >> >> <ard.biesheu...@linaro.org> >> >> 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 <leif.lindh...@linaro.org> >> >> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> >> > Signed-off-by: Christopher Co <christopher...@microsoft.com> >> >> > --- >> >> > 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 <ard.biesheu...@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel