[PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Chris Co]

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

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Leif Lindholm]

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
> 

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Chris Co]

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
> >

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Ard Biesheuvel]

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>

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Chris Co]

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>

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Ard Biesheuvel]

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>

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Ard Biesheuvel]

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>

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Ard Biesheuvel]

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>