Re: [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library

[Laszlo Ersek <lersek@redhat.com>]

On 03/24/17 04:44, Zeng, Star wrote:
> I just think it seems a generic problem.
> Some platforms may dynamically determine whether ACPI should be supported or not.
> Some platforms may dynamically determine whether SMBIOS should be supported or not.
> Some platforms may dynamically determine whether HII should be supported or not.
> ...
> 
> We are thinking whether we can have a generic NULL instance to support all this kind of cases, for example:
> 1. Define a FixedAtBuild PCD PcdDepexGuid that will be a VOID* type PCD point to a depex GUID.
> 2. Implement a NULL instance DepexLib.
> [Defines]
>   INF_VERSION                    = 0x00010005
>   BASE_NAME                      = DepexLib
>   FILE_GUID                      = XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX
>   MODULE_TYPE                    = BASE
>   VERSION_STRING                 = 1.0
>   LIBRARY_CLASS                  = NULL
> 
> [Sources]
>   DepexLib.c
> 
> [Packages]
>   XXXPkg/XXXPkg.dec
> 
> [Depex]
>   PcdDepexGuid
> 
> 3. Link NULL instance and configure PCD in dsc.
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
>     <LibraryClasses>
>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>     <PcdsFixedAtBuild>
>       PcdDepexGuid|gEdkiiPlatformHasAcpiGuid
> }
> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf {
>     <LibraryClasses>
>       NULL|XXXPkg/Library/DepexLib/DepexLib.inf
>     <PcdsFixedAtBuild>
>       PcdDepexGuid|gEdkiiPlatformHasSmbiosGuid
> }
> 
> But current BaseTools does not support the syntax to declare PCD in [Depex] section of inf yet.
> 
> Based on the statements above, I have below comments.

> 1. I agree to add the GUID into MdeModulePkg, but how about using
> gEdkiiPlatformHasAcpiGuid instead of
> gEdkiiPlatformHasAcpiProtocolGuid and add the GUID into [Guids]
> section of MdeModulePkg.dec? As Platform can install
> gEdkiiPlatformHasAcpiGuid PPI to control PEIM and install
> gEdkiiPlatformHasAcpiGuid PROTOCOL to control DRIVER.

Sounds reasonable, I'll do this.

> And another, no
> need to add a include file PlatformHasAcpi.h as BaseTools supports to
> add the GUID definitions to AutoGen files from package dec.

I disagree with this. I mean, I *personally* wouldn't mind, but this
would be inconsistent with existing MdeModulePkg practice. For example, see

MdeModulePkg/Include/Guid/TtyTerm.h

Similarly to the current case, that GUID ("gEfiTtyTermGuid") has no data
structures associated with it; the header file only has an extern
declaration, and -- importantly -- an array initializer macro called
EFI_TTY_TERM_GUID, which can be used in the following syntax:

STATIC CONST EFI_GUID mThisIsMyGuid = EFI_TTY_TERM_GUID;

So, I think that the GUID header file is required. I will add it in v3.

Once we generalize this idea to the design that you laid out above, we
can perhaps eliminate the header file. But, for now, I think we should
remain consistent with other MdeModulePkg GUID definitions.

> 2. You can file bugzilla bug to request BaseTools syntax support to
> declare PCD in [Depex] section of inf. Then PcdDepexGuid and DepexLib
> could be finally added into core package MdeModulePkg or even MdePkg
> (I prefer).

I filed:

https://bugzilla.tianocore.org/show_bug.cgi?id=443 -- for BaseTools,
https://bugzilla.tianocore.org/show_bug.cgi?id=444 -- for the INF spec

> Before that, how about implementing the
> PlatformHasAcpiLib in none core package?

Works for me; I'll split that part off and keep it under ArmPkg.

Thanks
Laszlo

> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Thursday, March 23, 2017 5:09 PM
> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
> Cc: Tian, Feng <feng.tian@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library
> 
> On 23 March 2017 at 01:41, Zeng, Star <star.zeng@intel.com> wrote:
>> I prefer to do not include the protocol definition and the library instance into MdeModulePkg at this moment until they need to be used by multiple platforms/archs.
>>
> 
> I disagree. Nowhere in the PI or UEFI spec is there any mention (for any architecture) that ACPI is mandatory. Given that EDK2 is the reference platform UEFI/PI, and not for ACPI or the PC/x86 platform, I think it is unreasonable to have lots and lots of PC quirks (i.e., allocations below 4 GB) in MdeModulePkg, but then disallow a clean approach to make ACPI selectable, in a way that is true to the spirit of PI (i.e., using protocol dependencies)
> 
> So I think at least the protocol definitions belong in MdeModulePkg.
> 
> Thanks,
> Ard.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>