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

[Laszlo Ersek <lersek@redhat.com>]

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
>