From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web11.16662.1681492752614081502 for ; Fri, 14 Apr 2023 10:19:12 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=Q+pYnOG1; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: ray.ni@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1681492752; x=1713028752; h=from:to:subject:date:message-id:references:in-reply-to: mime-version; bh=WPscN8HhgE4VwDAQMqAOUcLRfRvPKo2e0eNVIijQxJE=; b=Q+pYnOG1D3HN8kpYZWaJI6+9BByN8wGKRMfdOOkOzOPUTnF4J+BlwhXq Y49hi1A8oE6E2lAH6zHoeKGjs3uzMfxps/IhbekxzaFO5QKcywNY+ggmX 8WMuN90v0p33mICCOD/ONLoeg4jTJbx7IqqeQdg9M61BxyrsPj8xgasqu b5dAZxtq9ug47WRkOIPYYTk1FC6sYBSTpwDpd5CBS/nlLMDnvTOdYeSrv I0z9NL7VFOPO5Cv/rvkh8uBCfrJkgYhpKzjLMvqYxyToVn9rSukpeN2H5 Q4kD1L9Tfm7te9oxD90uqPaXUJ1CMuio5GaVm9LMyCuj39EodNz+8yIn+ A==; X-IronPort-AV: E=McAfee;i="6600,9927,10680"; a="372385622" X-IronPort-AV: E=Sophos;i="5.99,197,1677571200"; d="scan'208,217";a="372385622" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Apr 2023 10:19:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10680"; a="1019651989" X-IronPort-AV: E=Sophos;i="5.99,197,1677571200"; d="scan'208,217";a="1019651989" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by fmsmga005.fm.intel.com with ESMTP; 14 Apr 2023 10:19:11 -0700 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Fri, 14 Apr 2023 10:19:11 -0700 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23 via Frontend Transport; Fri, 14 Apr 2023 10:19:11 -0700 Received: from NAM04-DM6-obe.outbound.protection.outlook.com (104.47.73.45) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.23; Fri, 14 Apr 2023 10:19:10 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=h/j3nKOf4aaKoTJeN/tvyjrSaThGeu4SrOpHIKH079YYSPlwZ+7tp+MTMFHJh0ABa9uAc6DqP5in9TPJeddaTpgoyLx+G+cGXE9pRrkr4pPK75T5GnsFe8VRzM9VvStXavXH8XAReMb3b30tQ7eTzYtdmsIqn28QqmsiC3hmbj+PcT2cRks6y30+JxcXzM5bdnOOcPplTAetreYfw9mHmaihFQorw9MGjrkLgp2pgIVR3raRUbOf3FZQyEg3TclEWQDy+AH6+VTK097Bk/PyeL7kBHipJkE4XhIcP9gSM3YfyS7wTLZlSdRZpS12JbThyiwG8kmlrs8qzvUuThTRdg== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=v54d9JBRaElXHy1xgqTj1OVPDDPQu20zkgGt4WjxMhA=; b=OtwAc85w3QVQfIOmQWzNXqQvw1vg+M+uamESMnPm3EjUs1Gqc6jkih89AZWOzzcPg/FcH+OYUU687aU3R0lpyMCVaGKX9cdiVUhrKmZ6k0XmOncdgwwl8sRdpydiwPwCcHPiGXCjy3/96Zh1JVZTS47fE1L6JMSkliUAV49jdp8i0gKld5df9Rygdd1s9wffh9sdRaM3FEt2i/cDetCaHjRDR/rUieeMv/uoH0gYrJDRdTEIuGM10ar+BI0vRBz7hYbnxH+NzzKo5HWEfiXzu0DD7SnVkA7d7zJ8yqCMPUs6b3J4QBjQA1taPVM0vxsaD4oT3DXDmD35eJkedELMZQ== 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 Received: from MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by PH7PR11MB7480.namprd11.prod.outlook.com (2603:10b6:510:268::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6298.30; Fri, 14 Apr 2023 17:19:07 +0000 Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::892b:b8e6:bab7:635d]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::892b:b8e6:bab7:635d%5]) with mapi id 15.20.6298.030; Fri, 14 Apr 2023 17:19:05 +0000 From: "Ni, Ray" To: "devel@edk2.groups.io" , "thomas.lendacky@amd.com" , "Tan, Dun" Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table Thread-Topic: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table Thread-Index: AQHZbRyoGhev/k0Lh0ak5MT8foi7/q8nX/cggACu7wCAALRekIAAqcYAgADTs3CAAJNVAIAAOr8O Date: Fri, 14 Apr 2023 17:19:05 +0000 Message-ID: References: <1755241E6695EAE7.1885@groups.io> <2301e275-1f69-e5c0-997d-d967264aa590@amd.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|PH7PR11MB7480:EE_ x-ms-office365-filtering-correlation-id: c1ed8eec-1ca1-4cb8-32da-08db3d0c59ce x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: K+1lAIUAXivMo1GNRQMAAbdI4ItadEFQ0OhwKxtPH2ZN2JvKnnrOsgE+mSVxKMHQrECWB+zQ4pQJhfQ+wSN6NVZDEWN1Bn+wW0puSD8of+plw7qsHYE45vaeCBon+WJs/8RL7mMV3mf6odFAXzKug4BDSNlXiZssflMEGpuBXBuzEHNJuUHEE40b0zwmnoawLG9p1jkRhM+6csLtGJ95BrCnGWhi2DMBzuLpeKXyYZt5o8/4vXBru3eqCCuy5lo0/Ng6UKo2nwT6aMYdr4+/ljKYfSu9Zvc+WLxntCJYPUL7ZdbmJ9yBwCr/r9yibdNW2w8kWD5P5l6i0bCc4Wtl63SQjb2ahknApbn8U/7jr8uC7mkOOiJpC83od/iEnJJzZu99HU55/OVnO2J2SA0ddzoLQCu95WKBqankzPhWDG4AEoRxiysfo7yu0ZJgh2jhSX3TkJFR5ZMiUEZXQsoRmBDZLQgjnBZ58NEfIg7Uv5AIrEE540V0yTNTrMoZZRJ1Illw8GlAuWogGS3PW72bk9G91rxIFvc44l2eJuCvjD8/2OeO9gg0PuXYh4yw2IC0MA8CYToheFqcIcCGemtfQ5TMY2DQqXCx2NVrChdbRz3y0Dur0LBm6BpxRNWaiBJ9MJSbNFWb8zbS/a3Q84lZ6w== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MN6PR11MB8244.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(39860400002)(366004)(396003)(376002)(136003)(346002)(451199021)(110136005)(9686003)(53546011)(26005)(6506007)(6636002)(55016003)(66899021)(186003)(83380400001)(966005)(71200400001)(7696005)(5660300002)(33656002)(41300700001)(82960400001)(316002)(8936002)(8676002)(52536014)(38100700002)(38070700005)(86362001)(122000001)(478600001)(66946007)(66446008)(76116006)(64756008)(91956017)(66556008)(66476007)(2906002)(30864003)(21615005)(166002)(15650500001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?J0S+Y4LK/CuYU587j7YJw1FJ25qfhPev6opztvWCREKHhdTSljd8W3T/VSFK?= =?us-ascii?Q?7tE65b4HDJEw/ckU3PzZY9RpFaZPNbQIYoWzAbvBACCt5IDGJai4jAQIx7j0?= =?us-ascii?Q?lggOs4A2Y8nEwpkd5fiPvxwgmGucxvWv7o9P+xfV0lEfE6Qa7MK/T7XX734U?= =?us-ascii?Q?8yh63YCT8nNSmzNJdk9Tme/tbGI413nZb6qzpgE4XPO5DZnaLkN7C38K8B6E?= =?us-ascii?Q?gZ9ZfGhQRp/c4g7Q3292+/mooOd+tukGIHfJltdhTeLAHULFVI1yP+IHxh5u?= =?us-ascii?Q?OMzPHX17D285OFU17/N0GebwtXOQJSsurUdlwt42Vh3abo3Wh9diUUV0SNTe?= =?us-ascii?Q?LPlITskTQpjR43hmEEykndO2AAxsB8zSxwlkk1IpGMsSrBh3ZvYu3KGPx6bV?= =?us-ascii?Q?RVKNmoSjhQlZ+gs7IFj5HRa52zBYUGMPbAJRiDB5N1MoSQxVEPkjEWamss+Y?= =?us-ascii?Q?mtIE/wS68L4Uhcy3Y3ODCwmkvUoHm6spFBbiYzPnJxj4NyL/5u/osM0USL5v?= =?us-ascii?Q?FbLcUjumhUG9IXdZcS5x4pvugaFp9AlWG5uJIhT5szN8R/XoPmEltUYvIEnv?= =?us-ascii?Q?FvRdeKKbtH/TyDV2n8M8yRHNKIk/OxtXmrsfNEIaHfiWjYOglb3CUO14hB9B?= =?us-ascii?Q?TmsBUWfuTkubDkIVeDJ2jnVFjndbxVACre/3B+Ju730kYspwXHOJ5E8kUomd?= =?us-ascii?Q?cmmc4I56UfniWJpTKQ+umlhIIPen2PjNanW+jjZrKy3R9pZLxUbezHw1L1+y?= =?us-ascii?Q?stnMh807eeyxMibOAfhij1HTuAPaHS7iNzmkz+NyVOyhgkFTve3im5NBCLgj?= =?us-ascii?Q?SRYOLY+PDf9kCbqlUy/rVRwSbUKDscUqqmMxhjUcTHvTApOS1oQtY39AFku+?= =?us-ascii?Q?koLmQvUUTpfuN5EC5VdsIWfqDD5o4conOtRV4tAw7fc6cGQoeSm62iseQFLC?= =?us-ascii?Q?uo+pI3K9QPy4Ii3SOQ23sLuSufmPLebu8HetGxROSub4oyNymuRUHlAg0MOK?= =?us-ascii?Q?UPMHcvXorxZQyIxCKd+SV/FW7bFUNX3W23/TnwpQJtD1z5VZmYHw6nkV8oyA?= =?us-ascii?Q?I4hRhYSoKOnHrabytLr7JNYfel8F94a6XIwrG1bT+VpsQnMLZB5ChSyXXbxc?= =?us-ascii?Q?9AsUXhX0y6ApdJoy7Ryq2WApxOVnmqe869qCNzeVR4dpfVHQ1ujDHkI03Hbg?= =?us-ascii?Q?W81aQGX3CJ1T0JAzgWj4VVX3DZakT33y2LuCbUTPVDPN42S08JXDHeA2dT11?= =?us-ascii?Q?4pgV6zHxBCaLKG2G5Bu8ZSuvaAy+skpAKJb+uRzwIRoeTv78DHqWVmSVG5/M?= =?us-ascii?Q?VPp0/0HrbOxb8D0wNgQEjW4PcO4i2rBDLns0t0XwtIT9LVsBudDe9HFZMtRk?= =?us-ascii?Q?wEnmC7ip1+rA8/BbTa2xLYXPUNjJBv/weZVDEJxrxD7mFSwz1K/1WESeiVE0?= =?us-ascii?Q?UHdyi7gPVOhh9x8SerzIpXxuoH5/EXwAwOyO6F9EtztNEVobT51SvCmGUWoZ?= =?us-ascii?Q?MNOgCEnOvSeY56C5eMo2piMeykuhhQVZQyPXmgvbgM2zs5ggy5NU+VlBa4ag?= =?us-ascii?Q?zgKrB3sNu5jPJ7yl+xWboGsUQLi3pO6EELDo1XGz98cLr5Rk6Wnxa5kbNkWr?= =?us-ascii?Q?wA=3D=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8244.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: c1ed8eec-1ca1-4cb8-32da-08db3d0c59ce X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Apr 2023 17:19:05.8252 (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: oqEL2By7YDy9wKoK+OkGs7Hg2HR84XmlwQ5E1SJE5ExokMNJdtheeyt04cLCln6f++cCANCxZKA/ZWsm29Te6A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7480 Return-Path: ray.ni@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB8244850E56285EBE95705B8B8C999MN6PR11MB8244namp_" --_000_MN6PR11MB8244850E56285EBE95705B8B8C999MN6PR11MB8244namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable tom, if the c bit is not required for non leaf page table entries, why the trunk= code sets the c bit for all entities including nonleaf ones? i went back to read again the smm issue you met. you said the c bit is set = for non leaf entries that caused a deference issue. But the pagetablelib co= de doesn't set c bit to non leaf entries. then who sets the c bit? thanks, ray thanks, ray ________________________________ From: devel@edk2.groups.io on behalf of Lendacky, Th= omas via groups.io Sent: Friday, April 14, 2023 9:43:52 PM To: Ni, Ray ; Tan, Dun ; devel@edk2.gr= oups.io Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and = update smm page table On 4/14/23 00:07, Ni, Ray wrote: > > >> -----Original Message----- >> From: Tom Lendacky >> Sent: Friday, April 14, 2023 12:19 AM >> To: Tan, Dun ; devel@edk2.groups.io >> Cc: Ni, Ray >> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create a= nd >> update smm page table >> >> On 4/13/23 04:14, Tan, Dun wrote: >>> Hi Tom, >>> >>> Thank you for your help with testing. >>> For the build failure, it's because that the CpuPageTableLib instance i= s >> added into OvmfPkg DSC in the last pacth ' OvmfPkg: Add CpuPageTableLib >> required by PiSmmCpuDxe'. I have moved this patch to the head of the pat= ch >> set. >>> >>> For the boot failure, I think it's because that the encrypt mask was no= t >> applied to the memory used by page table in page table non-leaf entry. >> Initially I thought the encrypt mask would only be applied to the leaf e= ntry in >> AMD SEV feature. So I treated the encryption process as non 1:1 mapping, >> which only applies the encrypt mask to leaf entry. I'm also curious why = the >> DxeIpl patch set works good. All the page table non-leaf entries are als= o not >> encrypted in the DxeIpl page table related patch set. >> >> Right, and that works for SEV. All non-leaf pagetable entries are treate= d >> as encrypted regardless of the encryption bit. Since the tables were bui= lt >> being mapped encrypted, the pagetable walk works when the non-leaf >> entries don't have the encryption bit set. In this case, though, the enc= ryption >> bit is present in the non-leaf entry and that is the reason why there ar= e >> issues. > > Can you point us which doc here (https://www.amd.com/en/developer/sev.htm= l) > says the page table is encrypted regardless the KEY_ID bits value? > How can the encryption engine know if a chunk of memory belongs to page t= able? It doesn't. For an SEV guest, when the hardware walks the pagetables, it will always treat the memory accesses as encrypted (see section 15.34.5 of the AMD APM Vol 2 at https://www.amd.com/system/files/TechDocs/24593.pdf). But, because the initial pagetables that are built to map everything as encrypted/private to start with (see OvmfPkg/ResetVector/Ia32/PageTables64.asm), only changing to shared when specifically requested, any memory allocated and used will be encrypted. Thus, when new pagetables are allocated/created in the CpuPageTableLib library, they will be encrypted and so everything works. And those new pagetables will map everything encrypted by default, except for the GHCB pages. If they were mapped shared when they were created, then the pagetable walk would fail. > > My understanding to SEV is any physical address field in guest page table= should have > the KEY_ID bits set if the physical pages are private to guest. Only some= pages for GMCB > don't have KEY_ID bits set as those are shared between guest and host. Right, the encryption bit in the leaf entry of the pagetables will determine the encryption mode. > > I thought Dun's patch works because all guest memory is marked as shared = because > the KEY_ID bits in all entries are not set. Only some pages that're used = by GMCB > have the KEY_ID bits set. Just the opposite, the CpuPageTableLib library marks everything encrypted and only clears the encryption bit for the GHCB pages. In MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c, the CreateIdentityMappingPageTables() function retrieves the encryption bit and saves it in AddressEncMask. AddressEncMask is then applied to the mapping attribute used when calling CreateOrUpdatePageTable() to build the initial pagetables. > > >> >> Here is some debug after setting PagingEntry at line 436 of >> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c: >> >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF81000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF80000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF83000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF81000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF80000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF83000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF81000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF80000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 800003FC01000 > > Are you testing the SME or SEV? > My understanding is with SME, only the highest C bit should be set indica= ting > the physical page is encrypted. I am testing SEV. There is only a single bit to indicate whether a page is encrypted. The guest ASID is used to determine what key is used to decrypt the page. From a pagetable leaf entry, SME and SEV are equivalent, the encryption bit determines how the memory will be accessed. SME and SEV differ in how they deal with instruction fetches and pagetable walks, with SME obeying the encryption bit and SEV always performing the accesses as encrypted accesses for security. Thanks, Tom > > > >> !!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - >> 00000000 !!!! >> >> 0x800003FC01000 isn't mapped and so it fails - I'm not exactly sure how >> the #PF turns into a #GP, though, maybe because the virtual address isn'= t >> canonical that point. >> >> Thanks, >> Tom >> >>> >>> I'll added another patch in my code branch to fix this issue later. In = the new >> commit, from the perspective of CpuPageTableLib, the whole memory can >> be divided into 3 categories: memory used by page table, guest private >> memory and guest shared memory. CpuPageTableLib will always apply the >> encrypt mask to memory used by page table, which means all the non-leaf >> page table entries are encrypted. For guest private memory, this case ca= n be >> treated as non-1:1 mapping. We can apply the encrypt mask by setting the >> input parameter of PageTableMap() API like " Attribute.Uint64 =3D >> LinearAddress | AddressEncMask". For guest shared memory, this case can >> be treated as normal 1:1 mapping. I'll let you know once the new patch i= s >> ready. >>> >>> Thanks, >>> Dun >>> -----Original Message----- >>> From: devel@edk2.groups.io On Behalf Of >> Lendacky, Thomas via groups.io >>> Sent: Thursday, April 13, 2023 3:26 AM >>> To: devel@edk2.groups.io; Tan, Dun >>> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create >> and update smm page table >>> >>> On 4/12/23 05:17, duntan via groups.io wrote: >>>> Hi Tom, >>>> >>>> This patch set is to change PiSmmCpuDxeSmm code to use >> CpuPageTableLib to create and update SMM page table. The Pcd >> PcdPteMemoryEncryptionAddressOrMask is also used in PiSmmCpuDxeSmm >> code and the whole range covered by page table is mapped encrypted, >> which is different from the situation in DxeIpl module. >>>> So could you also help do a test to make sure the AMD SEV feature stil= l >> works good in SMM with this patch set? >>>> Here is the code branch in my fork repo: >>>> https://github.com/td36/edk2/commits/SmmPageTable_V2 >>> >>> Hi Dun, >>> >>> I tested at the final commit of the branch and encountered a #GP with a= n >> SEV guest. It looks like the CpuPageTableLibrary doesn't take the encryp= tion >> bit into account. For example: >>> >>> Line 436 of UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c >>> PagingEntry =3D (IA32_PAGING_ENTRY >> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry- >>> Pnle); >>> >>> This will get an address with the encryption bit set and then try to >> reference it. When I clear the encryption bit, the code proceeds a bit f= urther, >> but then encounters a #GP in a different location. >>> >>> So it appears that the CpuPageTableLibrary doesn't deal with the >> encryption bit properly. >>> >>> Also, going through a build/test of each individual patch had mixed res= ults. >>> >>> - With the second patch in the series applied, I get a build error= : >>> >>> /root/kernels/ovmf-dun-build-X64/OvmfPkg/OvmfPkgX64.dsc(...): >> error 4000: Instance of library class [CpuPageTableLib] is not found >>> in [/root/kernels/ovmf-dun-build- >> X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf] [X64] >>> consumed by module [/root/kernels/ovmf-dun-build- >> X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf] >>> >>> that isn't resolved until the final patch. >>> >>> Thanks, >>> Tom >>> >>>> >>>> Thanks, >>>> Dun >>>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of >> duntan >>>> Sent: Wednesday, April 12, 2023 4:54 PM >>>> To: devel@edk2.groups.io >>>> Subject: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and >>>> update smm page table >>>> >>>> In V2 patch set: >>>> 1.In 'Refinement to code about updating smm page table', use QuickSort= () >> in BaseLib instead or PerformQuickSort() in BaseSortLib. >>>> 2.Remove the patch to add BaseSortLib in DSC file. >>>> 3.Add a new patch to add CpuPageTableLib in UefiCpuPkg.dsc. >>>> 4.Add a temp patch to add CpuPageTableLib in OvmfPkg dsc files for >>>> test(A previous patch I sent before '[Patch V2 4/8] OvmfPkg: Add >>>> CpuPageTableLib required by DxeIpl in DSC file' contains all the >>>> changes in this patch) >>>> >>>> Dun Tan (8): >>>> OvmfPkg: Add CpuPageTableLib required by PiSmmCpuDxe >>>> UefiPayloadPkg: Add CpuPageTableLib required by PiSmmCpuDxe >>>> UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute. >>>> UefiCpuPkg/PiSmmCpuDxeSmm: Avoid setting non-present range to >> RO/NX >>>> UefiCpuPkg: Extern mSmmShadowStackSize in PiSmmCpuDxeSmm.h >>>> UefiCpuPkg: Refinement to current smm page table generation code >>>> UefiCpuPkg: Refinement to code about updating smm page table >>>> UefiCpuPkg/PiSmmCpuDxeSmm: Remove unnecessary function >>>> >>>> OvmfPkg/CloudHv/CloudHvX64.dsc | 2 +- >>>> OvmfPkg/OvmfPkgIa32.dsc | 3 ++- >>>> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +- >>>> OvmfPkg/OvmfPkgX64.dsc | 2 +- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 5 +++-- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 3 +-- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 2 +- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 132 ---------= -------- >> ------------------------------------------------------------------------= ---------------------- >> --------------------- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 8 ++++++- >> - >>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 97 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++------------------------------------- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 + >>>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 629 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++++++++++++++++++++++++++++++++++++++++++-------------------------- >> ------------------------------------------------------------------------= ---------------------- >> ------------------------------------------------------------------------= ---------------------- >> ------------------------------------------------------------------------= ---------------------- >> ----------------------------------------------- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 348 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> +++++++++---------------------------------------------------------------= ---------------- >> ------------------------------------------------------------------------= ---------------------- >> -------------------------------------------------- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 229 >> ++++++++++++++++++++++++++++++------------------------------------------= ---- >> ------------------------------------------------------------------------= ---------------------- >> ----------------------------------------------------------- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 3 +-- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 19 ++------- >> ---------- >>>> UefiPayloadPkg/UefiPayloadPkg.dsc | 2 +- >>>> 17 files changed, 510 insertions(+), 977 deletions(-) >>>> >>> >>> >>> >>> >>> --_000_MN6PR11MB8244850E56285EBE95705B8B8C999MN6PR11MB8244namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
tom,
if the c bit is not required for non leaf page table entri= es, why the trunk code sets the c bit for all entities including nonleaf on= es?

i went back to read again the smm issue you met. you said = the c bit is set for non leaf entries that caused a deference issue. But th= e pagetablelib code doesn't set c bit to non leaf entries. then who sets th= e c bit?

thanks,
ray

thanks,
ray
From: devel@edk2.groups.io = <devel@edk2.groups.io> on behalf of Lendacky, Thomas via groups.io &l= t;thomas.lendacky=3Damd.com@groups.io>
Sent: Friday, April 14, 2023 9:43:52 PM
To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@intel.com= >; devel@edk2.groups.io <devel@edk2.groups.io>
Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to crea= te and update smm page table
 
On 4/14/23 00:07, Ni, Ray wrote:
>
>
>> -----Original Message-----
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>> Sent: Friday, April 14, 2023 12:19 AM
>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to cr= eate and
>> update smm page table
>>
>> On 4/13/23 04:14, Tan, Dun wrote:
>>> Hi Tom,
>>>
>>> Thank you for your help with testing.
>>> For the build failure, it's because that the CpuPageTableLib i= nstance is
>> added into OvmfPkg DSC in the last pacth ' OvmfPkg: Add CpuPageTab= leLib
>> required by PiSmmCpuDxe'. I have moved this patch to the head of t= he patch
>> set.
>>>
>>> For the boot failure, I think it's because that the encrypt ma= sk was not
>> applied to the memory used by page table in page table non-leaf en= try.
>> Initially I thought the encrypt mask would only be applied to the = leaf entry in
>> AMD SEV feature. So I treated the encryption process as non 1:1 ma= pping,
>> which only applies the encrypt mask to leaf entry. I'm also curiou= s why the
>> DxeIpl patch set works good. All the page table non-leaf entries a= re also not
>> encrypted in the DxeIpl page table related patch set.
>>
>> Right, and that works for SEV. All non-leaf pagetable entries are = treated
>> as encrypted regardless of the encryption bit. Since the tables we= re built
>> being mapped encrypted, the pagetable walk works when the non-leaf=
>> entries don't have the encryption bit set. In this case, though, t= he encryption
>> bit is present in the non-leaf entry and that is the reason why th= ere are
>> issues.
>
> Can you point us which doc here (https://www.amd.com/en/developer/sev.html)
> says the page table is encrypted regardless the KEY_ID bits value?
> How can the encryption engine know if a chunk of memory belongs to pag= e table?

It doesn't. For an SEV guest, when the hardware walks the pagetables, it will always treat the memory accesses as encrypted (see section 15.34.5 of =
the AMD APM Vol 2 at https://www.amd.com/system/files/TechDocs/24593.pdf).

But, because the initial pagetables that are built to map everything as encrypted/private to start with (see
OvmfPkg/ResetVector/Ia32/PageTables64.asm), only changing to shared when specifically requested, any memory allocated and used will be encrypted. Thus, when new pagetables are allocated/created in the CpuPageTableLib
library, they will be encrypted and so everything works. And those new
pagetables will map everything encrypted by default, except for the GHCB pages. If they were mapped shared when they were created, then the
pagetable walk would fail.

>
> My understanding to SEV is any physical address field in guest page ta= ble should have
> the KEY_ID bits set if the physical pages are private to guest. Only s= ome pages for GMCB
> don't have KEY_ID bits set as those are shared between guest and host.=

Right, the encryption bit in the leaf entry of the pagetables will
determine the encryption mode.

>
> I thought Dun's patch works because all guest memory is marked as shar= ed because
> the KEY_ID bits in all entries are not set. Only some pages that're us= ed by GMCB
> have the KEY_ID bits set.

Just the opposite, the CpuPageTableLib library marks everything encrypted <= br> and only clears the encryption bit for the GHCB pages.

In MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c, the
CreateIdentityMappingPageTables() function retrieves the encryption bit and saves it in AddressEncMask. AddressEncMask is then applied to the
mapping attribute used when calling CreateOrUpdatePageTable() to build the =
initial pagetables.

>
>
>>
>> Here is some debug after setting PagingEntry at line 436 of
>> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c:
>>
>> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF81000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF80000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF83000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF81000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF80000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF83000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF81000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 3FF80000 >> *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry =3D 800003FC01= 000
>
> Are you testing the SME or SEV?
> My understanding is with SME, only the highest C bit should be set ind= icating
> the physical page is encrypted.

I am testing SEV. There is only a single bit to indicate whether a page is =
encrypted. The guest ASID is used to determine what key is used to decrypt =
the page. From a pagetable leaf entry, SME and SEV are equivalent, the
encryption bit determines how the memory will be accessed.

SME and SEV differ in how they deal with instruction fetches and pagetable =
walks, with SME obeying the encryption bit and SEV always performing the accesses as encrypted accesses for security.

Thanks,
Tom

>
>
>
>> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU A= pic ID -
>> 00000000 !!!!
>>
>> 0x800003FC01000 isn't mapped and so it fails - I'm not exactly sur= e how
>> the #PF turns into a #GP, though, maybe because the virtual addres= s isn't
>> canonical that point.
>>
>> Thanks,
>> Tom
>>
>>>
>>> I'll added another patch in my code branch to fix this issue l= ater. In the new
>> commit, from the perspective of CpuPageTableLib, the whole memory = can
>> be divided into 3 categories: memory used by page table, guest pri= vate
>> memory and guest shared memory. CpuPageTableLib will always apply = the
>> encrypt mask to memory used by page table, which means all the non= -leaf
>> page table entries are encrypted. For guest private memory, this c= ase can be
>> treated as non-1:1 mapping. We can apply the encrypt mask by setti= ng the
>> input parameter of PageTableMap() API like " Attribute.Uint64= =3D
>> LinearAddress | AddressEncMask". For guest shared memory, thi= s case can
>> be treated as normal 1:1 mapping. I'll let you know once the new p= atch is
>> ready.
>>>
>>> Thanks,
>>> Dun
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Beh= alf Of
>> Lendacky, Thomas via groups.io
>>> Sent: Thursday, April 13, 2023 3:26 AM
>>> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com> >>> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib t= o create
>> and update smm page table
>>>
>>> On 4/12/23 05:17, duntan via groups.io wrote:
>>>> Hi Tom,
>>>>
>>>> This patch set is to change PiSmmCpuDxeSmm code to use
>> CpuPageTableLib to create and update SMM page table. The Pcd
>> PcdPteMemoryEncryptionAddressOrMask is also used in PiSmmCpuDxeSmm=
>> code and the whole range covered by page table is mapped encrypted= ,
>> which is different from the situation in DxeIpl module.
>>>> So could you also help do a test to make sure the AMD SEV = feature still
>> works good in SMM with this patch set?
>>>> Here is the code branch in my fork repo:
>>>> https://github.com/td36/edk2/commits/SmmPageTable_V2
>>>
>>> Hi Dun,
>>>
>>> I tested at the final commit of the branch and encountered a #= GP with an
>> SEV guest. It looks like the CpuPageTableLibrary doesn't take the = encryption
>> bit into account. For example:
>>>
>>> Line 436 of UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap= .c
>>>      PagingEntry =3D (IA32_PAGING_ENT= RY
>> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry= -
>>> Pnle);
>>>
>>> This will get an address with the encryption bit set and then = try to
>> reference it. When I clear the encryption bit, the code proceeds a= bit further,
>> but then encounters a #GP in a different location.
>>>
>>> So it appears that the CpuPageTableLibrary doesn't deal with t= he
>> encryption bit properly.
>>>
>>> Also, going through a build/test of each individual patch had = mixed results.
>>>
>>>      - With the second patch in the s= eries applied, I get a build error:
>>>
>>>        /root/kernels/ovmf-d= un-build-X64/OvmfPkg/OvmfPkgX64.dsc(...):
>> error 4000: Instance of library class [CpuPageTableLib] is not fou= nd
>>>          &nb= sp;     in [/root/kernels/ovmf-dun-build-
>> X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf] [X64]
>>>          &nb= sp;     consumed by module [/root/kernels/ovmf-dun-buil= d-
>> X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf]
>>>
>>>        that isn't resolved = until the final patch.
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>> Thanks,
>>>> Dun
>>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On= Behalf Of
>> duntan
>>>> Sent: Wednesday, April 12, 2023 4:54 PM
>>>> To: devel@edk2.groups.io
>>>> Subject: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib t= o create and
>>>> update smm page table
>>>>
>>>> In V2 patch set:
>>>> 1.In 'Refinement to code about updating smm page table', u= se QuickSort()
>> in BaseLib instead or PerformQuickSort() in BaseSortLib.
>>>> 2.Remove the patch to add BaseSortLib in DSC file.
>>>> 3.Add a new patch to add CpuPageTableLib in UefiCpuPkg.dsc= .
>>>> 4.Add a temp patch to add CpuPageTableLib in OvmfPkg dsc f= iles for
>>>> test(A previous patch I sent before '[Patch V2 4/8] OvmfPk= g: Add
>>>> CpuPageTableLib required by DxeIpl in DSC file' contains a= ll the
>>>> changes in this patch)
>>>>
>>>> Dun Tan (8):
>>>>      OvmfPkg: Add CpuPageTableLib= required by PiSmmCpuDxe
>>>>      UefiPayloadPkg: Add CpuPageT= ableLib required by PiSmmCpuDxe
>>>>      UefiCpuPkg: Use CpuPageTable= Lib to convert SMM paging attribute.
>>>>      UefiCpuPkg/PiSmmCpuDxeSmm: A= void setting non-present range to
>> RO/NX
>>>>      UefiCpuPkg: Extern mSmmShado= wStackSize in PiSmmCpuDxeSmm.h
>>>>      UefiCpuPkg: Refinement to cu= rrent smm page table generation code
>>>>      UefiCpuPkg: Refinement to co= de about updating smm page table
>>>>      UefiCpuPkg/PiSmmCpuDxeSmm: R= emove unnecessary function
>>>>
>>>>     OvmfPkg/CloudHv/CloudHvX64.dsc&nbs= p;            &= nbsp;       |   2 +-
>>>>     OvmfPkg/OvmfPkgIa32.dsc  = ;            &n= bsp;            = ; |   3 ++-
>>>>     OvmfPkg/OvmfPkgIa32X64.dsc &n= bsp;            = ;           |  = 2 +-
>>>>     OvmfPkg/OvmfPkgX64.dsc  =             &nb= sp;            =   |   2 +-
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Pag= eTbl.c           | &= nbsp; 5 +++--
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Smm= FuncsArch.c      |   3 +--
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Smm= ProfileArch.c    |   2 +-
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/MpServic= e.c            =   | 132 -----------------
>> ------------------------------------------------------------------= ----------------------------
>> ---------------------
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpu= DxeSmm.c         |   8 ++= ++++-
>> -
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpu= DxeSmm.h         |  97
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++-------------------------------------
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpu= DxeSmm.inf       |   1 +
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMe= moryManagement.c | 629
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++------------------------= --
>> ------------------------------------------------------------------= ----------------------------
>> ------------------------------------------------------------------= ----------------------------
>> ------------------------------------------------------------------= ----------------------------
>> -----------------------------------------------
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfi= le.c            = ; | 348
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> +++++++++---------------------------------------------------------= ----------------------
>> ------------------------------------------------------------------= ----------------------------
>> --------------------------------------------------
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/X64/Page= Tbl.c            | 2= 29
>> ++++++++++++++++++++++++++++++------------------------------------= ----------
>> ------------------------------------------------------------------= ----------------------------
>> -----------------------------------------------------------
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmF= uncsArch.c       |   3 +--
>>>>     UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmP= rofileArch.c     |  19 ++-------
>> ----------
>>>>     UefiPayloadPkg/UefiPayloadPkg.dsc&= nbsp;           &nbs= p;     |   2 +-
>>>>     17 files changed, 510 insertions(+= ), 977 deletions(-)
>>>>
>>>
>>>
>>>
>>>
>>>






--_000_MN6PR11MB8244850E56285EBE95705B8B8C999MN6PR11MB8244namp_--