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

[Laszlo Ersek <lersek@redhat.com>]

On 03/29/17 11:49, Gao, Liming wrote:
> Laszlo:

> I agree GuidCName can't be used to initialize the global variable. If
> there is such requirement, GUID C MACRO will have to be defined.
> Then, its header file will be required.

> Now, we have no rule to forbid to add the header file if it has no
> other definitions except for GUID value, and we have no such
> discussion to define those rules. It is the developer choice to add
> it or not.

> Here, I want to mention that BaseTools has added "extern gGuidCName"
> into AutoGen header file. The consumer code can directly use
> gGuidCName without include its header file. If no GUID MACRO usage,
> its header file is not necessary.
> 
> For this case gEdkiiPlatformHasAcpiGuid, I know it will be as
> protocol dependency. I don't see its GUID C MACRO usage. So, I
> suggest not to add its header file. This is just my opinion.

Thank you for explaining the situation.

For now we've moved all of the generic/core patches in this series to
EmbeddedPkg (and the v4 series has been committed). Once Leif returns,
we can perhaps think about moving those GUIDs / lib instances to
MdeModulePkg, and we could agree at that point to drop the Guid/ header.

> For BaseTools enhancement to generated GUID C MACRO in autogen.h, I
> think it is valuable. To avoid the generated MACRO conflict with the
> existing MACRO, the generated MACRO will still have "G" prefix, such
> as gEdkiiPlatformHasAcpiGuid ==> G_EDKII_PLATFORM_HAS_ACPI_GUID.

Thanks -- I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=449>.
I marked it as a whishlist item.

Cheers
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, March 28, 2017 3:50 PM
>> To: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
>> <feng.tian@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/28/17 07:25, Gao, Liming wrote:
>>> Ersek:
>>>   I don't want to block your patch. You can still check in Guid header file if you
>> think it is necessary.
>>
>> I don't want to check in without formal MdeModulePkg maintainer
>> approval. If I check in a patch without formal approval, that will only
>> lead to chaos. I don't want to circumvent the process; I want the
>> process to *work*.
>>
>> If MdeModulePkg maintainers agree with my
>>
>>  [PATCH v3 04/12] MdeModulePkg: introduce EDKII Platform Has ACPI GUID
>>
>> then they should please give their Reviewed-by for it.
>>
>> If they disagree with it, they should please explain why.
>>
>> The specific argument you raised, namely that a BaseTools utility
>> generates the necessary C language artifacts for GUID use, is not
>> precise. BaseTools generate *some* artifacts, but they do not generate a
>> macro that is usable for initializing a GUID object of static storage
>> duration (= global variable GUID, or a GUID field in a global variable
>> structure).
>>
>> This is what the ISO C99 standard says:
>>
>>  6.7.8 Initialization
>>
>>  [...]
>>
>>  Constraints
>>
>>  [...]
>>
>>  4 All the expressions in an initializer for an object that has static
>>    storage duration shall be constant expressions or string literals.
>>
>>  [...]
>>
>>  16 Otherwise, the initializer for an object that has aggregate or
>>     union type shall be a brace-enclosed list of initializers for the
>>     elements or named members.
>>
>>  [...]
>>
>>  20 If the aggregate or union contains elements or members that are
>>     aggregates or unions, these rules apply recursively to the
>>     subaggregates or contained unions. [...]
>>
>> This is the GUID type definition:
>>
>> typedef struct {
>>  UINT32  Data1;
>>  UINT16  Data2;
>>  UINT16  Data3;
>>  UINT8   Data4[8];
>> } GUID;
>>
>> For this structure, the above standardese implies that we have to
>> provide integer constant expressions in the initializer.
>>
>>  6.6 Constant expressions
>>
>>  [...]
>>
>>  6 An /integer constant expression/ shall have integer type and shall
>>    only have operands that are integer constants, enumeration
>>    constants, character constants, /sizeof/ expressions whose results
>>    are integer constants, and floating constants that are the
>>    immediate operands of casts. Cast operators in an integer constant
>>    expression shall only convert arithmetic types to integer types,
>>    except as part of an operand to the /sizeof/ operator.
>>
>> The end result is that "gEdkiiPlatformHasAcpiGuid" is not usable in such
>> contexts, but the EDKII_PLATFORM_HAS_ACPI_GUID macro is.
>>
>>
>> If you want, I can file a TianoCore feature request BZ for generating
>> such macros as well in BaseTools, but for now, I don't think it should
>> either block my patch noted above, or force me to drop the Guid/ header
>> file from the patch. Right now, the Guid/ header adds something that is
>> not available from BaseTools, and I wouldn't like to delay the v3 series
>> any longer.
>>
>> Do you agree to the above method? I file a TianoCore Feature Request BZ
>> for BaseTools, and MdeModulePkg maintainers formally approve the v3
>> 04/12 patch?
>>
>>
>> (Side point: I think the Guid/ header file has another benefit:
>> documentation. I don't think we can add the amount of documentation that
>> is acceptable in a standalone Guid/ header to an in-line comment in the
>> .DEC file. So, I think that the new rule for not adding Guid/ headers is
>> immature at this point -- and, worse, I don't recall an open
>> announcement or discussion about this rule, so that we could have raised
>> concerns before turning the proposal into an actual rule.)
>>
>> Thanks
>> Laszlo
>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Tuesday, March 28, 2017 1:32 AM
>>>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming
>> <liming.gao@intel.com>
>>>> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; afish@apple.com; Tian, Feng
>>>> <feng.tian@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/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
>>>
>