Re: [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New DP500/DP550/DP650 platform library.

[Alexei Fedorov <Alexei.Fedorov@arm.com>]

Hi Ard,


You wrote:


> > +  Status = gDS->SetMemorySpaceAttributes (
> > +                  *VramBaseAddress,
> > +                  *VramSize,
> > +                  EFI_MEMORY_WC
>
> Please add EFI_MEMORY_XP here
>


Setting EFI_MEMORY_XP causes assertion because  gDS->SetMemorySpaceAttributes() returns Unsupported in edk2\MdeModulePkg\Core\Dxe\Gcd\Gcd.c  with Status set at line #834:


      if ((Entry->Capabilities & Attributes) != Attributes) {

        //***AF
        DEBUG ((DEBUG_GCD, " @%d EFI_UNSUPPORTED: Capabilitie=0x%lX Attributes=0x%lX\n",
                __LINE__, Entry->Capabilities, Attributes));

        Status = EFI_UNSUPPORTED;
        goto Done;
      }

because Entry->Capabilities don't have EFI_MEMORY_XP set, see debug output below:


===================

 Framebuffer @0xF98A4000 0x4000000
GCD:SetMemorySpaceAttributes(Base=00000000F98A4000,Length=0000000004000000)
  Attributes  = 0000000000004002
 @834 EFI_UNSUPPORTED: Capabilities=0x800000000000000E Attributes=0x4002
  Status = Unsupported
GCDMemType Range                             Capabilities     Attributes
========== ================================= ================ ================
NonExist   0000000000000000-0000000007FFFFFF 0000000000000000 0000000000000000
MMIO       0000000008000000-000000000BFFFFFF C000000000000001 8000000000000001
NonExist   000000000C000000-000000001C16FFFF 0000000000000000 0000000000000000
MMIO       000000001C170000-000000001C170FFF C000000000000001 8000000000000001
NonExist   000000001C171000-000000004FFFFFFF 0000000000000000 0000000000000000
MMIO       0000000050000000-00000000501FFFFF C000000000000001 0000000000000001*
MMIO       0000000050200000-0000000057FFFFFF C000000000000001 0000000000000001
NonExist   0000000058000000-000000007FFFFFFF 0000000000000000 0000000000000000
SystemMem  0000000080000000-00000000FEFFFFFF 800000000000000E 0000000000000008*
NonExist   00000000FF000000-000000087FFFFFFF 0000000000000000 0000000000000000
SystemMem  0000000880000000-00000009FFFFFFFF 800000000000000E 0000000000000008*
NonExist   0000000A00000000-0000003FFFFFFFFF 0000000000000000 0000000000000000
MMIO       0000004000000000-00000040000FFFFF C000000000000001 0000000000000001*
MMIO       0000004000100000-00000040FFFFFFFF C000000000000001 0000000000000001
NonExist   0000004100000000-0000FFFFFFFFFFFF 0000000000000000 0000000000000000

ASSERT_EFI_ERROR (Status = Unsupported)
ASSERT [HdLcdGraphicsDxe] n:\edk2\ArmPlatformPkg\ArmJunoPkg\Library\HdLcdArmJunoLib\HdLcdArmJuno.c(254): !EFI_ERROR (Status)
===============

If  gDS->SetMemorySpaceAttributes() call is replaced with Cpu->SetMemoryAttributes(), attributes are set successfully with eXecute bit for frame buffer being cleared.


Alexei.

________________________________
From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Evan Lloyd <Evan.Lloyd@arm.com>
Sent: 08 January 2018 18:51:12
To: Ard Biesheuvel
Cc: "Matteo.Carlini@arm.com"@arm.com; "leif.lindholm@linaro.org"@arm.com; "nd@arm.com"@arm.com; edk2-devel@lists.01.org; Arvind Chauhan; "ard.biesheuvel@linaro.org"@arm.com; Thomas Abraham
Subject: Re: [edk2] [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New DP500/DP550/DP650 platform library.



> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 23 December 2017 16:07
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; Arvind Chauhan <Arvind.Chauhan@arm.com>;
> Daniil Egranov <Daniil.Egranov@arm.com>; Thomas Abraham
> <thomas.abraham@arm.com>; "ard.biesheuvel@linaro.org"@arm.com;
> "leif.lindholm@linaro.org"@arm.com;
> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
> Subject: Re: [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New
> DP500/DP550/DP650 platform library.
>
> On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
> > From: Girish Pathak <girish.pathak at arm.com>
> >
...
> > +
> > +/* Internal helper function to allocate memory if memory is not
> > +already
> > +  reserved for framebuffer
> > +
> > +  @param[in]  VramSize          Requested framebuffer size in bytes.
> > +  @param[out] VramBaseAddress   Pointer to memory allocated for
> framebuffer.
> > +
> > +  @retval EFI_SUCCESS           Framebuffer memory allocated successfully.
> > +  @retval EFI_UNSUPPORTED       Allocated address wider than 40 bits.
> > +  @retval !EFI_SUCCESS          Other errors.
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +GetVram (
> > +  IN  UINTN                 CONST  VramSize,
> > +  OUT EFI_PHYSICAL_ADDRESS *CONST  VramBaseAddress
> > +  )
> > +{
> > +  EFI_STATUS Status;
> > +
> > +  // Check if memory is already reserved for the framebuffer.
> > +#if (FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase) != 0)
> > +
>
> Please don't use CPP conditionals for control flow
>
> > +#if (!DP_VALID_BASE_ADDR (FixedPcdGet64
> > +(PcdArmLcdDdrFrameBufferBase))) #error ARM Mali DP framebuffer
> base address cannot be wider than 40 bits.
> > +#else
> > +  *VramBaseAddress =
> > +    (EFI_PHYSICAL_ADDRESS)FixedPcdGet64
> > +(PcdArmLcdDdrFrameBufferBase);
> > +
> > +  Status = EFI_SUCCESS;
> > +#endif
> > +
> > +#else
> > +  // If not already reserved, attempt to allocate the VRAM from the
> DRAM.
> > +  Status = gBS->AllocatePages (
> > +                  AllocateAnyPages,
> > +                  EfiBootServicesData,
> > +                  EFI_SIZE_TO_PAGES (*VramSize),
> > +                  VramBaseAddress
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "ArmMaliDpLib: Failed to allocate
> framebuffer.\n"));
> > +    ASSERT (FALSE);
> > +    return Status;
> > +  }
> > +
> > +  // ARM Mali DP framebuffer base address can not be wider than 40 bits.
> > +  if (!DP_VALID_BASE_ADDR (*VramBaseAddress)) {
> > +    gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> > +    ASSERT (DP_VALID_BASE_ADDR (*VramBaseAddress));
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  // Mark the VRAM as write-combining. The VRAM is inside the DRAM,
> > + which is  // cacheable, for ARM/AArch64 EFI_MEMORY_WC memory is
> actually uncached.
> > +  Status = gDS->SetMemorySpaceAttributes (
> > +                  *VramBaseAddress,
> > +                  *VramSize,
> > +                  EFI_MEMORY_WC
>
> Please add EFI_MEMORY_XP here
>

 [[Evan Lloyd]] We can do that, happily.  However, in looking at this we found that the UEFI spec has in "2.3.6 AArch64 Platforms", section "2.3.6.1 Memory types":
EFI_MEMORY_XP, ...                                                                             Not used or defined

Does that suggest we need a minor spec update?

> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    ASSERT (FALSE);
> > +    gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> > +  }
> > +#endif
> > +
> > +  return Status;
> > +}
> > +
> > +/** Allocate VRAM memory in DRAM for the framebuffer
> > +  (unless it is reserved already).
> > +
> > +  The allocated address can be used to set the framebuffer as a base
> > + buffer  address for any layer of the ARM Mali DP.
> > +
> > +  @param[out] VramBaseAddress     A pointer to the framebuffer
> address.
> > +  @param[out] VramSize            A pointer to the size of the frame
> > +                                  buffer in bytes
> > +
> > +  @retval EFI_SUCCESS             Frame buffer memory allocation success.
> > +  @retval EFI_UNSUPPORTED         Allocated address wider than 40 bits
> > +  @retval !EFI_SUCCESS            Other errors.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformGetVram (
> > +  OUT EFI_PHYSICAL_ADDRESS * CONST VramBaseAddress,
> > +  OUT UINTN                * CONST VramSize
> > +  )
> > +{
> > +  ASSERT (VramBaseAddress != NULL);
> > +  ASSERT (VramSize != NULL);
> > +
> > +  // Set the VRAM size.
> > +  *VramSize = (UINTN)FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize);
> > +
> > +  return GetVram (*VramSize, VramBaseAddress); }
> > +
> > +/** Return total number of modes supported.
> > +
> > +  Note: Valid mode numbers are 0 to MaxMode - 1  See Section 12.9 of
> > + the UEFI Specification 2.7
> > +
> > +  @retval UINT32             Mode Number.
> > +**/
> > +UINT32
> > +LcdPlatformGetMaxMode (VOID)
> > +{
> > +  return  mMaxMode;
> > +}
> > +
> > +/** Set the requested display mode.
> > +
> > +  @param[in] ModeNumber            Mode Number.
> > +
> > +  @retval EFI_SUCCESS              Mode set successful.
> > +  @retval EFI_INVALID_PARAMETER    Requested mode not found.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformSetMode (
> > +  IN CONST UINT32 ModeNumber
> > +  )
> > +{
> > +
> > +  if (ModeNumber >= mMaxMode) {
> > +    ASSERT (ModeNumber < mMaxMode);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  // On models, platform specific clock/mux settings are not required.
> > +  // Display controller specific settings for Mali DP are done in
> LcdSetMode.
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** Return information for the requested mode number.
> > +
> > +  @param[in]  ModeNumber          Mode Number.
> > +  @param[out] Info                Pointer for returned mode information
> > +                                  (on success).
> > +
> > +  @retval EFI_SUCCESS             Display mode information of the
> requested
> > +                                  mode returned successfully.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformQueryMode (
> > +  IN  CONST UINT32                                 ModeNumber,
> > +  OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
> > +  )
> > +{
> > +  ASSERT (ModeNumber < mMaxMode);
> > +  ASSERT (Info != NULL);
> > +
> > +  Info->Version = 0;
> > +  Info->HorizontalResolution =
> > + mDisplayModes[ModeNumber].Horizontal.Resolution;
> > +  Info->VerticalResolution =
> > + mDisplayModes[ModeNumber].Vertical.Resolution;
> > +  Info->PixelsPerScanLine =
> > + mDisplayModes[ModeNumber].Horizontal.Resolution;
> > +
> > +  Info->PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** Return the display timing information for the requested mode
> number.
> > +
> > +  @param[in]  ModeNumber         Mode Number.
> > +
> > +  @param[out] Horizontal         Pointer to horizontal timing parameters.
> > +                                 (Resolution, Sync, Back porch, Front porch)
> > +  @param[out] Vertical           Pointer to vertical timing parameters.
> > +                                 (Resolution, Sync, Back porch, Front
> > + porch)
> > +
> > +  @retval EFI_SUCCESS             Display timing information of the
> requested
> > +                                  mode returned successfully.
> > +  @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformGetTimings (
> > +  IN  UINT32                ModeNumber,
> > +  OUT CONST SCAN_TIMINGS ** Horizontal,
> > +  OUT CONST SCAN_TIMINGS ** Vertical
> > +  )
> > +{
> > +  ASSERT (Horizontal != NULL);
> > +  ASSERT (Vertical != NULL);
> > +
> > +  if (ModeNumber >= mMaxMode) {
> > +    ASSERT (ModeNumber < mMaxMode);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *Horizontal = &mDisplayModes[ModeNumber].Horizontal;
> > +  *Vertical = &mDisplayModes[ModeNumber].Vertical;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** Return bits per pixel information for a mode number.
> > +
> > +  @param[in]  ModeNumber          Mode Number.
> > +  @param[out] Bpp                 Pointer to value Bytes Per Pixel.
> > +
> > +  @retval EFI_SUCCESS             Bits per pixel information of the display
> > +                                  mode returned successfully.
> > +  @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +**/
> > +EFI_STATUS
> > +LcdPlatformGetBpp (
> > +  IN  CONST UINT32    ModeNumber,
> > +  OUT LCD_BPP * CONST Bpp
> > +  )
> > +{
> > +  ASSERT (Bpp != NULL);
> > +  if (ModeNumber >= mMaxMode) {
> > +    // Check valid ModeNumber.
> > +    ASSERT (ModeNumber < mMaxMode);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *Bpp = LCD_BITS_PER_PIXEL_24;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > diff --git
> >
> a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> >
> b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > index
> >
> 1e8f3205312ebf30fa1666130bc944261384359a..f0b482f91a84d91ac9914
> c18b794
> > 1dd32ab8c8a7 100644
> > ---
> a/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > +++
> b/Platform/ARM/VExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > @@ -21,9 +21,10 @@
> >  #include <ArmPlatform.h>
> >
> >  #define FRAME_BUFFER_DESCRIPTOR ((FixedPcdGet64
> > (PcdArmLcdDdrFrameBufferBase) != 0) ? 1 : 0)
> > +#define DP_BASE_DESCRIPTOR      ((FixedPcdGet64
> (PcdArmMaliDpBase) != 0) ? 1 : 0)
> >
> >  // Number of Virtual Memory Map Descriptors -#define
> > MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS (9 +
> FRAME_BUFFER_DESCRIPTOR)
> > +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS (9 +
> > +FRAME_BUFFER_DESCRIPTOR + DP_BASE_DESCRIPTOR)
> >
> >  // DDR attributes
> >  #define DDR_ATTRIBUTES_CACHED
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> > @@ -158,6 +159,13 @@ ArmPlatformGetVirtualMemoryMap (
> >    VirtualMemoryTable[Index].Attributes =
> > ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >  #endif
> >
> > +#if (FixedPcdGet64 (PcdArmMaliDpBase) != 0)
> > +  // DP500/DP550/DP650 peripheral memory region
> > +  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > +(PcdArmMaliDpBase);
> > +  VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64
> > +(PcdArmMaliDpBase);
> > +  VirtualMemoryTable[Index].Length = FixedPcdGet32
> > +(PcdArmMaliDpMemoryRegionLength);
> > +  VirtualMemoryTable[Index].Attributes =
> > +ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +#endif
> >    // Map sparse memory region if present
> >    if (HasSparseMemory) {
> >      VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.