Hi, Just checking if there is anything needed on my end to get this patch merged in.
Chris > -----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