Re: [PATCH edk2-platforms v3 12/17] ARM/VExpressPkg: Allocate framebuffer using EfiRuntimeServicesData

[Girish Pathak <Girish.Pathak@arm.com>]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 21 March 2018 03:38
> To: Girish Pathak <Girish.Pathak@arm.com>
> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>;
> Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt
> <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>; Arvind Chauhan
> <Arvind.Chauhan@arm.com>; Daniil Egranov <Daniil.Egranov@arm.com>;
> Thomas Abraham <thomas.abraham@arm.com>
> Subject: Re: [PATCH edk2-platforms v3 12/17] ARM/VExpressPkg: Allocate
> framebuffer using EfiRuntimeServicesData
> 
> On 21 March 2018 at 00:18, Girish Pathak <girish.pathak@arm.com> wrote:
> > As per the UEFI specification(2.7) section 12.9, the GOP framebuffer
> > memory can be accessed in the pre-boot and the post boot phase (by OS)
> > Therefore the memory type EfiBootServicesData is incorrect for the
> > framebuffer memory allocation. Change EfiBootServicesData with
> > EfiRuntimeServicesData flag so that allocated memory can be access by
> > the OS in the post boot phase.
> >
> 
> EfiRuntimeServicesData is intended for allocations that the EFI runtime
> services need to access themselves at runtime, and will hence be virtually
> remapped by SetVirtualAddressMap().
> 
> This does not apply to the framebuffer. Even if it may be used at OS runtime,
> the firmware itself will never access it, so EfiRuntimeServicesData is not
> appropriate
> 
> Please use EfiReservedMemory instead.

Specification (UEFI Spec 2_7_A Sept 6.pdf) describes EfiReservedMemoryType as  Not usable before and after ExitBootServices, See Table 28 & 29
Hence EfiReservedMemoryType is not suitable in this case.  Agree? 

> 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> > Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> > ---
> >
> Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpr
> ess.c       | 2 +-
> >
> >
> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArm
> VEx
> > press.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> >
> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> pres
> > s.c
> >
> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> pres
> > s.c index
> >
> f7cae39c9cc9954ba4cad1bd597ebfc8baf10f11..c0a25a18d3fcfe91a76ee985ee
> 58
> > 145b97900fa0 100644
> > ---
> >
> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> pres
> > s.c
> > +++
> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> > +++ press.c
> > @@ -176,7 +176,7 @@ LcdPlatformGetVram (
> >    }
> >    Status = gBS->AllocatePages (
> >                    AllocationType,
> > -                  EfiBootServicesData,
> > +                  EfiRuntimeServicesData,
> >                    EFI_SIZE_TO_PAGES (((UINTN)LCD_VRAM_SIZE)),
> >                    VramBaseAddress
> >                    );
> > diff --git
> >
> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr
> mV
> > Express.c
> >
> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr
> mV
> > Express.c index
> >
> 2f4814a2adbf01517ba14d75ce579ff35c362379..61ddf77e903e6c33a26b2aa8b7
> 61
> > 21e807195a9a 100644
> > ---
> >
> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr
> mV
> > Express.c
> > +++
> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd
> > +++ ArmVExpress.c
> > @@ -232,7 +232,7 @@ LcdPlatformGetVram (
> >      // Allocate the VRAM from the DRAM so that nobody else uses it.
> >      Status = gBS->AllocatePages (
> >                      AllocateAddress,
> > -                    EfiBootServicesData,
> > +                    EfiRuntimeServicesData,
> >                      EFI_SIZE_TO_PAGES (((UINTN)LCD_VRAM_SIZE)),
> >                      VramBaseAddress
> >                      );
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >