Re: [edk2-devel] [Patch] ShellPkg/AcpiView: Fix IA32 link error

["Carsey, Jaben" <jaben.carsey@intel.com>]

I think the shell DSC be updated to catch build errors in the NULL libs. 

I am good with Mike's proposed solution below, but not very concerned with the method to catch the error.

Thanks
-Jaben


> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, August 01, 2019 1:30 PM
> To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>
> Subject: RE: [edk2-devel] [Patch] ShellPkg/AcpiView: Fix IA32 link error
> 
> Hi Sami,
> 
> I agree with your feedback.  I saw that there was a larger
> patch set for the ShellPkg, so I let that complete before
> returning to this topic.
> 
> The reason that I noticed this issue in the first place is
> when I added the acpiview command to a platform and there
> was an IA32 build failure.  It would be better if the ShellPkg
> build caught this issue.  Adding the acpiview command to
> the standard shell build adds 50K to an uncompressed DEBUG
> IA32 build.
> 
>     869,312 Shell_7C04A583-9E3E-4f1c-AD65-E05268D0B4D1.efi
>     920,928 Shell_EA4BB293-2D7F-4456-A681-1F22F42CD0BC.efi
> 
> Should acpiview be added to the standard ShellPkg build,
> or should I add an extra build of the Shell to the
> ShellPkg DSC file with a different GUID to make sure
> the shell builds when all NULL libs are included without
> any !if statements.  For example:
> 
>   ShellPkg/Application/Shell/Shell.inf {
>    <Defines>
>       FILE_GUID = EA4BB293-2D7F-4456-A681-1F22F42CD0BC
>     <PcdsFixedAtBuild>
>       gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>     <LibraryClasses>
> 
> NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2Comma
> ndsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1Comma
> ndsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3Comma
> ndsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1Com
> mandsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1Com
> mandsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Com
> mandsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1
> CommandsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2
> CommandsLib.inf
> 
> NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCo
> mmandLib.inf
>   }
> 
> Thanks,
> 
> Mike
> 
> > -----Original Message-----
> > From: Sami Mujawar [mailto:Sami.Mujawar@arm.com]
> > Sent: Thursday, July 11, 2019 1:24 AM
> > To: devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Subject: RE: [edk2-devel] [Patch] ShellPkg/AcpiView:
> > Fix IA32 link error
> >
> > Hi Mike,
> >
> > Since LocalityCount is 64-bit wide the SLIT validation
> > code could possibly end up in an infinite loop. I am
> > not aware of a platform that has a large enough
> > LocalityCount to hit this condition. However, would it
> > be good to have a check that limits the validation to
> > MAX_UINT32?
> >
> > e.g. Something like
> > if  (LocalityCount < MAX_UINT32) {
> >   // Validate
> >   for (Count = 0; Count < LocalityCount; Count++) {
> >     for (Index = 0; Index < LocalityCount; Index++) {
> >   ...
> > } else {
> >   Print (L"INFO: Skipping validation of System
> > Localities as locality count is > MAX_UINT32\n"); }
> >
> > Regards,
> >
> > Sami Mujawar
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > Behalf Of Michael D Kinney via Groups.Io
> > Sent: 10 July 2019 11:35 PM
> > To: devel@edk2.groups.io
> > Cc: Jaben Carsey <jaben.carsey@intel.com>; Ray Ni
> > <ray.ni@intel.com>; Zhichao Gao <zhichao.gao@intel.com>
> > Subject: [edk2-devel] [Patch] ShellPkg/AcpiView: Fix
> > IA32 link error
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1970
> >
> > Update local variable in ParseAcpiSlot() to be UINT32
> > instead of UINT64 to avoid 64-bit multiply operation in
> > the SLIT_ELEMENT() macro.
> >
> > Cc: Jaben Carsey <jaben.carsey@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > Signed-off-by: Michael D Kinney
> > <michael.d.kinney@intel.com>
> > ---
> >
> > .../UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> > .c     | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitParser.c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitParser.c
> > index 1f9dac66ee..af85c9aa1c 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitParser.c
> > +++
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitPars
> > +++ er.c
> > @@ -57,8 +57,8 @@ ParseAcpiSlit (
> >    )
> >  {
> >    UINT32 Offset;
> > -  UINT64 Count;
> > -  UINT64 Index;
> > +  UINT32 Count;
> > +  UINT32 Index;
> >    UINT64 LocalityCount;
> >    UINT8* LocalityPtr;
> >    CHAR16 Buffer[80];  // Used for AsciiName param of
> > ParseAcpi
> > --
> > 2.21.0.windows.1
> >
> >
> > 
> >
> > 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.