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 C72B221BC6A7E for ; Tue, 28 Mar 2017 00:49:54 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2F2F14E4D1; Tue, 28 Mar 2017 07:49:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2F2F14E4D1 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 2F2F14E4D1 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-89.phx2.redhat.com [10.3.116.89]) by smtp.corp.redhat.com (Postfix) with ESMTP id DB9468C536; Tue, 28 Mar 2017 07:49:51 +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> Cc: "Zeng, Star" , "Kinney, Michael D" , "afish@apple.com" , "Tian, Feng" , edk2-devel-01 , Leif Lindholm From: Laszlo Ersek Message-ID: <28eb520f-2c2a-8743-80e6-dc68e79decf5@redhat.com> Date: Tue, 28 Mar 2017 09:49:50 +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: <4A89E2EF3DFEDB4C8BFDE51014F606A14D70446E@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 28 Mar 2017 07:49:54 +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: Tue, 28 Mar 2017 07:49:55 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 >