From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4FE5B2041D9DC for ; Wed, 29 Mar 2017 06:03:16 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A804BC05167A; Wed, 29 Mar 2017 13:03:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A804BC05167A Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com A804BC05167A Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-134.phx2.redhat.com [10.3.116.134]) by smtp.corp.redhat.com (Postfix) with ESMTP id 68A95B23EF; Wed, 29 Mar 2017 13:03:13 +0000 (UTC) To: "Gao, Liming" , Ard Biesheuvel References: <20170317204731.31488-1-lersek@redhat.com> <20170317204731.31488-6-lersek@redhat.com> <20170318150041.GL16034@bivouac.eciton.net> <1bc7b29c-7a71-86cb-adce-5a14de129c63@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B8377AB@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B84CA03@shsmsx102.ccr.corp.intel.com> <5b2c0152-85fe-db6b-e173-0e4b0891e0f2@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D702AE8@shsmsx102.ccr.corp.intel.com> <66e4cce5-1a24-2ea2-f701-39ff9dde0897@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D70446E@shsmsx102.ccr.corp.intel.com> <28eb520f-2c2a-8743-80e6-dc68e79decf5@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D7062E2@shsmsx102.ccr.corp.intel.com> Cc: "Zeng, Star" , "Kinney, Michael D" , "afish@apple.com" , "Tian, Feng" , edk2-devel-01 , Leif Lindholm From: Laszlo Ersek Message-ID: Date: Wed, 29 Mar 2017 15:03:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D7062E2@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 29 Mar 2017 13:03:15 +0000 (UTC) Subject: Re: [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Mar 2017 13:03:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 . 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 ; Ard Biesheuvel >> >> Cc: Zeng, Star ; Kinney, Michael D >> ; afish@apple.com; Tian, Feng >> ; edk2-devel-01 ; Leif >> Lindholm >> 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 ; Gao, Liming >> >>>> Cc: Zeng, Star ; Kinney, Michael D >> ; afish@apple.com; Tian, Feng >>>> ; edk2-devel-01 ; Leif >> Lindholm >>>> 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 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 ; Ard Biesheuvel >>>>>>> ; Kinney, Michael D >>>>>>> ; afish@apple.com >>>>>>> Cc: Tian, Feng ; Gao, Liming >> ; >>>>>>> edk2-devel-01 ; Leif Lindholm >>>>>>> >>>>>>> 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 { >>>>>>>> >>>>>>>> NULL|XXXPkg/Library/DepexLib/DepexLib.inf >>>>>>>> >>>>>>>> PcdDepexGuid|gEdkiiPlatformHasAcpiGuid >>>>>>>> } >>>>>>>> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf { >>>>>>>> >>>>>>>> NULL|XXXPkg/Library/DepexLib/DepexLib.inf >>>>>>>> >>>>>>>> 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 ; Kinney, Michael D >>>>>>> ; afish@apple.com >>>>>>>> Cc: Tian, Feng ; Laszlo Ersek >> ; >>>>>>> edk2-devel-01 ; Leif Lindholm >>>>>>> >>>>>>>> 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 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 >>> >