From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web12.21453.1622969538618735627 for ; Sun, 06 Jun 2021 01:52:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=t+i+xeA2; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: min.m.xu@intel.com) IronPort-SDR: Jj24S2PAolJ6UYSADBs1lWPjQe3x4HDvXgIIzDil+ZY6LXbWTdGSjG3ybE+xYKzmpfQhDP3A7q nkMeCY4a9XsA== X-IronPort-AV: E=McAfee;i="6200,9189,10006"; a="191823476" X-IronPort-AV: E=Sophos;i="5.83,252,1616482800"; d="scan'208";a="191823476" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Jun 2021 01:52:16 -0700 IronPort-SDR: JWwoq7v60cxF5LFihn3dDbLCHPoRJtcN8YO1BgANZq5+PWR39qe5CpV1RPTTyVK3wQTA8bLWkC pmb0eT28yb2g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.83,252,1616482800"; d="scan'208";a="634412393" Received: from orsmsx602.amr.corp.intel.com ([10.22.229.15]) by fmsmga006.fm.intel.com with ESMTP; 06 Jun 2021 01:52:15 -0700 Received: from orsmsx601.amr.corp.intel.com (10.22.229.14) by ORSMSX602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Sun, 6 Jun 2021 01:52:15 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4 via Frontend Transport; Sun, 6 Jun 2021 01:52:15 -0700 Received: from NAM02-BN1-obe.outbound.protection.outlook.com (104.47.51.44) by edgegateway.intel.com (134.134.137.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2242.4; Sun, 6 Jun 2021 01:52:14 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=P2lN1lOT+jTZY3C3SCRyOQAaTPcGNTnj1UMGGN0uwSAQe9nYN00JN1gT+Au3WY5DX+RR3qs4iW+/aiWJk+ZE76X6A5UX/Fq9cQzmbo591EfJ4Uwh4oFMFF6g8RB62LlOsFS3kB1B5C4QCKFbAiaQBNv/4rnSFE4MaCRTYnGjlMYHf4EV2DmoORseHrt2rRgQHv2eLToroSMWwNK/W6VCD5MaWBYeNGaS+Xjfp75Dnn0JuVP7am4yiiimeHgvV2kH7tbbp1/qWskJ4eN9PA6mFgDav3pFgGjk8GWlJwAcar/0VjUl92j1MRC/KB/6dTemjQPA7e/AOOr0glaDYO19JQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=BKo0doPPslenXXh0ZD86AEbpT9qS8+4jlypMYToJ7Y4=; b=MH3tE3jiC17WTwkjvjnGn0BoXis261VJHzeCEUZ9pUplgNs6Zio2CTTq2+HElOUpPOrsuNCmbg0IFJHInwVpR1dSm4Up9ml1i6hqrJdk+bebZsjlyNrHNl1Gr62HBk/ORpQQ+cbO4w7z0cJtd9wbUE4+C7jhdzRWHbnQeEx8/+MUWs2k6p9daQlyyMe9lSqJLHlDF8DeJV5reOTbNzvAsy61w8kuazLU3PgNbVJO1cstvlWt4mcZxJVljkeladSuCQBlbTr1jo8kGtPZbCrxs4Wm6/bVISU1TRRe+09KTQ0il3AMYCoU8WP3LDNvQksoKloH88a8Ll7hqtj9TsX+YA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=BKo0doPPslenXXh0ZD86AEbpT9qS8+4jlypMYToJ7Y4=; b=t+i+xeA2yUbACxou7DhaX8u6rdomaOG8aYCEs98T4lmpMBNEHylXe7glx0wBSqMm8k/hj78vlFwrka3Y0QZ76sxHqC2YVB9z/ExEt3IeqRLDr6Dqzno5PkqpGCenIfoEsGeWX/yOTKmnm7kMEt7pwnpmvHuvsJUeT+/GekI9Ly8= Received: from PH0PR11MB5064.namprd11.prod.outlook.com (2603:10b6:510:3b::15) by PH0PR11MB4808.namprd11.prod.outlook.com (2603:10b6:510:39::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4195.23; Sun, 6 Jun 2021 08:52:12 +0000 Received: from PH0PR11MB5064.namprd11.prod.outlook.com ([fe80::b4be:3994:dd4d:7b9d]) by PH0PR11MB5064.namprd11.prod.outlook.com ([fe80::b4be:3994:dd4d:7b9d%8]) with mapi id 15.20.4195.029; Sun, 6 Jun 2021 08:52:12 +0000 From: "Min Xu" To: "devel@edk2.groups.io" , "lersek@redhat.com" , "Yao, Jiewen" , "rfc@edk2.groups.io" CC: "jejb@linux.ibm.com" , Brijesh Singh , Tom Lendacky , "erdemaktas@google.com" , "cho@microsoft.com" , "bret.barkelew@microsoft.com" , Jon Lange , Karen Noel , Paolo Bonzini , Nathaniel McCallum , "Dr. David Alan Gilbert" , Ademar de Souza Reis Jr. Subject: Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF Thread-Topic: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF Thread-Index: AddYf4DUPECuZ9ubQaOwhq0M0PgYbAAE6sKAAIM5BIA= Date: Sun, 6 Jun 2021 08:52:11 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=intel.com; x-originating-ip: [101.229.202.75] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 2c099226-ae0e-4b9b-196c-08d928c8602d x-ms-traffictypediagnostic: PH0PR11MB4808: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:5236; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: JMvgQj8JGbADkS3x5wcC0/gzGKkTOkSskvdlqp9lX7+tPWL6zgwRd8dLj7TWd0y3ZcpGOzCbTn5vWvDDYXtQ1Ui3vkIgXEd4mPHW7SG7gaUy3kek7btu18VlkgFFukAnKEpw5SQs9NCPazDLxnUPs7SaAnxMoXbz8n+qDiKjo5q8n4vYxTf6gZae7zDOoE0NONqIYXLbQJGMXoRLosNUwER+6Xpzj5ANIPs3u/TJYabqGtbmY5IEl+ybcz06tnllbrfrG4wXwos83JTvT6SIgRHD43TbYH8f8lpmTLUbhi5S1k1vel7pCjgKx7MQMLt2n7Q921f0YBsVLS6j6ywohoc+ELeR9ux5aR2QeZpReuryVSN0NM92DpcVeNYDwwQxNZibyUDROfCSRTDa0dvOKHQaO+UJyxta5V0O3FUBzeSziUAZVI+Lzlx4JdaZ9ZqJ8Ki0wgkv6xE3ydwnnYZrCwqseko/5l3cwg32X4aymYDPPI2WBUjLLsBonhghWUrGLLov5twTjmAQdtsOii7+IdcErpp+s+z49UxBG2LsjhmTiCmLNWAyEcVu38zJNutHlQxlX2Wrxs93yJH5yJuju9+61IWkonh6Kj4Ics/+Z5ORU85zXy2hAwQOzSA5WGfhw8Lnapd/MmqFWgG56+o/Ufu2KMKCKVbfn5xJz1Mdlbjpkdu8fzFWXTBqqTbnLa0ttGgmHnjmOjqpv7C/eICEYV76r2ulq2c0/7id+g0A0utfzwva0lMqEGid7J6Jo0zSp/NQC9LmAwPH029wRVW6Ag== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:PH0PR11MB5064.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(366004)(136003)(39860400002)(346002)(376002)(396003)(7416002)(33656002)(478600001)(71200400001)(53546011)(2906002)(6506007)(7696005)(55016002)(9686003)(38100700002)(19627235002)(54906003)(76116006)(316002)(966005)(4326008)(186003)(30864003)(86362001)(66446008)(66946007)(66476007)(52536014)(66556008)(64756008)(5660300002)(8676002)(8936002)(110136005)(122000001)(83380400001)(26005);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: =?us-ascii?Q?ekVRpWWo3El7Qmn+AcUvxEt3I0i8In6f6/bUKi6tjtRnDFKV2qmHBR0kjcO/?= =?us-ascii?Q?kjmSbYo/fWohTnza5JMmBx00jwJIXpLTWUKgn4aBXsgfAOTDS/riVBT1i+P6?= =?us-ascii?Q?9QyhikCaCzkJYAOsbOwWtrOlXehQm3Lzwfs7LNwzn4GTdYJ9ytn3T1pqcL8t?= =?us-ascii?Q?ZfY5BZiiI5phaT9OKpXNUgNA88AUqfTLDmb9DLP1VWWqKGDidKRyWyylNrN/?= =?us-ascii?Q?Qw+e06syo9orK+FHxmpp/OjYo9hFjhfeyzRpgyWC8OEXeTVdU0MKL7rDm7qG?= =?us-ascii?Q?Hf8yQ3NjQpjU2cdktmcRG+FOGRecmwiOyi6gBa6+I2+rg3jFPQ9AhEzrEYCU?= =?us-ascii?Q?lT4i9JqOQITsC7HfprEXEPJXZEsqFw2tWt6/t98i6Dl/bjj06B7olgEQ1haw?= =?us-ascii?Q?byoUgThlzdyeQ6JR/nPS3AHNUb2gXST/KovOJcxr+6T8uPhEC+NrGFDBv8Ug?= =?us-ascii?Q?e03FnCanoEVi0opbhFz318ZKC5bLqZOiYvO5W+2AFkeoxt91jCpw+swmzlOm?= =?us-ascii?Q?llfoeRMsNdSOc5MFuZIZGXnnBHseBWt6jREzWa+1TwiqIdquc12qMmledP7Y?= =?us-ascii?Q?ghmn4dHQ3PmOdrjWFnE7s1rK0HNglOt+IjPOPeoIDQC5jLtkZWFsJfNAxCRQ?= =?us-ascii?Q?tT23OpGylR7iw20QkJ19C0Kkd1r01jmW0Aress8InA/8ldsnQRMYrlKdTWGw?= =?us-ascii?Q?nN8nT1a8R1xVV4E63IZ22tBK9x8Q+zNGS3Jhecf+a1gD5xa1khpDw9LJVtjb?= =?us-ascii?Q?pCeoXxPho2ZPr8mGNTjfMfUSnG799avzIGRj7b+A2iC27ETwL4qlvpO94eh9?= =?us-ascii?Q?tsJ79WOp548l1wIm50/1pR7iKJveYM2dETx2ZKrMN0y4gNSNh9I0sPSodxMz?= =?us-ascii?Q?zv0xzZesI+XuHNWld2ys7rHf/fTT1Q8VSOiOgQeoCVo3j6+miTVSMM0TOGxr?= =?us-ascii?Q?pQ79z4v3byz8UhjuAo3TMDPKTl2HRYuPwsRl32xxQFw7LQhoko4u5WXqV+E+?= =?us-ascii?Q?wlKhuHtJ911HA+3VJ9tYc/SSx9tLnLZzubCfFsG4n3euuirDQty7odRRufiW?= =?us-ascii?Q?aggqiw5Cc+aL4g8+tT8tN5aYcbiQsezSih8cgXfBt3IvAF6tNBGVzQMlG4W2?= =?us-ascii?Q?YVw+ac220w4CajWgEIZCZfCLa9cw9DnKj1xlTRVna5/PT+PGL3s1lQ8VqGSy?= =?us-ascii?Q?nHQRcSpEemcryo4lvGgMT4EABC0ywuf3uJbVxsnoroZk40UBnUtxZ4eOTamt?= =?us-ascii?Q?qAW1JOnQm46R+OYG/o4wwSZIM+MKqjGrGURJ287Z3q0t/zLuD5gkFvimRVve?= =?us-ascii?Q?MVlDCCdNNc7/K0KApw01RxOg?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5064.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 2c099226-ae0e-4b9b-196c-08d928c8602d X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jun 2021 08:52:12.0111 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: P2mPGLHJcjGYpAAWg2M+f9L3fXDLgc23kAsf4hKza8faiiNsWSaz9dSkgUHhxyyYOzayF+enbi9LgEMeoJVeOw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4808 Return-Path: min.m.xu@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable On June 4, 2021 12:12 AM, Laszlo wrote: > On 06/03/21 15:51, Yao, Jiewen wrote: > > Hi, All > > We plan to do a design review for TDVF in OVMF package. > > > > > > The TDVF Design slides for TinaoCore Design Review Meeting (Jun 11) is > > now available in blow link: > > https://edk2.groups.io/g/devel/files/Designs/2021/0611. > > > > The Bugzilla is https://bugzilla.tianocore.org/show_bug.cgi?id=3D3429 > > > > > > > > You can have an offline review first. You comments will be warmly > > welcomed and we will continuously update the slides based on the > > feedbacks. >=20 > Resending my earlier comments in this mailing list thread, with the feedb= ack > inserted at the proper spots that has been given in the off-list thread s= ince > then: >=20 Continue my comments from here. > *** Slide 35-36: DXE phase >=20 > (16) "Some DXE Drivers not allowed to load/start in Td guest -- Network > stack, RNG, ..." >=20 > Same comment as (several times) above. The Linuxboot project is a good > example for eliminating cruft from DXEFV (in fact, for eliminating most o= f the > DXE phase). In a TDX environment, why include drivers in the firmware bin= ary > that are never used? Meanwhile, DXEFV in OVMF grows by a MB every 1.5 > years or so. Again, remove these drivers from the DSC/FDF then, and it ne= eds > to be a separate platform. >=20 It is because of the *one binary* so that we have to include all the driver= s in the firmware binary. Some of the DXE drivers are not called in TDX, but they're= dispatched in Non-Tdx environment. I will explain more about this topic in next version of design slides. Also I would wait for the conclusion to the *one binary*. > (17) "Other DXE Phase drivers -- [...] AcpiPlatformDxe" >=20 > I'm not sure what this section is supposed to mean. Other DXE phase drive= rs > included, or excluded? Without AcpiPlatformDxe, the guest OS will not see > QEMU's ACPI content, and will almost certainly malfunction. >=20 Sorry for the confusion about the title. This slides means Other DXE phase = drivers including AcpiPlatformDxe is included. I will update the slides to be more = clear and accurate. > ... Back from slide 48: ignore this for now, I'll comment in more detail = later. >=20 >=20 > *** Slide 37: DXE Core >=20 > (18) says "SMM is not supported in Td guest" -- how is the variable store > protected from direct hardware (pflash) access from the guest OS? > Without SMM, the guest OS need not go through gRT->SetVariable() to > update authenticated non-volatile UEFI variables, and that undermines > Secure Boot. >=20 Let me explain the SMM and Secure boot in TDX like below: 1) TDX doesn't support virtual SMM in guest. Virtual SMI cannot be injected into TD guest. 2) SMI/SMM is used to manage variable update to avoid expose Flash direct. So SMM is not must-to-have for secure boot, but help to mitigate the se= curity risk. 3) We don't trust VMM. That is why we need TDX.=20 4) If you trust VMM to emulate SMM, then you don't need TDX. > Note that, while SEV-ES has the same limitation wrt. SMM, the > "OvmfPkg/AmdSev/AmdSevX64.dsc" platform doesn't even include the > Secure Boot firmware feature. For another example, the OVMF firmware > binary in RHEL that's going to support SEV-ES is such a build of > "OvmfPkgX64.dsc" > that does not include the Secure Boot feature either. >=20 > But in this TDX slide deck, Secure Boot certificates are embedded in the = CFV > (configuration firmware volume) -- see slide 11 and slide 16 --, which > suggests that this platform does want secure boot. >=20 Yes, TDVF does support Secure Boot. > ... Back from slide 48: I'm going to make *additional* comments on this, > when I'm at slide 48, too. >=20 > The rest of this slide (slide 37) looks reasonable (generic DXE Core chan= ges -- > possibly PI spec changes too). Yes, there is new attribute (EFI_RESOURCE_ATTRIBUTE_ENCRYPTED) in PiHob.h >=20 >=20 > *** Slides 38 through 39: >=20 > These seem reasonable (TdxDxe assumes some responsibilities of > OvmfPkg/PlatformPei) >=20 >=20 > *** Slides 40 through 42: >=20 > *If* you really can implement TDX support for the IOMMU driver *this* > surgically, then I'm OK with it. The general logic in the IOMMU driver wa= s > truly difficult to write, and I'd be seriously concerned if those parts w= ould > have to be modified. Customizing just the page encryption/decryption > primitives for TDX vs. SEV is OK. Actually all the changes we do in IOMMU is to customize the page encryption= =20 / decryption primitive for TDX and SEV. We will probe the Td or Non-Td in run-time, then the corresponding APIs will be called. =20 >=20 >=20 > *** Slides 43 through 47: >=20 > (19) Slide 46 and slide 47 are almost identical. Please consolidate them = into a > single slide. >=20 Slide 46 is of Td measurement, like TpmMeasurement.=20 SecurityPkg/Library/DxeTpmMeasurementLib Slide 47 is of Measure boot.=20 SecurityPkg/Library/DxeTpm2MeasureBootLib I will refine the two slides to make it more clear. Thanks for reminder. > (20) the TPM2 infrastructure in edk2 is baroque (over-engineered), in my > opinion. It has so many layers that I can never keep them in mind. When w= e > added TPM support to OVMF, I required commit messages that would help us > recall the layering. In particular, please refer to commit 0c0a50d6b3ff > ("OvmfPkg: include Tcg2Dxe module", 2018-03-09). Here's an > excerpt: >=20 > TPM 2 consumer driver > | > v > Tpm2DeviceLib class: Tpm2DeviceLibTcg2 instance > | > v > TCG2 protocol interface > | > v > TCG2 protocol provider: Tcg2Dxe.inf driver > | > v > Tpm2DeviceLib class: Tpm2DeviceLibRouter instance > | > v > NULL class: Tpm2InstanceLibDTpm instance > (via earlier registration) > | > v > TPM2 chip (actual hardware) >=20 > The slide deck says that EFI_TD_PROTOCOL is supposed to reuse the > EFI_TCG2_PROTOCOL definition. If that's the case, then why don't we push > the TDX specifics (more or less: the replacement of PCRs with RTMR) down > to the lowest possible level? >=20 > Can we have "Tpm2InstanceLibTdxRtmr", plugged into the same Tcg2Dxe.inf > driver? >=20 > If not, can we have a new TdTcg2Dxe.inf driver, but make it so that it in= stall > the same protocol as before (EFI_TCG2_PROTOCOL -- same old protocol > GUID)? Then DxeTpmMeasurementLib doesn't have to change. >=20 > As long as there is *at most* one EFI_TCG2_PROTOCOL instance published in > the protocol database, DxeTpmMeasurementLib should be fine. In SEV* > guests, the standard Tcg2Dxe driver provides that protocol. In TDX guests= , > TdTcg2Dxe.inf should provide the protocol. Arbitration between the two ca= n > be implemented with the pattern seen in the following > commits: >=20 > 1 05db0948cc60 EmbeddedPkg: introduce EDKII Platform Has ACPI GUID > 2 786f476323a6 EmbeddedPkg: introduce PlatformHasAcpiLib > 3 65a69b214840 EmbeddedPkg: introduce EDKII Platform Has Device Tree > GUID > 4 2558bfe3e907 ArmVirtPkg: add PlatformHasAcpiDtDxe >=20 > The basic idea is that Tcg2Dxe can have a depex on a new "running in SEV" > protocol GUID, and TdTcg2Dxe can have a depex on a new "running in TDX" > protocol GUID. A separate platform driver can install the proper GUID -- > possibly *neither* of those GUIDs. >=20 > And, we don't have to change the depex section of > "SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf" for this; we can implement a librar= y > instance with an empty constructor, but a non-empty depex, and then hook > this lib instance into Tcg2Dxe.inf via module scope NULL lib class overri= de in > the DSC file. Basically we could forcibly restrict Tcg2Dxe's DEPEX by mak= ing it > inherit the new DEPEX from the library. >=20 This is a big topic. I need to discuss it internally first then give my com= ments. Thanks for your patience. >=20 > *** Slide 48: DXE Phase -- Other Modules >=20 > Regarding IncompatiblePciDeviceSupportDxe: the proposed update sounds > plausible and simple enough. >=20 > (21) "AcpiPlatformDxe: Support for MADT/ACPI addition to report Td Mailbo= x > entry" >=20 > Firmware-owned tables must not be installed from this driver. >=20 > Please refer to my "Xen removal" patch set again, for > , which I mention > above in point (14). As part of the Xen removal, the AcpiPlatformDxe driv= er in > OvmfPkg is significantly trimmed: all unused (dead) cruft is removed, > including any ACPI table templates that are built into the firmware. >=20 > OvmfPkg/AcpiPlatformDxe is responsible *solely* for implementing the > client side of the QEMU ACPI linker/loader. >=20 > If you need to prepare & install different ACPI tables, please do it else= where, > in another DXE driver. A bunch of other firmware modules do that (NFIT, I= BFT, > BGRT, ...). >=20 > For example, the OvmfPkg/TdxDxe DXE_DRIVER is supposed to be launched > early in the DXE phase, via APRIORI section -- please consider registerin= g a > protocol notify in that driver, for EFI_ACPI_TABLE_PROTOCOL, and when it > becomes available, install whatever > *extra* tables you need. >=20 > Note that if you need to *customize* an ACPI table that QEMU already > provides, then you will have to modify the ACPI generator on the QEMU sid= e. > It is a design tenet between QEMU and OVMF that OVMF include no business > logic for either parsing or fixing up ACPI tables that QEMU provides. > AcpiPlatformDxe contains the minimum (which is already a whole lot, > unfortunately) that's necessary for implementing the QEMU ACPI > linker/loader client in the UEFI environment. >=20 > The slide deck mentions MADT, which is also known as the "APIC" table -- > and indeed, QEMU does provide that. (See acpi_build_madt() > [hw/i386/acpi-common.c].) So if TDX needs MADT customizations, that > should go into QEMU. >=20 Thanks for reminder. We will exam the implementation of MADT/ACPI carefully= . > (22) EmuVariableFvbRuntimeDxe >=20 > Ouch, this is an unpleasant surprise. >=20 > First, if you know for a fact that pflash is not part of the *board* in a= ny TDX > setup, then pulling >=20 > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >=20 > into the firmware platform is useless, as it is mutually exclusive with >=20 > OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf >=20 > (via dynamic means -- a dynamic PCD). >=20 > Note that the FDF file places QemuFlashFvbServicesRuntimeDxe in APRIORI > DXE when SMM_REQUIRE is FALSE. This driver checks for pflash presence, > and lets EmuVariableFvbRuntimeDxe perform its in-RAM flash emulation only > in case pflash is not found. >=20 > So this is again in favor of a separate platform -- if we know pflash is = never > part of the board, then QemuFlashFvbServicesRuntimeDxe is never needed, > but you cannot remove it from the traditional DSC/FDF files. >=20 > Second, EmuVariableFvbRuntimeDxe consumes the PlatformFvbLib class, for > the PlatformFvbDataWritten() API (among other things). This lib class is > implemented by two instances in OvmfPkg, PlatformFvbLibNull and > EmuVariableFvbLib. The latter instance allows Platform BDS to hook an eve= nt > (for signaling) via "PcdEmuVariableEvent" into the > EmuVariableFvbRuntimeDxe driver. >=20 > In old (very old) QEMU board configurations, namely those without pflash, > this (mis)feature is used by OVMF's PlatformBootManagerLib to write out a= ll > variables to the EFI system partition in a regular file called \NvVars, w= ith the > help of NvVarsFileLib, whenever EmuVariableFvbRuntimeDxe writes out an > emulated "flash" block. For this purpose, the traditional OVMF DSC files = link > EmuVariableFvbLib into EmuVariableFvbRuntimeDxe. >=20 > But it counts as an absolute disaster nowadays, and should not be revived= in > any platform. If you don't have pflash in TDX guests, just accept that yo= u > won't have non-volatile variables. And link PlatformFvbLibNull into > EmuVariableFvbRuntimeDxe. You're going to need a separate > PlatformBootManagerLib instance anyway. >=20 > (We should have removed EmuVariableFvbRuntimeDxe a long time ago from > the traditional OVMF platforms, i.e. made pflash a hard requirement, even > when SMM is not built into the platform -- but whenever I tried that, Jor= dan > always shot me down.) >=20 > My point is: using EmuVariableFvbRuntimeDxe in the TDX platform may be > defensible per se, but we must be very clear that it will never provide a > standards-conformant service for non-volatile UEFI variables, and we must > keep as much of the \NvVars mess out of EmuVariableFvbRuntimeDxe as > possible. This will require a separate PlatformBootManagerLib instance fo= r > TDX anyway (or maybe share PlatformBootManagerLibGrub with > "OvmfPkg/AmdSev/AmdSevX64.dsc"). >=20 > Apart from the volatility aspect, let's assume we have this in-RAM emulat= ed > "flash device", storing authenticated UEFI variables for Secure Boot purp= oses. > And we don't have SMM. >=20 > What protects this in-RAM variable store from tampering by the guest OS? > It's all guest RAM only, after all. What provides the privilege barrier b= etween > the guest firmware and the guest OS? >=20 Thanks Laszlo. I will carefully read your comments and discuss it internall= y first. >=20 > *** Slide 50: Library >=20 > (23) Should we introduce Null instances for all (or most) new lib classes= here? > Code size is a concern (static linking). If we extend a common OvmfPkg > module with new hook points, it's one thing to return from that hook poin= t > early *dynamically*, but it's even better (given separate platforms) to a= llow > the traditional platform firmware to use a Null lib instance, to cut out = all the > dead code statically. > Yes, agree. We will introduce the NULL instance for the new libs. =20 >=20 > *** Slides 51 through 52 >=20 > Seems OK. >=20 >=20 > *** Slide 53: >=20 > (24) It might be worth noting that BaseIoLibIntrinsic already has some SE= V > enlightenment, as the FIFO IO port operations (which normally use the REP > prefix) are not handled on SEV. I don't have an immediate idea why this > might matter, we should just minimize code duplication if possible. >=20 Agree that we should minimize the code duplication if possible. Before we start to enable IoLib for Tdx, we search out the EDK2 and find: For BaseIoLibIntrinsicSev.inf, it is imported in OvmfPkg, such as - OvmfPkg/OvmfXen.dsc - OvmfPkg/Bhyve/BhyveX64.dsc - OvmfPkg/OvmfPkgIa32.dsc - OvmfPkg/OvmfPkgX64.dsc - OvmfPkg/OvmfPkgIa32X64.dsc - OvmfPkg/AmdSev/AmdSevX64.dsc But for BaseIoLibIntrinsic.inf it seems it is not imported in any dsc in Ov= mfPkg. BaseIoLibIntrinsic is the right IoLib base that we can enable TDX. But it doesn't support SEV for the FIFO IO port operations.=20 BaseIoLibIntrinsicSev handles the FIFO IO on SEV. But we have an open about the name.=20 Anyway we agree code duplication should be minimized. >=20 > *** Slides 54-56: >=20 > No comments, this stuff seems reasonable. >=20 >=20 > *** Slide 57: MpInitLib >=20 > I don't know enough to give a summary judgement. >=20 >=20 > All in all, I see the controversial / messy parts in the platform bringup= , and > how all that differs from the traditional ("legacy") OVMF platforms. I ad= mit I > *may* be biased in favor of SEV, possibly because SEV landed first -- if = you > find signs of such a bias in my comments, please don't hesitate to debunk > those points. Yet my general impression is that the early bringup stuff i= s > significantly different from everything before, and because of this, a > separate platform is justified. >=20 > Definitely separate from the traditional OVMF IA32, IA32X64, and X64 > platforms, and *possibly* separate from the "remote attestation" > AmdSevX64.dsc platform. I would approach the TDX feature-set in complete > isolation (exactly how Intel commenced the work, if I understand correctl= y), > modulo obviously shareable / reusable parts, and then slowly & gradually > work on extracting / refactoring commonalities. >=20 > (But, given my stance on Xen for example, I could disagree even with the > latter, retroactive kind of unification -- it all boils down to shared de= veloper > and user base. Component sharing should reflect the community structure, > otherwise maintenance will be a nightmare.) >=20 > Thanks > Laszlo >=20 Some of the comments need be discussed internally in intel first. So I cann= ot answer all your comments right now.=20 Thanks again Laszlo for your valuable review comments! Min