On 20 June 2018 at 21:09, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: > On 20 June 2018 at 18:39, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: >> 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 >> > > 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 <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