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

2018-03-22 Thread Ard Biesheuvel 
On 22 March 2018 at 15:20, Evan Lloyd <evan.ll...@arm.com> wrote:
> Hi Ard.
>
>> -Original Message-
>> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Ard
>> Biesheuvel
>> Sent: 21 March 2018 18:27
>> To: Girish Pathak <girish.pat...@arm.com>
>> Cc: nd <n...@arm.com>; edk2-devel@lists.01.org; Leif Lindholm
>> <leif.lindh...@linaro.org>; Stephanie Hughes-Fitt > f...@arm.com>; Arvind Chauhan <arvind.chau...@arm.com>
>> Subject: Re: [edk2] [PATCH edk2-platforms v3 12/17] ARM/VExpressPkg:
>> Allocate framebuffer using EfiRuntimeServicesData
>>
>> On 21 March 2018 at 19:07, Girish Pathak <girish.pat...@arm.com>
>> wrote:
>> >
>> >
>> >> -Original Message-
>> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> >> Sent: 21 March 2018 03:38
>> >> To: Girish Pathak <girish.pat...@arm.com>
>> >> Cc: edk2-devel@lists.01.org; Leif Lindholm
>> >> <leif.lindh...@linaro.org>; Matteo Carlini <matteo.carl...@arm.com>;
>> >> Stephanie Hughes-Fitt <stephanie.hughes-f...@arm.com>; nd
>> >> <n...@arm.com>; Arvind Chauhan <arvind.chau...@arm.com>; Daniil
>> Egranov
>> >> <daniil.egra...@arm.com>; Thomas Abraham
>> <thomas.abra...@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.pat...@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?
>> >
>>
>> It is not usable as ordinary memory, given that you turn it into 'special'
>> memory (with side effects) by turning it into a framebuffer.
>>
>> So EfiReservedMemory is perfectly appropriate here.
> [[Evan Lloyd]] First, I agree EfiReservedMemory is probably the sensible 
> option.  The only alternative would be EfiMemoryMappedIO, and that, as you 
> have pointed out in the past, introduces alignment requirements.  If you are 
> happy to accept it, I'll ask Girish to go with that.

EfiReservedMemory is the only appropriate type to use here, and if the
spec is unclear, we should fix that.


>  However, Girish has a point, if only that the UEFI spec need more clarity on 
> this, especially as 
> https://lists.01.org/pipermail/edk2-devel/2017-February/007494.html pretty 
> much confirms it is the right way to go.  In particular, the bald statement 
> "EfiReservedMemoryTypeNot usable.", seems unfortunate.
>
> Regards,
> Evan
>
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-03-22 Thread Evan Lloyd 
Hi Ard.

> -Original Message-
> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Ard
> Biesheuvel
> Sent: 21 March 2018 18:27
> To: Girish Pathak <girish.pat...@arm.com>
> Cc: nd <n...@arm.com>; edk2-devel@lists.01.org; Leif Lindholm
> <leif.lindh...@linaro.org>; Stephanie Hughes-Fitt  f...@arm.com>; Arvind Chauhan <arvind.chau...@arm.com>
> Subject: Re: [edk2] [PATCH edk2-platforms v3 12/17] ARM/VExpressPkg:
> Allocate framebuffer using EfiRuntimeServicesData
> 
> On 21 March 2018 at 19:07, Girish Pathak <girish.pat...@arm.com>
> wrote:
> >
> >
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: 21 March 2018 03:38
> >> To: Girish Pathak <girish.pat...@arm.com>
> >> Cc: edk2-devel@lists.01.org; Leif Lindholm
> >> <leif.lindh...@linaro.org>; Matteo Carlini <matteo.carl...@arm.com>;
> >> Stephanie Hughes-Fitt <stephanie.hughes-f...@arm.com>; nd
> >> <n...@arm.com>; Arvind Chauhan <arvind.chau...@arm.com>; Daniil
> Egranov
> >> <daniil.egra...@arm.com>; Thomas Abraham
> <thomas.abra...@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.pat...@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?
> >
> 
> It is not usable as ordinary memory, given that you turn it into 'special'
> memory (with side effects) by turning it into a framebuffer.
> 
> So EfiReservedMemory is perfectly appropriate here.
[[Evan Lloyd]] First, I agree EfiReservedMemory is probably the sensible 
option.  The only alternative would be EfiMemoryMappedIO, and that, as you have 
pointed out in the past, introduces alignment requirements.  If you are happy 
to accept it, I'll ask Girish to go with that.  However, Girish has a point, if 
only that the UEFI spec need more clarity on this, especially as 
https://lists.01.org/pipermail/edk2-devel/2017-February/007494.html pretty much 
confirms it is the right way to go.  In particular, the bald statement 
"EfiReservedMemoryTypeNot usable.", seems unfortunate.

Regards,
Evan

> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-03-21 Thread Ard Biesheuvel 
On 21 March 2018 at 19:07, Girish Pathak  wrote:
>
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: 21 March 2018 03:38
>> To: Girish Pathak 
>> Cc: edk2-devel@lists.01.org; Leif Lindholm ;
>> Matteo Carlini ; Stephanie Hughes-Fitt
>> ; nd ; Arvind Chauhan
>> ; Daniil Egranov ;
>> Thomas Abraham 
>> Subject: Re: [PATCH edk2-platforms v3 12/17] ARM/VExpressPkg: Allocate
>> framebuffer using EfiRuntimeServicesData
>>
>> On 21 March 2018 at 00:18, Girish Pathak  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?
>

It is not usable as ordinary memory, given that you turn it into
'special' memory (with side effects) by turning it into a framebuffer.

So EfiReservedMemory is perfectly appropriate here.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-03-21 Thread Girish Pathak 


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 21 March 2018 03:38
> To: Girish Pathak 
> Cc: edk2-devel@lists.01.org; Leif Lindholm ;
> Matteo Carlini ; Stephanie Hughes-Fitt
> ; nd ; Arvind Chauhan
> ; Daniil Egranov ;
> Thomas Abraham 
> Subject: Re: [PATCH edk2-platforms v3 12/17] ARM/VExpressPkg: Allocate
> framebuffer using EfiRuntimeServicesData
> 
> On 21 March 2018 at 00:18, Girish Pathak  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 
> > Signed-off-by: Evan Lloyd 
> > ---
> >
> 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)'
> >
> >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-03-20 Thread Ard Biesheuvel 
On 21 March 2018 at 00:18, Girish Pathak  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.


> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Girish Pathak 
> Signed-off-by: Evan Lloyd 
> ---
>  Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c  
>  | 2 +-
>  
> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c 
> | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git 
> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c 
> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index 
> f7cae39c9cc9954ba4cad1bd597ebfc8baf10f11..c0a25a18d3fcfe91a76ee985ee58145b97900fa0
>  100644
> --- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.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/PL111LcdArmVExpress.c
>  
> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 
> 2f4814a2adbf01517ba14d75ce579ff35c362379..61ddf77e903e6c33a26b2aa8b76121e807195a9a
>  100644
> --- 
> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ 
> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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)'
>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-03-20 Thread Girish Pathak 
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.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Girish Pathak 
Signed-off-by: Evan Lloyd 
---
 Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c   
| 2 +-
 Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c 
| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git 
a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c 
b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
index 
f7cae39c9cc9954ba4cad1bd597ebfc8baf10f11..c0a25a18d3fcfe91a76ee985ee58145b97900fa0
 100644
--- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
+++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.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/PL111LcdArmVExpress.c 
b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index 
2f4814a2adbf01517ba14d75ce579ff35c362379..61ddf77e903e6c33a26b2aa8b76121e807195a9a
 100644
--- 
a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ 
b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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)'


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel