* [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base @ 2018-04-13 23:43 Chris Co 2018-04-16 10:44 ` Leif Lindholm 0 siblings, 1 reply; 8+ messages in thread From: Chris Co @ 2018-04-13 23:43 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: Chris Co, Leif Lindholm, Ard Biesheuvel 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.lindholm@linaro.org> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Christopher Co <christopher.co@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); // Clean/invalidate the cache for this section, but only // if we are modifying the memory type attributes -- 2.15.1.gvfs.2.39.g03d366a ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base 2018-04-13 23:43 [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base Chris Co @ 2018-04-16 10:44 ` Leif Lindholm 2018-04-16 19:45 ` Chris Co 0 siblings, 1 reply; 8+ messages in thread From: Leif Lindholm @ 2018-04-16 10:44 UTC (permalink / raw) To: Chris Co; +Cc: edk2-devel@lists.01.org, Ard Biesheuvel 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.lindholm@linaro.org> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Christopher Co <christopher.co@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 > > // Clean/invalidate the cache for this section, but only > // if we are modifying the memory type attributes > -- > 2.15.1.gvfs.2.39.g03d366a > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base 2018-04-16 10:44 ` Leif Lindholm @ 2018-04-16 19:45 ` Chris Co 2018-04-19 12:30 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Chris Co @ 2018-04-16 19:45 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Ard Biesheuvel Hi Leif, > -----Original Message----- > From: Leif Lindholm <leif.lindholm@linaro.org> > Sent: Monday, April 16, 2018 3:44 AM > To: Chris Co <Christopher.Co@microsoft.com> > Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@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.lindholm@linaro.org> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Christopher Co <christopher.co@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. Thanks, Chris > > > > // Clean/invalidate the cache for this section, but only > > // if we are modifying the memory type attributes > > -- > > 2.15.1.gvfs.2.39.g03d366a > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base 2018-04-16 19:45 ` Chris Co @ 2018-04-19 12:30 ` Ard Biesheuvel 2018-06-19 20:52 ` Chris Co 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2018-04-19 12:30 UTC (permalink / raw) To: Chris Co; +Cc: Leif Lindholm, edk2-devel@lists.01.org On 16 April 2018 at 21:45, Chris Co <Christopher.Co@microsoft.com> wrote: > Hi Leif, > >> -----Original Message----- >> From: Leif Lindholm <leif.lindholm@linaro.org> >> Sent: Monday, April 16, 2018 3:44 AM >> To: Chris Co <Christopher.Co@microsoft.com> >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@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.lindholm@linaro.org> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> > Signed-off-by: Christopher Co <christopher.co@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.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base 2018-04-19 12:30 ` Ard Biesheuvel @ 2018-06-19 20:52 ` Chris Co 2018-06-20 16:39 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Chris Co @ 2018-06-19 20:52 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Leif Lindholm, edk2-devel@lists.01.org Hi, Just checking if there is anything needed on my end to get this patch merged in. Chris > -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Sent: Thursday, April 19, 2018 5:30 AM > To: Chris Co <Christopher.Co@microsoft.com> > Cc: Leif Lindholm <leif.lindholm@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.Co@microsoft.com> wrote: > > Hi Leif, > > > >> -----Original Message----- > >> From: Leif Lindholm <leif.lindholm@linaro.org> > >> Sent: Monday, April 16, 2018 3:44 AM > >> To: Chris Co <Christopher.Co@microsoft.com> > >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel > >> <ard.biesheuvel@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.lindholm@linaro.org> > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > Contributed-under: TianoCore Contribution Agreement 1.1 > >> > Signed-off-by: Christopher Co <christopher.co@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.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base 2018-06-19 20:52 ` Chris Co @ 2018-06-20 16:39 ` Ard Biesheuvel 2018-06-20 19:09 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2018-06-20 16:39 UTC (permalink / raw) To: Chris Co; +Cc: Leif Lindholm, edk2-devel@lists.01.org On 19 June 2018 at 22:52, Chris Co <Christopher.Co@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.biesheuvel@linaro.org> >> Sent: Thursday, April 19, 2018 5:30 AM >> To: Chris Co <Christopher.Co@microsoft.com> >> Cc: Leif Lindholm <leif.lindholm@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.Co@microsoft.com> wrote: >> > Hi Leif, >> > >> >> -----Original Message----- >> >> From: Leif Lindholm <leif.lindholm@linaro.org> >> >> Sent: Monday, April 16, 2018 3:44 AM >> >> To: Chris Co <Christopher.Co@microsoft.com> >> >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel >> >> <ard.biesheuvel@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.lindholm@linaro.org> >> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> >> > Signed-off-by: Christopher Co <christopher.co@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.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base 2018-06-20 16:39 ` Ard Biesheuvel @ 2018-06-20 19:09 ` Ard Biesheuvel 2018-06-21 14:11 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2018-06-20 19:09 UTC (permalink / raw) To: Chris Co; +Cc: Leif Lindholm, edk2-devel@lists.01.org On 20 June 2018 at 18:39, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 19 June 2018 at 22:52, Chris Co <Christopher.Co@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. >>> -----Original Message----- >>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Sent: Thursday, April 19, 2018 5:30 AM >>> To: Chris Co <Christopher.Co@microsoft.com> >>> Cc: Leif Lindholm <leif.lindholm@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.Co@microsoft.com> wrote: >>> > Hi Leif, >>> > >>> >> -----Original Message----- >>> >> From: Leif Lindholm <leif.lindholm@linaro.org> >>> >> Sent: Monday, April 16, 2018 3:44 AM >>> >> To: Chris Co <Christopher.Co@microsoft.com> >>> >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel >>> >> <ard.biesheuvel@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.lindholm@linaro.org> >>> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> >> > Contributed-under: TianoCore Contribution Agreement 1.1 >>> >> > Signed-off-by: Christopher Co <christopher.co@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.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base 2018-06-20 19:09 ` Ard Biesheuvel @ 2018-06-21 14:11 ` Ard Biesheuvel 0 siblings, 0 replies; 8+ messages in thread From: Ard Biesheuvel @ 2018-06-21 14:11 UTC (permalink / raw) To: Chris Co; +Cc: Leif Lindholm, edk2-devel@lists.01.org On 20 June 2018 at 21:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 20 June 2018 at 18:39, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 19 June 2018 at 22:52, Chris Co <Christopher.Co@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.biesheuvel@linaro.org> >>>> Sent: Thursday, April 19, 2018 5:30 AM >>>> To: Chris Co <Christopher.Co@microsoft.com> >>>> Cc: Leif Lindholm <leif.lindholm@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.Co@microsoft.com> wrote: >>>> > Hi Leif, >>>> > >>>> >> -----Original Message----- >>>> >> From: Leif Lindholm <leif.lindholm@linaro.org> >>>> >> Sent: Monday, April 16, 2018 3:44 AM >>>> >> To: Chris Co <Christopher.Co@microsoft.com> >>>> >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel >>>> >> <ard.biesheuvel@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.lindholm@linaro.org> >>>> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> >> > Contributed-under: TianoCore Contribution Agreement 1.1 >>>> >> > Signed-off-by: Christopher Co <christopher.co@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.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-21 14:11 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-04-13 23:43 [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base Chris Co 2018-04-16 10:44 ` Leif Lindholm 2018-04-16 19:45 ` Chris Co 2018-04-19 12:30 ` Ard Biesheuvel 2018-06-19 20:52 ` Chris Co 2018-06-20 16:39 ` Ard Biesheuvel 2018-06-20 19:09 ` Ard Biesheuvel 2018-06-21 14:11 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox