Re: [PATCH] ShellPkg: Add acpiview tool to dump ACPI tables

[Evan Lloyd <Evan.Lloyd@arm.com>]

Hi Ray.
Comments inline below

>-----Original Message-----
>From: Ni, Ruiyu [mailto:ruiyu.ni@intel.com]
>Sent: 20 December 2016 02:59
>To: Carsey, Jaben; Evan Lloyd
>Cc: Leif Lindholm; ard.biesheuvel@linaro.org; edk2-devel@ml01.01.org
>Subject: RE: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI
>tables
>
>Evan,
>Thanks for the answers. BSD license of your code is great! Thanks for your
>contribution.
>But I do have some other thoughts in below.
>
>Jaben, Evan,
>The implementation style of "acpiview" is to embed it into "Shell.efi" as an
>internal
>command, in "Debug1" profile.
>
>I am afraid it's a violation of Shell spec because Shell spec defines a fixed list
>of commands
>that belongs to "Debug1" profile, while "acpiview" isn't in the list.

Don't tell anyone, but we hope eventually to get this added to the shell spec.
We wrote the tool to investigate some problems, and it proved so useful that we wanted to make it available as quickly as possible.

>On the other side, some people may want a standalone "acpiview.efi" that
>he use it in
>any UEFI Shell environment.

Sami originally had this - I am to blame for making it a Shell app like smbiosview.

The .efi is helpful for platforms that support USB drives, as you can just invoke it off a drive.
Sami will look at adding the INF #2 to our patch.

>
>How about we provide two INF files for this tool:
>1. NULL class INF providing constructor/destructor(Current INF)
>2. APP INF generating standalone "acpiview.efi" (New INF I want)
>
>When creating INF #2, in order to embed the help message into the
>resource section of EFI file
>so that Shell core can find it and display when "-?" is typed, we need to:
>1. declare "UEFI_HII_RESOURCE_SECTION  = TRUE" to let build tool embed
>HII resource (string)
>    into the resource section.
>2. write code in entrypoint of application to retrieve the string resource
>from ImageHandle, and
>    free the resource in Unload.
>    Sample code is in
>MdeModulePkg/Universal/HiiResourcesSampleDxe/HiiResourcesSample.c
>3. Explicitly reference the token ID of help string in C file so that build tool
>doesn't optimize out
>    the help string from HII resource.
>    Sample code in MdeModulePkg/Application/HelloWorld/HelloWorld.c:
>    GLOBAL_REMOVE_IF_UNREFERENCED EFI_STRING_ID
>mStringHelpTokenId
>                                             = STRING_TOKEN
>(STR_HELLO_WORLD_HELP_INFORMATION);
>
>The step 2&3 can be put in a new .C file that only be referenced by the APP
>INF file.

Thank you for the pointers.  We'll add this advice into our next update.

>
>Since now we provide both library and application for acpiview, how about
>put it in new location
>ShellPkg/Application/AcpiView directory?

You're the maintainer - we'll happily take your advice on the best place for it.

>
>What's your option? If you agree, I can help to create the APP INF.

We'll take all the help we can get, please.  If you would like to send us a .INF, we'd be grateful for the time it will save us.

>
>Thanks/Ray
>
>> -----Original Message-----
>> From: Carsey, Jaben
>> Sent: Tuesday, December 20, 2016 2:01 AM
>> To: Evan Lloyd <Evan.Lloyd@arm.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
>> edk2-devel@ml01.01.org
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; ard.biesheuvel@linaro.org;
>> Carsey, Jaben <jaben.carsey@intel.com>
>> Subject: RE: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI
>tables
>>
>> I am good with this addition.  The BSD is a huge advantage for me.
>>
>> Reviewed-by: Jaben Carsey <Jaben.carsey@intel.com>
>>
>> I will allow Ray to resolve any issues he and push it.
>>
>> -Jaben
>>
>> > -----Original Message-----
>> > From: Evan Lloyd [mailto:Evan.Lloyd@arm.com]
>> > Sent: Monday, December 19, 2016 7:57 AM
>> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@ml01.01.org
>> > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Leif Lindholm
>> > <leif.lindholm@linaro.org>; ard.biesheuvel@linaro.org
>> > Subject: RE: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI
>> > tables
>> > Importance: High
>> >
>> > Hi Ray.
>> > Good question.  Answer inline.
>> >
>> > >-----Original Message-----
>> > >From: Ni, Ruiyu [mailto:ruiyu.ni@intel.com]
>> > >Sent: 19 December 2016 09:49
>> > >To: Evan Lloyd; edk2-devel@ml01.01.org
>> > >Cc: Carsey, Jaben; Leif Lindholm; ard.biesheuvel@linaro.org
>> > >Subject: RE: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI
>> > >tables
>> > >
>> > >I happened to find another version of acpi dump tool in shell.
>> > >Binary can be downloaded from: https://acpica.org/downloads/uefi-
>> > >support
>> > >Source can be downloaded from: https://github.com/acpica/acpica
>> > >
>> > >Are there any differences between the above one and yours?
>> >
>> > The main differences are:
>> > 1.acpiview provides a formatted dump of non-AML tables at the Shell
>level.
>> > This provides the  equivalent of using acpidump -b, then transferring
>> > the file to use "iasl -d" on another system.  However, it works on
>> > systems where the tables do not yet enable an OS to boot, or have
>limited
>> disk functionality.
>> > 2.It provides a small amount of specification consistency
>> > checking/error reporting at the Shell level.  We have found that to be
>> > an invaluable aid to debugging.
>> > 3.It has a normal UEFI BSD license, so doesn't instigate legal anxiety.
>> >
>> > Regards,
>> > Evan
>> >
>> >
>> > >
>> > >Thanks/Ray
>> > >
>> > >> -----Original Message-----
>> > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
>Behalf
>> > >> Of evan.lloyd@arm.com
>> > >> Sent: Saturday, December 17, 2016 2:26 AM
>> > >> To: edk2-devel@ml01.01.org
>> > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu
>> > ><ruiyu.ni@intel.com>;
>> > >> Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
>> > >> <ard.biesheuvel@linaro.org>
>> > >> Subject: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI
>> > >> tables
>> > >>
>> > >> From: Sami Mujawar <sami.mujawar@arm.com>
>> > >>
>> > >> This program is provided to allow examination of ACPI table
>> > >> contents from the UEFI Shell.  This can help with investigations,
>> > >> especially at that stage where the tables are not enabling an OS to
>boot.
>> > >> The program is not exhaustive, and only encapsulates detailed
>> > >> knowledge of a limited number of table types.
>> > >>
>> > >> Default behaviour is to display the content of all tables installed.
>> > >> 'Known' table types will be parsed and displayed with descriptions
>> > >> and field values.  Where appropriate a degree of consistency
>> > >> checking is done and errors may be reported in the output.
>> > >> Other table types will be displayed as an array of Hexadecimal bytes.
>> > >>
>> > >> To facilitate debugging, the -t and -b options can be used to
>> > >> generate a binary file image of a table that can be copied
>> > >> elsewhere for investigation using tools such as those provided by
>> > >> acpica.org.  This is especially relevant for AML type tables like DSDT
>and
>> SSDT.
>> > >>
>> > >> The inspiration for this is the existing smbiosview Debug1 Shell
>> > >> command, and the command is also intended for Debug1.
>> > >>
>> > >> Many tables are not explicitly handled, in part because no examples
>> > >> are available for our testing.
>> > >>
>> > >> The program is designed to be extended to new tables with minimal
>> > >> effort, and contributions are invited.
>> > >>
>> > >> The code is available for examination at:
>> > >> https://github.com/EvanLloyd/tianocore/tree/651_acpiview_v1
>> > >>
>> > >> Contributed-under: TianoCore Contribution Agreement 1.0
>> > >> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> > >> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>> > >> ---
>> > >>  ShellPkg/ShellPkg.dec                                                        |   2 +
>> > >>
>> > >>
>> > ...
>> > >> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>> > >>
>> > >> _______________________________________________
>> > >> edk2-devel mailing list
>> > >> edk2-devel@lists.01.org
>> > >> https://lists.01.org/mailman/listinfo/edk2-devel
>> > 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.
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.