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

[Laszlo Ersek <lersek@redhat.com>]

On 03/27/17 09:00, Ard Biesheuvel wrote:
> On 27 March 2017 at 03:42, Gao, Liming <liming.gao@intel.com> wrote:
>> Laszlo:
>>   On GUID header file, some header file are here, because they are added before BaseTools supports it. Now, we allow not to add header file if the header file only has GUID value definition.
> 
> I have to agree with Laszlo here. The BaseTools support is incomplete,
> since it does not add a #define for the GUID to AutoGen.h. This makes
> it impossible to initialize static structures containing GUIDs, such
> as templates containing vendor device paths.
> 
> For instance, the following
> 
> typedef struct {
>   EFI_GUID foo;
> } TYPE;
> 
> TYPE mFoo {
>   SOME_GUID
> };
> 
> is not possible without a Guid/xxx.h include file containing a #define
> for SOME_GUID.

I wonder if we can commit this series before end of April.

Or is that too soon? End of May perhaps?

The mind boggles.

Laszlo

> 
>>   On GUID C MACRO, we suggest to use GuidCName in every C source code. So, we don't suggest to add it.  As you know, some existing header files have GuidCName and GUID Macro. Now, we have no plan to clean up the existing one. But, we expect new added file follows this rule.
>>
>> Thanks
>> Liming
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Saturday, March 25, 2017 1:09 AM
>>> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; afish@apple.com
>>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.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 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
>>>>
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel