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

["Gao, Liming" <liming.gao@intel.com>]

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