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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thank you, Lloyd.

Comment inline.

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Evan Lloyd
Sent: Tuesday, December 20, 2016 9:21 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@ml01.01.org; Leif Lindholm <leif.lindholm@linaro.org>; ard.biesheuvel@linaro.org
Subject: Re: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI tables

Hi Jiewen.
Some responses inline below:

> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> Sent: 20 December 2016 05:54
> To: Ni, Ruiyu; Carsey, Jaben; Evan Lloyd
> Cc: Carsey, Jaben; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org>; Leif Lindholm; ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>
> Subject: RE: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI tables
>
> I like this idea - Shell.efi should only support commands defined by Shell spec.
> Then we should not port code to be a shell library.
>
> I have concern on ShellPkg\Library\UefiDpLib, because DP depends on TimerLib and TimerLib is a platform specific library.
> I do not object the idea to move DP from performancepkg to shellPkg.
> I would suggest DP had better be a standard shell application, rather than a shell lib and included in a shell bin.
>
>
> Now back to acpiview:
> First, it is great to have such tool in UEFI shell.

Thanks.  We hoped people would see the benefit.

>
> Second, I found the function is incomplete.
> 1)       Some ACPI defined tables are not complete. Take MADT as example, it only supports EFI_ACPI_6_1_GIC/EFI_ACPI_6_1_GICD/EFI_ACPI_6_1_GIC_MSI_FRAME/EFI_ACPI_6_1_GICR/EFI_ACPI_6_1_GIC_ITS.
> 2)       Some ACPI defined tables are missing. Such as BGRT, FPDT.

At this stage we have only added tables for which we had an immediate use, as available effort is limited.
In addition, we did not want to submit code we were not able to test.  Clearly, our platforms use GIC, not APIC.
Also  we are not well placed to judge the best representation for some fields, e.g. are APIC interrupt numbers normally decimal or hex?

[Jiewen] Got it.
As you mention, I hope it can be in SHELL spec, too.

So that we can have a standard way to dump all table information.

For X86 system, I have written similar tool to dump X86 related info.
I just check in to https://github.com/jyao1/EdkiiShellTool/tree/master/AcpiToolPkg.
It is also BSD license code. I tried by best to dump info for *all* the ACPI table.
But I do not validate ARM system. We complement to each other. :)

And mine does not have any consistency *check* function so far.
Your #2 below is very good.


> 3)       I found some ACPI reserved tables are added here. Such as DBG2, IORT.

Is there any problem with dumping the reserved tables?
[Jiewen] No problem at all.
This question is combined with 4). And I hope it can be complete as well.

> 4)       But not all ACPI reserved tables are added. Such as HPET, TPM2.

We only wrote code for tables we could test.  We have no example hardware/firmware with those tables.
[Jiewen] Same as 2). I have written tool for X64 system, but not for ARM.

> May I know what criteria we are using to select which feature is supported or not supported?

As stated above, we only provided the tables/sections that we needed to dump and could test.
>From the commit message:
"""
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.
"""
[Jiewen] Got it and agree.

> Or it is just something from start and still need some work to make it complete?

Yes, this is just a starting point and very far from complete.
We found it detected a number of problems for us, so we felt it worth contributing.
If we can get people to use it, that may mean fewer problems for us to debug.   ;-)
[Jiewen] See my last comment.

>
> Third, I found we need a long ? ?switch (*AcpiTableSignature) case? to dump/parse ACPI table specific function.
> Do you think it will be better, that we provide a register function to let each ACPI table register a parser?
> For example: RegisterDumpAcpiTable (EFI_ACPI_1_0_APIC_SIGNATURE, DumpAcpiMADT);
> Then the core logic just need use a table-driven way to find out the corresponding parser function by a SIGNATURE.

You are right, that would be a much better abstract design, and would make adding a table simpler.
We discussed this in our pre-release review.
The trade-off is that you need to maintain a dynamic registry of handlers, and write your own lookup code.
Using a switch eliminates the registry, and relies on the compiler for efficient look up.
We would be very happy with this change, should someone have the time available to contribute it.
[Jiewen] Thank you considering that. Actually, I have such code for X86 system.
I have a generic library for registration - https://github.com/jyao1/EdkiiShellTool/tree/master/AcpiToolPkg/Library/DumpAcpi/DumpAcpiTableFuncLib
So that every individual table can register itself by calling RegisterDumpAcpiTable() in the library constructor.

Our review also suggested changing the switch to just set a function pointer, with a single call at the end.
That would make each switch case much simpler.  Would that be an acceptable "half-way house"?
[Jiewen] Let me summarize current status:

1)       We know there is strong requirement to dump ACPI table in UEFI shell environment, for debug purposes.

2)       UEFI shell specification does not contain any ACPI dump command. (Although it has SmbiosDump)

3)       ACPICA.ORG has sample code to dump ACPI.
> > >Binary can be downloaded from: https://acpica.org/downloads/uefi-support
> > >Source can be downloaded from: https://github.com/acpica/acpica

4)       This patch provides dump ACPI information with consistency check. But limitation is: it only supports the limited table on ARM platform. NO support for X86 platform.

5)       I have similar code to dump ACPI. But the limitation is: it only validated on X86 platform. It is not validated on ARM platform.


So I would like to propose:

1)       DOCUMENTATION: Can we co-work to submit an ECR for SHELL, to add AcpiDump command? So that people may get standard dump log in the future.

2)       CODE: It seems both of our code is POC quality and has partial validation only. It might not be suitable to check in EDKII immediately.

I would like to propose we co-work in EDKII staging tree - https://github.com/tianocore/edk2-staging/ (We have 4 features there.)

We can align our design, then I can help validate X86 and you can help validate ARM.

Then we can submit a complete patch to EDKII ShellPkg.

What about your idea?

>
> Finally, I agree with Ruiyu that a standalone tool might be more proper.

Agreed in other response.

>
> Thank you
> Yao Jiewen

>> > > ...
> > > >> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel