From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.97454.1674806066603466989 for ; Thu, 26 Jan 2023 23:54:26 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Nack9qE0; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: kraxel@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1674806065; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wU5S1xek1viw4g/6WJtJhY+FJ9z7ESdhVcUB3Wjxf4c=; b=Nack9qE0Sthe1LWjZKkJ/mU1AZ61nkGtFG3EGFDajPlrRuUGkOz+a1JGkU1hXKgX26L0LO zz8HGJcKPxKDfoRmICC6NOdIeb6sHrxQ0uZag6cyEBzD0UUbz5DUpDYnrgVSrfIbuyaBI9 Fyq3S355VlBQxIjalUBL6eM5X2HbJek= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-619-1NlIYX30PqC2Hha7zir1Vg-1; Fri, 27 Jan 2023 02:54:21 -0500 X-MC-Unique: 1NlIYX30PqC2Hha7zir1Vg-1 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 mimecast-mx02.redhat.com (Postfix) with ESMTPS id 27ACB801779; Fri, 27 Jan 2023 07:54:21 +0000 (UTC) Received: from sirius.home.kraxel.org (unknown [10.39.192.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D5FD02026D4B; Fri, 27 Jan 2023 07:54:20 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id 7EEFB1800606; Fri, 27 Jan 2023 08:54:19 +0100 (CET) Date: Fri, 27 Jan 2023 08:54:19 +0100 From: "Gerd Hoffmann" To: Min Xu Cc: devel@edk2.groups.io, Erdem Aktas , James Bottomley , Jiewen Yao , Tom Lendacky , Michael Roth Subject: Re: [PATCH V4 06/12] OvmfPkg/PeilessStartupLib: Build GuidHob for Tdx measurement Message-ID: <20230127075419.72x2sbvn2fcau3jw@sirius.home.kraxel.org> References: <20230127001106.2038-1-min.m.xu@intel.com> <20230127001106.2038-7-min.m.xu@intel.com> MIME-Version: 1.0 In-Reply-To: <20230127001106.2038-7-min.m.xu@intel.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Jan 27, 2023 at 08:11:00AM +0800, Min Xu wrote: > From: Min M Xu > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4243 > > 2 new functions are added in PeilessStartupLib/IntelTdx.c. > - BuildTdxMeasurementGuidHob > - InternalBuildGuidHobForTdxMeasurement > > These 2 functions build GuidHob for Tdx measurement. But you don't use them anywhere? The point of splitting the patches is not only to simplify review, but also to simplify testing (and in case a bug shows up later finding it with bisecting). So, current state of the code is: There are MeasureHobList() + MeasureFvImage(), doing measurement and logging in one go, using TpmMeasureAndLogData(). Problem is this doesn't work in SEC, so you want split. So, you add TdxHelperMeasureTdHob (doing the measurement part of MeasureHobList) and TdxHelperMeasureCfvImage (likewise doing the measurement part of MeasureFvImage) and logging both is handled by TdxHelperBuildGuidHobForTdxMeasurement(). So I think the series should have: (1) One or more patches doing cleanups (like reusing the struct). (2) A patch removing MeasureHobList and adding TdxHelperMeasureTdHob with the first half of TdxHelperBuildGuidHobForTdxMeasurement (3) A patch removing MeasureFvImage and adding TdxHelperMeasureCfvImage with the second half of TdxHelperBuildGuidHobForTdxMeasurement (4) A patch moving the code from PlatformInitLib to TdxHelperLib. (5) A patch moving the calls to TdxHelperMeasureTdHob and TdxHelperMeasureCfvImage to SEC. (6) A patch adding the Tdxhelper* calls to OvmfPkgX64. What is the reason to create a new TdxHelperLib btw.? Are there any problems with the code being in PlatformInitLib? take care, Gerd