ArmPkg/PlatformBootManagerLib: fixes

[edk2] [PATCH 2/3] ArmPkg/PlatformBootManagerLib: refer to Shell FILE_GUID directly

Posted by Ard Biesheuvel 7 years, 7 months ago

Instead of indirecting the reference to the Shell binary via a PCD
that is defined in IntelFrameworkModulePkg, and which invariably
gets set to the same value by all users of this library, move the
reference into the code, and drop the reference to the PCD entirely.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 7 +++++--
 ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 -
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index cc5a4d1ff9b3..d479c28775fb 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -33,6 +33,9 @@
 
 #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
 
+STATIC CONST EFI_GUID mUefiShellFileGuid = {
+  0x7C04A583, 0x9E3E, 0x4f1c, { 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
+};
 
 #pragma pack (1)
 typedef struct {
@@ -327,7 +330,7 @@ AddOutput (
 STATIC
 VOID
 PlatformRegisterFvBootOption (
-  EFI_GUID                         *FileGuid,
+  CONST EFI_GUID                   *FileGuid,
   CHAR16                           *Description,
   UINT32                           Attributes
   )
@@ -540,7 +543,7 @@ PlatformBootManagerAfterConsole (
   // Register UEFI Shell
   //
   PlatformRegisterFvBootOption (
-    PcdGetPtr (PcdShellFile), L"UEFI Shell", LOAD_OPTION_ACTIVE
+    &mUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE
     );
 }
 
diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 8ec4f1dea6c4..8ac3b3799674 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -59,7 +59,6 @@ [FeaturePcd]
 
 [FixedPcd]
   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdLogoFile
-  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
-- 
2.7.4

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

Re: [edk2] [PATCH 2/3] ArmPkg/PlatformBootManagerLib: refer to Shell FILE_GUID directly

Posted by Leif Lindholm 7 years, 7 months ago

On Mon, Mar 20, 2017 at 08:35:44PM +0000, Ard Biesheuvel wrote:
> Instead of indirecting the reference to the Shell binary via a PCD
> that is defined in IntelFrameworkModulePkg, and which invariably
> gets set to the same value by all users of this library, move the
> reference into the code, and drop the reference to the PCD entirely.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 7 +++++--
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 -
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index cc5a4d1ff9b3..d479c28775fb 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -33,6 +33,9 @@
>  
>  #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
>  
> +STATIC CONST EFI_GUID mUefiShellFileGuid = {
> +  0x7C04A583, 0x9E3E, 0x4f1c, { 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
> +};

Surely this ought to be defined in a shared header file rather than
replicated across the tree? (And yes, used in QuarkPlatformPkg as
well.)

Otherwise, looks like a good change.

/
    Leif

>  
>  #pragma pack (1)
>  typedef struct {
> @@ -327,7 +330,7 @@ AddOutput (
>  STATIC
>  VOID
>  PlatformRegisterFvBootOption (
> -  EFI_GUID                         *FileGuid,
> +  CONST EFI_GUID                   *FileGuid,
>    CHAR16                           *Description,
>    UINT32                           Attributes
>    )
> @@ -540,7 +543,7 @@ PlatformBootManagerAfterConsole (
>    // Register UEFI Shell
>    //
>    PlatformRegisterFvBootOption (
> -    PcdGetPtr (PcdShellFile), L"UEFI Shell", LOAD_OPTION_ACTIVE
> +    &mUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE
>      );
>  }
>  
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 8ec4f1dea6c4..8ac3b3799674 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -59,7 +59,6 @@ [FeaturePcd]
>  
>  [FixedPcd]
>    gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdLogoFile
> -  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
> -- 
> 2.7.4
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/3] ArmPkg/PlatformBootManagerLib: refer to Shell FILE_GUID directly

Posted by Ard Biesheuvel 7 years, 7 months ago

On 22 March 2017 at 12:53, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Mar 20, 2017 at 08:35:44PM +0000, Ard Biesheuvel wrote:
>> Instead of indirecting the reference to the Shell binary via a PCD
>> that is defined in IntelFrameworkModulePkg, and which invariably
>> gets set to the same value by all users of this library, move the
>> reference into the code, and drop the reference to the PCD entirely.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 7 +++++--
>>  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 -
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> index cc5a4d1ff9b3..d479c28775fb 100644
>> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -33,6 +33,9 @@
>>
>>  #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
>>
>> +STATIC CONST EFI_GUID mUefiShellFileGuid = {
>> +  0x7C04A583, 0x9E3E, 0x4f1c, { 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
>> +};
>
> Surely this ought to be defined in a shared header file rather than
> replicated across the tree? (And yes, used in QuarkPlatformPkg as
> well.)
>
> Otherwise, looks like a good change.
>

Yes, it would make the most sense to add this to the [Guid] section of
ShellPkg itself. I will propose it as a separate series, and replace
the quark reference as well
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/3] ArmPkg/PlatformBootManagerLib: refer to Shell FILE_GUID directly

Posted by Leif Lindholm 7 years, 7 months ago

On Wed, Mar 22, 2017 at 01:16:26PM +0000, Ard Biesheuvel wrote:
> On 22 March 2017 at 12:53, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Mar 20, 2017 at 08:35:44PM +0000, Ard Biesheuvel wrote:
> >> Instead of indirecting the reference to the Shell binary via a PCD
> >> that is defined in IntelFrameworkModulePkg, and which invariably
> >> gets set to the same value by all users of this library, move the
> >> reference into the code, and drop the reference to the PCD entirely.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 7 +++++--
> >>  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 -
> >>  2 files changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> index cc5a4d1ff9b3..d479c28775fb 100644
> >> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> @@ -33,6 +33,9 @@
> >>
> >>  #define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
> >>
> >> +STATIC CONST EFI_GUID mUefiShellFileGuid = {
> >> +  0x7C04A583, 0x9E3E, 0x4f1c, { 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
> >> +};
> >
> > Surely this ought to be defined in a shared header file rather than
> > replicated across the tree? (And yes, used in QuarkPlatformPkg as
> > well.)
> >
> > Otherwise, looks like a good change.
> >
> 
> Yes, it would make the most sense to add this to the [Guid] section of
> ShellPkg itself. I will propose it as a separate series, and replace
> the quark reference as well

Since I'm about to go offline - with that change implemented:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel