Problem with Arm Mmu code in CpuDxe

Problem with Arm Mmu code in CpuDxe

[Kurt Kennett]

I am having a problem on my system (assert), and during investigation I may have found a problem with the Arm CpuDxe Mmu code that may affect all ARM platform users.

CpuDxeInitialize is the entry point, and pretty soon after entry it does:

  SyncCacheConfig (&mCpu);

This calls into:
  ArmPkg\Drivers\CpuDxe\Arm\Mmu.c

The code asserts that the Mmu is enabled, gets the memory space map, then starts to process the page tables by getting the TTBR0 base address.

  // obtain page table base
  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)(ArmGetTTBR0BaseAddress ());

  // Get the first region
  NextSectionAttributes = FirstLevelTable[0] & (TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK | TT_DESCRIPTOR_SECTION_AP_MASK);

  // iterate through each 1MB descriptor
  NextRegionBase = NextRegionLength = 0;
  for (i=0; i < TRANSLATION_TABLE_SECTION_COUNT; i++) {
    if ((FirstLevelTable[i] & TT_DESCRIPTOR_SECTION_TYPE_MASK) == TT_DESCRIPTOR_SECTION_TYPE_SECTION) {
...<snip>
    } else if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(FirstLevelTable[i])) {
      Status = SyncCacheConfigPage (
          i,FirstLevelTable[i],
          NumberOfDescriptors, MemorySpaceMap,
          &NextRegionBase,&NextRegionLength,&NextSectionAttributes);
      ASSERT_EFI_ERROR (Status);
    } else {

Note that the "NextSectionAttributes" is computed based on the assumption that the entry is indeed a section, and not a first-level pagetable entry.  The cache policy mask and ap mask have no meaning for a pagetable entry.

Assume my TTBR0 points to 0x80000000 and the very first entry in the translation table is 0x4FFEB009.  Decoding this entry gives:
  Entry is a Page table (low bits 01)
  NS is 1 (Not Secure)
  Domain is 0
  Page table base address is 0x4FFEB000

But the 'NextSectionAttributes' computed above is 0xB000 (again, which is garbage since the entry is a page table not a section).

Above, the system calls SyncCacheConfigPage, with the 'NextSectionAttributes' address, which leads to:

  // Convert SectionAttributes into PageAttributes
  NextPageAttributes =
      TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY(*NextSectionAttributes,0) |
      TT_DESCRIPTOR_CONVERT_TO_PAGE_AP(*NextSectionAttributes);

>From 0xB000, this creates page attributes which are invalid.

The code then follows:

  for (i=0; i < TRANSLATION_TABLE_PAGE_COUNT; i++) {
    if ((SecondLevelTable[i] & TT_DESCRIPTOR_PAGE_TYPE_MASK) == TT_DESCRIPTOR_PAGE_TYPE_PAGE) {
      // extract attributes (cacheability and permissions)
      PageAttributes = SecondLevelTable[i] & (TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK | TT_DESCRIPTOR_PAGE_AP_MASK);

      if (NextPageAttributes == 0) {
...<snip>
      } else if (PageAttributes != NextPageAttributes) {
        // Convert Section Attributes into GCD Attributes
        Status = PageToGcdAttributes (NextPageAttributes, &GcdAttributes);
        ASSERT_EFI_ERROR (Status);
...<snip>
      }
    } else if (NextPageAttributes != 0) {

Assume the second level descriptor is something harmless like 0x00000072.
00000072 =
  Small Page, NX clear
  C = 0, B = 0,  TEX = 001, so Outer and Inner Non-cacheable Normal Memory
 AP = 011, so Read/Write for both Priviledged and User
  Physical page address 00000000

This is at least something different from 'NextPageAttributes' (which is not zero)

This leads to "convert section attributes into gcd attributes" a call to PageToGcdAttributes with the (invalid) NextPageAttributes.

The code does:
  switch(PageAttributes & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) {

Which results in an unknown cache policy, and the function returns EFU_UNSUPPORTED.  This causes an ASSERT.

I was investigating a fix but at the same time wanted to let the mailing list chime in on whether anyone else had run into this problem or had ideas on the proper way to solve it.

K2

Re: Problem with Arm Mmu code in CpuDxe

[Ard Biesheuvel]

On 21 September 2016 at 17:09, Kurt Kennett <Kurt.Kennett@microsoft.com> wrote:
> I am having a problem on my system (assert), and during investigation I may have found a problem with the Arm CpuDxe Mmu code that may affect all ARM platform users.
>
> CpuDxeInitialize is the entry point, and pretty soon after entry it does:
>
>   SyncCacheConfig (&mCpu);
>
> This calls into:
>   ArmPkg\Drivers\CpuDxe\Arm\Mmu.c
>
> The code asserts that the Mmu is enabled, gets the memory space map, then starts to process the page tables by getting the TTBR0 base address.

Before the assert, there is a comment that says

  // This code assumes MMU is enabled and filed [sic] with section translations

I don't know if this is a reasonable thing to assume, and how you end
up violating this assumption, but it does explain why the code does
not work correctly.

What does your memory map look like?

Re: Problem with Arm Mmu code in CpuDxe

[Kurt Kennett]

The memory map is initialized via ArmConfigureMmu, which builds the initial translation tables.  This code is in ArmPkg\Library\ArmMmuLib\Arm\ArmMmuLibCore.c, and it does "FillTranslationTable()".  This occurs in PEI/SEC before MMU is first enabled.  This code does not just use sections -- it uses PopulateLevel2PageTable to create page tables for regions that are not modulo 1MB in size.

The start of my memory map has:
Base 0x00000000
Size  0x00018000
Attr ARM_MEMORY_REGION_ATTRIBUTE_DEVICE

Which is the only region that matters really.

I know the ArmMmuLib was recently changed/moved.  Perhaps this is a side effect of that change?  Can the mover of that code comment?

It seems strange that Arm's code would set itself up one way but then assume another setup.

K2

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Wednesday, September 21, 2016 10:21 AM
To: Kurt Kennett <Kurt.Kennett@microsoft.com>
Cc: edk2-devel <edk2-devel@lists.01.org>
Subject: Re: [edk2] Problem with Arm Mmu code in CpuDxe

On 21 September 2016 at 17:09, Kurt Kennett <Kurt.Kennett@microsoft.com> wrote:
> I am having a problem on my system (assert), and during investigation I may have found a problem with the Arm CpuDxe Mmu code that may affect all ARM platform users.
>
> CpuDxeInitialize is the entry point, and pretty soon after entry it does:
>
>   SyncCacheConfig (&mCpu);
>
> This calls into:
>   ArmPkg\Drivers\CpuDxe\Arm\Mmu.c
>
> The code asserts that the Mmu is enabled, gets the memory space map, then starts to process the page tables by getting the TTBR0 base address.

Before the assert, there is a comment that says

  // This code assumes MMU is enabled and filed [sic] with section translations

I don't know if this is a reasonable thing to assume, and how you end up violating this assumption, but it does explain why the code does not work correctly.

What does your memory map look like?

Re: Problem with Arm Mmu code in CpuDxe

[Kurt Kennett]

Also, if that comment was correct, why would the code directly check for a pagetable:

    } else if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(FirstLevelTable[i])) {

I think maybe the comment was out of date.

K2

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Wednesday, September 21, 2016 10:21 AM
To: Kurt Kennett <Kurt.Kennett@microsoft.com>
Cc: edk2-devel <edk2-devel@lists.01.org>
Subject: Re: [edk2] Problem with Arm Mmu code in CpuDxe

On 21 September 2016 at 17:09, Kurt Kennett <Kurt.Kennett@microsoft.com> wrote:
> I am having a problem on my system (assert), and during investigation I may have found a problem with the Arm CpuDxe Mmu code that may affect all ARM platform users.
>
> CpuDxeInitialize is the entry point, and pretty soon after entry it does:
>
>   SyncCacheConfig (&mCpu);
>
> This calls into:
>   ArmPkg\Drivers\CpuDxe\Arm\Mmu.c
>
> The code asserts that the Mmu is enabled, gets the memory space map, then starts to process the page tables by getting the TTBR0 base address.

Before the assert, there is a comment that says

  // This code assumes MMU is enabled and filed [sic] with section translations

I don't know if this is a reasonable thing to assume, and how you end up violating this assumption, but it does explain why the code does not work correctly.

What does your memory map look like?