From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 1A9AD210E436F for ; Thu, 9 Aug 2018 09:09:35 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5889EB5C2; Thu, 9 Aug 2018 16:09:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-52.rdu2.redhat.com [10.10.121.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id 355072026D66; Thu, 9 Aug 2018 16:09:32 +0000 (UTC) To: "Zhang, Chao B" , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: "Zeng, Star" , "edk2-devel@lists.01.org" , "Gao, Liming" References: <551258016.93465.1533144825411.JavaMail.zimbra@lsd.ufcg.edu.br> <1169526465.93534.1533145845454.JavaMail.zimbra@lsd.ufcg.edu.br> <00859174-589e-92d8-5f97-dcf9d735424c@redhat.com> <23c3c4c3-461f-7e6a-a8ca-0574021c1ccf@redhat.com> <1029983813.116236.1533303568344.JavaMail.zimbra@lsd.ufcg.edu.br> <70d053a8-eeef-8a9f-0454-9374f716f28e@redhat.com> <57c7039d-2012-7bcc-1e0b-a0abab362be6@redhat.com> From: Laszlo Ersek Message-ID: Date: Thu, 9 Aug 2018 18:09:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 09 Aug 2018 16:09:34 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 09 Aug 2018 16:09:34 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Aug 2018 16:09:36 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 08/09/18 17:46, Zhang, Chao B wrote: > Hi Laszlo: > We seriously considered such dependency change in design. The library is shared between DXE & PEI. So PCD is the generic way to share the data and reduce real register touch. > Therefore, the problem is that Library can’t be used by SEC anymore. The decision is, since there is no SEC usage so far, we’d like to change Lib type first & Split it when there is real usage. > I think your suggestion to split the patch & provide more detailed information in log is good. I will follow this rule later on. Right, I agree that, if the goal was to exclude SEC from the permitted client module types, then the LIBRARY_CLASS restriction change was valid. I also agree that placing the LIBRARY_CLASS change into the exact same patch was correct, as the introduction of the PCD dependency. In fact, in this case, separating the LIBRARY_CLASS restrictions to a different patch would have been wrong -- because then a commit would exist where the lib instance is no longer suitable for SEC, but the LIBRARY_CLASS restriction wouldn't enforce that. Therefore, in this case, the missing bit was really only a better the commit message -- basically, your above argument, about excluding SEC. ... A workflow comment again: this is now the 2nd time in a short timeframe that I realize that Intel performs careful investigation in the background, writes the corresponding patch, and then omits the entire investigation from the commit message. PLEASE, don't do that. You are throwing away your own work, and you are wasting the time of the community. Commit messages embody communication with people. That is the important part. That is what makes or breaks Open Development. The code in the body of the patch is just a consequence of the commit message. Thanks, Laszlo > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Thursday, August 9, 2018 10:56 PM > To: Marc-André Lureau ; Zhang, Chao B > Cc: Zeng, Star ; edk2-devel@lists.01.org; Gao, Liming > Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF > > On 08/09/18 16:09, Marc-André Lureau wrote: >> Hi >> >> On Mon, Aug 6, 2018 at 5:26 PM, Zhang, Chao B > wrote: >>> Hi Ricardo >>> I double checked OVMF Debug Build. All the 2 PCDs are already built as Dynamic PCD. There should be no problem >>> Setting & Getting these PCD as Dynamic. We also verified this feature on several real hardware platforms with same configuration. >>> No issue reported. >>> Can you share me the boot log? >>> >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek >>> Sent: Friday, August 3, 2018 10:46 PM >>> To: Ricardo Araújo >; Zhang, Chao B > >>> Cc: edk2-devel@lists.01.org; Zeng, Star >; Gao, Liming > >>> Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF >>> >>> On 08/03/18 15:39, Ricardo Araújo wrote: >>>> Hi folks, sorry for the delay! >>>> >>>> I've just applied the patch and got the same error message and empty PCRs. >>> >>> Thanks for the feedback -- although it's not the kind I had hoped for :) >>> >>> I have now filed "[regression] SecurityPkg commit f15cb995bb38 breaks >>> TPM2_ENABLE in OvmfPkg": >>> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=1075 >>> >>> Ricardo, please consider registering in the TianoCore Bugzilla, and >>> adding yourself to the CC list of BZ#1075. >>> >>> For now, I have assigned the BZ to Marc-André, for triaging / analysis. >>> swtpm is not set up on my end, and the TPM2 enablement for OvmfPkg was >>> contributed by Marc-André. Marc-André, are you OK with this? The BZ >>> assignment is about root-causing the issue, at the moment. >> >> That fixes the problem for me: >> >> - Constructor = Tpm2DeviceLibConstructor >> + CONSTRUCTOR = Tpm2DeviceLibConstructor > > Nice! \o/ > >> >> It looks to me like the patch "SecurityPkg: Cache TPM interface type >> info" could use more reviews. >> >> Fwiw, I also question why that change (just the line above) was necessary: >> >> - LIBRARY_CLASS = Tpm2DeviceLib >> + LIBRARY_CLASS = Tpm2DeviceLib|PEIM DXE_DRIVER >> DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER > > It's usually a good idea to spell out the client module types that are > permitted to consume the specific library instance, for a given library > class requirement. Different module types have different restrictions > and devices at their disposal, in their respective environments / > firmware phases, and library instances may be specific to those > restrictions / devices. > > In this specific case, a PcdLib dependency (more, precisely, a dynamic > PCD dependency) was added to the library instance, and so it might make > sense to restrict the library instance to module types whose > environments (their entry point functions anyway) support dynamic PCDs. > > I do agree though that this change should have been made either in a > separate patch (if the change isn't closely related to the PCD > dependency), *or* (if it is) it should have been explained / justified > specifically, in the commit message. The commit message is very lacking > indeed. > > Thank you for tracking this down! > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >