Re: [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Mar 06, 2017 at 05:05:58PM +0100, Ard Biesheuvel wrote:
> >> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> >> index 4b6f4ce392b7..93980d6d12db 100644
> >> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> >> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> >> @@ -16,6 +16,7 @@
> >>  #include <Uefi.h>
> >>  #include <Chipset/ArmV7.h>
> >>  #include <Library/BaseMemoryLib.h>
> >> +#include <Library/CacheMaintenanceLib.h>
> >>  #include <Library/MemoryAllocationLib.h>
> >>  #include <Library/ArmLib.h>
> >>  #include <Library/BaseLib.h>
> >> @@ -36,6 +37,12 @@
> >>  #define ID_MMFR0_SHR_IMP_HW_COHERENT   1
> >>  #define ID_MMFR0_SHR_IGNORED         0xf
> >>
> >> +// First Level Descriptors
> >> +typedef UINT32    ARM_FIRST_LEVEL_DESCRIPTOR;
> >> +
> >> +// Second Level Descriptors
> >> +typedef UINT32    ARM_PAGE_TABLE_ENTRY;
> >> +
> >
> > Copied from ArmPkg/Drivers/CpuDxe/Arm/Mmu.c, but not deleted there.
> > Can it be, or can it be moved out into a header somewhere?
> >
> > No other comments.
> >
> 
> It is used in both places, so I'd need to put in in a header file
> 
> ArmPkg/Include/Chipset/ArmV7Mmu.h comes to mind ...

Works for me.
If you fold that in:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> >>  UINTN
> >>  EFIAPI
> >>  ArmReadIdMmfr0 (
> >> @@ -406,6 +413,367 @@ ArmConfigureMmu (
> >>    return RETURN_SUCCESS;
> >>  }
> >>
> >> +STATIC
> >> +EFI_STATUS
> >> +ConvertSectionToPages (
> >> +  IN EFI_PHYSICAL_ADDRESS  BaseAddress
> >> +  )
> >> +{
> >> +  UINT32                  FirstLevelIdx;
> >> +  UINT32                  SectionDescriptor;
> >> +  UINT32                  PageTableDescriptor;
> >> +  UINT32                  PageDescriptor;
> >> +  UINT32                  Index;
> >> +
> >> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> >> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> >> +
> >> +  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
> >> +
> >> +  // Obtain page table base
> >> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> >> +
> >> +  // Calculate index into first level translation table for start of modification
> >> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> >> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> >> +
> >> +  // Get section attributes and convert to page attributes
> >> +  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
> >> +  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
> >> +
> >> +  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
> >> +  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)AllocatePages (1);
> >> +  if (PageTable == NULL) {
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  // Write the page table entries out
> >> +  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
> >> +    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
> >> +  }
> >> +
> >> +  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
> >> +  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);
> >> +
> >> +  // Formulate page table entry, Domain=0, NS=0
> >> +  PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
> >> +
> >> +  // Write the page table entry out, replacing section entry
> >> +  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +STATIC
> >> +EFI_STATUS
> >> +UpdatePageEntries (
> >> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> +  IN UINT64                    Length,
> >> +  IN UINT64                    Attributes,
> >> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status;
> >> +  UINT32        EntryValue;
> >> +  UINT32        EntryMask;
> >> +  UINT32        FirstLevelIdx;
> >> +  UINT32        Offset;
> >> +  UINT32        NumPageEntries;
> >> +  UINT32        Descriptor;
> >> +  UINT32        p;
> >> +  UINT32        PageTableIndex;
> >> +  UINT32        PageTableEntry;
> >> +  UINT32        CurrentPageTableEntry;
> >> +  VOID          *Mva;
> >> +
> >> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> >> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> >> +
> >> +  Status = EFI_SUCCESS;
> >> +
> >> +  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> >> +  // EntryValue: values at bit positions specified by EntryMask
> >> +  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
> >> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> >> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
> >> +  } else {
> >> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> >> +  }
> >> +
> >> +  // Although the PI spec is unclear on this the GCD guarantees that only
> >> +  // one Attribute bit is set at a time, so we can safely use a switch statement
> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // map to strongly ordered
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // map to normal non-cachable
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // write through with no-allocate
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // write back (with allocate)
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> +  }
> >> +
> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> >> +  } else {
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
> >> +  }
> >> +
> >> +  // Obtain page table base
> >> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> >> +
> >> +  // Calculate number of 4KB page table entries to change
> >> +  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
> >> +
> >> +  // Iterate for the number of 4KB pages to change
> >> +  Offset = 0;
> >> +  for(p = 0; p < NumPageEntries; p++) {
> >> +    // Calculate index into first level translation table for page table value
> >> +
> >> +    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> >> +    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> >> +
> >> +    // Read the descriptor from the first level page table
> >> +    Descriptor = FirstLevelTable[FirstLevelIdx];
> >> +
> >> +    // Does this descriptor need to be converted from section entry to 4K pages?
> >> +    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
> >> +      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> >> +      if (EFI_ERROR(Status)) {
> >> +        // Exit for loop
> >> +        break;
> >> +      }
> >> +
> >> +      // Re-read descriptor
> >> +      Descriptor = FirstLevelTable[FirstLevelIdx];
> >> +    }
> >> +
> >> +    // Obtain page table base address
> >> +    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
> >> +
> >> +    // Calculate index into the page table
> >> +    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
> >> +    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
> >> +
> >> +    // Get the entry
> >> +    CurrentPageTableEntry = PageTable[PageTableIndex];
> >> +
> >> +    // Mask off appropriate fields
> >> +    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
> >> +
> >> +    // Mask in new attributes and/or permissions
> >> +    PageTableEntry |= EntryValue;
> >> +
> >> +    if (VirtualMask != 0) {
> >> +      // Make this virtual address point at a physical page
> >> +      PageTableEntry &= ~VirtualMask;
> >> +    }
> >> +
> >> +    if (CurrentPageTableEntry  != PageTableEntry) {
> >> +      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
> >> +      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
> >> +        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
> >> +        // Note assumes switch(Attributes), not ARMv7 possibilities
> >> +        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
> >> +      }
> >> +
> >> +      // Only need to update if we are changing the entry
> >> +      PageTable[PageTableIndex] = PageTableEntry;
> >> +      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
> >> +    }
> >> +
> >> +    Status = EFI_SUCCESS;
> >> +    Offset += TT_DESCRIPTOR_PAGE_SIZE;
> >> +
> >> +  } // End first level translation table loop
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +STATIC
> >> +EFI_STATUS
> >> +UpdateSectionEntries (
> >> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> +  IN UINT64                    Length,
> >> +  IN UINT64                    Attributes,
> >> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status = EFI_SUCCESS;
> >> +  UINT32        EntryMask;
> >> +  UINT32        EntryValue;
> >> +  UINT32        FirstLevelIdx;
> >> +  UINT32        NumSections;
> >> +  UINT32        i;
> >> +  UINT32        CurrentDescriptor;
> >> +  UINT32        Descriptor;
> >> +  VOID          *Mva;
> >> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> >> +
> >> +  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> >> +  // EntryValue: values at bit positions specified by EntryMask
> >> +
> >> +  // Make sure we handle a section range that is unmapped
> >> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> >> +              TT_DESCRIPTOR_SECTION_AP_MASK;
> >> +  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
> >> +
> >> +  // Although the PI spec is unclear on this the GCD guarantees that only
> >> +  // one Attribute bit is set at a time, so we can safely use a switch statement
> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // map to strongly ordered
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // map to normal non-cachable
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // write through with no-allocate
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // write back (with allocate)
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> +  }
> >> +
> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> >> +  } else {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> >> +  }
> >> +
> >> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> >> +  }
> >> +
> >> +  // obtain page table base
> >> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> >> +
> >> +  // calculate index into first level translation table for start of modification
> >> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> >> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> >> +
> >> +  // calculate number of 1MB first level entries this applies to
> >> +  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
> >> +
> >> +  // iterate through each descriptor
> >> +  for(i=0; i<NumSections; i++) {
> >> +    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
> >> +
> >> +    // has this descriptor already been coverted to pages?
> >> +    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
> >> +      // forward this 1MB range to page table function instead
> >> +      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
> >> +    } else {
> >> +      // still a section entry
> >> +
> >> +      // mask off appropriate fields
> >> +      Descriptor = CurrentDescriptor & ~EntryMask;
> >> +
> >> +      // mask in new attributes and/or permissions
> >> +      Descriptor |= EntryValue;
> >> +      if (VirtualMask != 0) {
> >> +        Descriptor &= ~VirtualMask;
> >> +      }
> >> +
> >> +      if (CurrentDescriptor  != Descriptor) {
> >> +        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> >> +        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
> >> +          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
> >> +          // Note assumes switch(Attributes), not ARMv7 possabilities
> >> +          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
> >> +        }
> >> +
> >> +        // Only need to update if we are changing the descriptor
> >> +        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
> >> +        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
> >> +      }
> >> +
> >> +      Status = EFI_SUCCESS;
> >> +    }
> >> +  }
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +EFI_STATUS
> >> +ArmSetMemoryAttributes (
> >> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> +  IN UINT64                    Length,
> >> +  IN UINT64                    Attributes,
> >> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status;
> >> +  UINT64        ChunkLength;
> >> +  BOOLEAN       FlushTlbs;
> >> +
> >> +  FlushTlbs = FALSE;
> >> +  while (Length > 0) {
> >> +    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> >> +        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
> >> +
> >> +      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
> >> +
> >> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> >> +        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
> >> +        BaseAddress, ChunkLength, Attributes));
> >> +
> >> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> >> +                 VirtualMask);
> >> +
> >> +      FlushTlbs = TRUE;
> >> +    } else {
> >> +
> >> +      //
> >> +      // Process page by page until the next section boundary, but only if
> >> +      // we have more than a section's worth of area to deal with after that.
> >> +      //
> >> +      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
> >> +                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
> >> +      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
> >> +        ChunkLength = Length;
> >> +      }
> >> +
> >> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> >> +        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
> >> +        BaseAddress, ChunkLength, Attributes));
> >> +
> >> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> >> +                 VirtualMask);
> >> +    }
> >> +
> >> +    if (EFI_ERROR (Status)) {
> >> +      break;
> >> +    }
> >> +
> >> +    BaseAddress += ChunkLength;
> >> +    Length -= ChunkLength;
> >> +  }
> >> +
> >> +  if (FlushTlbs) {
> >> +    ArmInvalidateTlb ();
> >> +  }
> >> +  return Status;
> >> +}
> >> +
> >>  RETURN_STATUS
> >>  ArmSetMemoryRegionNoExec (
> >>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> --
> >> 2.7.4
> >>