| Message ID | 1490042145-19509-3-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ArmPkg/PlatformBootManagerLib: fixes | expand |
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
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
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
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
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(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel