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

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

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; Leif Lindholm; 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?

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

Is there any problem with dumping the reserved tables?

> 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.

> 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.
"""

> 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.   ;-)

>
> 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.

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"?

>
> 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.