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

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

Ersek:
  I don't want to block your patch. You can still check in Guid header file if you think it is necessary. 

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