From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web10.8709.1681201853902992460 for ; Tue, 11 Apr 2023 01:30:54 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=iEvY6AoJ; spf=pass (domain: intel.com, ip: 134.134.136.20, 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=1681201853; x=1712737853; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=qTad1+UmOsFqHX7k6KfI2BMCeACReiVu/bUDTwJ/6Mw=; b=iEvY6AoJaQGRTBtajUpeZl+BHUIFwxBmZltlmllPuQ9RnJB6gTs3JD7/ JzXdYAcZxahEqglfO7ELOKTrzcNQsI3iDbNotA3WXrQryn6q+toUQmQRP DA5j3sOPG+7hUzPvhU5XKvhuWUOaK56k+EBx9XXhJnRxTqqTb973mU2jL JN7mtErInQZB/JGd1vjKP+eVuNpnfCJz8UfS0SeqZtfePe5Jwp8yghd4G pwa7Q6UXTQo9852S1XdUEl3yfccfnsZ6Aqc65/JU4vD3AITSrKfpHmVIL n+HS006g5GYutunrm45QG3Gs5VAhgkDj28BrTcV74+vmeG/SvflCcX/WG g==; X-IronPort-AV: E=McAfee;i="6600,9927,10676"; a="332239585" X-IronPort-AV: E=Sophos;i="5.98,336,1673942400"; d="scan'208";a="332239585" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Apr 2023 01:30:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10676"; a="832264574" X-IronPort-AV: E=Sophos;i="5.98,336,1673942400"; d="scan'208";a="832264574" Received: from orsmsx601.amr.corp.intel.com ([10.22.229.14]) by fmsmga001.fm.intel.com with ESMTP; 11 Apr 2023 01:30:52 -0700 Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) 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.2507.23; Tue, 11 Apr 2023 01:30:52 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx610.amr.corp.intel.com (10.22.229.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23 via Frontend Transport; Tue, 11 Apr 2023 01:30:52 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.172) 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.2507.23; Tue, 11 Apr 2023 01:30:52 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PW1I5elocHucL9o95QcGd/L3uCJ7snq+LAFXcnYgk/QZ8EnZQH2ZESAO+Nbg+waE/jsu73NaktHcm5r8Q4ZntYOw7wEm/QRiQcW5LWcRZfjSH80L8k3/DuR1OwBSzsRfHnZ2+LyB61xW+fOsgbJeu+bP0ioXF9XSQRQZWS41Ca2Hml0BYEuBfr4dxo5gykMrY3cxL2pMKt9modKP0NEFlX1z2G80QOY4JNAK21Quh0fIEnEsvdTRmqCpna8UjuK8mlYyBv3Hrmhycr8xItqLZCDCexdMoPuj5V9PGwBz3bDYk/3BBwnkiZUnOEdHt7vqEgPsb0Q5VOqJE5XzNOR1vQ== 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=+0tO8uj1MqFjiGb4eLiGx+3wQ3KHQ3Xos3Tvbe59QDw=; b=CbZ0QTYwN8zw7ke7pbK6IBDl29chXVYT5wXvMKDjPEJ9mDCTqhjcWbOAUb5RFJfExSFkb8WZBpxT/Pp3VnfWqZcYkENzGcI8LBz+rvYgnZCRlVoPFTTySVhQ4/xTOIVvLGqIWUNoNxYdCkIqkr8UfVfYYJGQHTyqxOe++I8wHuja1s0kHeizwmrC5Ozvku1zHLOfOKLwRFuTIHxm4pdkW31vbZziThxBVKKoahDLO1lG+qgBLe+O27htiMft13nX0FbDAgNodMbpCYqG47vom9W3M89q7JP85JUqodlPisF1Ap12oBSLP5l0i3A3eYnmGYocSHmFBM6hha6lHJA2fw== 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 DS7PR11MB7783.namprd11.prod.outlook.com (2603:10b6:8:e1::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6277.35; Tue, 11 Apr 2023 08:30:50 +0000 Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::ae07:e96a:4a24:8a69]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::ae07:e96a:4a24:8a69%7]) with mapi id 15.20.6277.034; Tue, 11 Apr 2023 08:30:50 +0000 From: "Ni, Ray" To: "devel@edk2.groups.io" , "abdattar@amd.com" CC: Abdul Lateef Attar , Paul Grimes , Garrett Kirkendall , Abner Chang , "Dong, Eric" , "Kumar, Rahul R" , Gerd Hoffmann Subject: Re: [edk2-devel] [PATCH v8 3/9] UefiCpuPkg: Implements SmmSmramSaveStateLib library class Thread-Topic: [edk2-devel] [PATCH v8 3/9] UefiCpuPkg: Implements SmmSmramSaveStateLib library class Thread-Index: AQHZa50s3ve8U2plt0+B7tOXzPuXI68lvZpQ Date: Tue, 11 Apr 2023 08:30:50 +0000 Message-ID: References: <65d62d0f62fd888c0b1054535044870d8a15c175.1681121324.git.abdattar@amd.com> In-Reply-To: <65d62d0f62fd888c0b1054535044870d8a15c175.1681121324.git.abdattar@amd.com> 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_|DS7PR11MB7783:EE_ x-ms-office365-filtering-correlation-id: 31045a45-480f-4427-0d47-08db3a670e9d x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: YDt8v30X8cd6VxqdAIFrxhloG3hywAjwznr5WJl7i73xCkeNP496moPrff4DsmKrgnYlPqjd8Yv5espcfI5nY0ogethQdAKtjbgkmSzH9dOqLAtVQd9x+cS7/4LbuZEJmCa7IrQ4+/H1VFxKz4cRqvw2nns2OQqNNugZ6RK2NVbobKuOi0FOV9FDPlDcPefIfW71m8XBv6RArrFNnQf3ySYrEomfNAqYbEGb2tuUfI9z8A5+hcxmatRYkeyBPqIa6tWUGdIEh3IMxPcIEsf2nXcbRDFM8DB2qeVFGIF0uiV2Gq3wKvk17ydxFty1+Qr0CCqk/Oevmca+7kVfaVlhVPhThmGv7zE/3KBaBVgv45W34HaaMKu/qf9LrhdPyJyutyRYShMY3VDzlHJsVKbcB3CnoHu/vvNihYL2IA20fP0PchSZWc7sKWbj1TKaNELG34R2PzG2B/Tyq//OOBrF8FnC5HEp72aefk5HGGq0SXJfkXZBoZkyDPZ0BD5WY201MOVEPbJK4p7vexDkwISviFylo2rY2eThhZpEX0M2ObnUMeB7iL9HjULOI30ALuyRX9THHR6VQ/8YTxbMAv3ouSNHVs/Rw1OVkoQ6zBNy0IN3J9HVXhrCO2Hq5fGnG/ooDXJO6A2TuD07njlUQP9uGQ== 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)(346002)(376002)(396003)(366004)(136003)(451199021)(7696005)(71200400001)(478600001)(55016003)(86362001)(33656002)(38070700005)(83380400001)(82960400001)(38100700002)(122000001)(2906002)(9686003)(316002)(110136005)(6506007)(54906003)(26005)(186003)(66476007)(52536014)(8676002)(66556008)(8936002)(5660300002)(41300700001)(66446008)(64756008)(66946007)(4326008)(76116006)(213903007);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ljK77PwRPTxRV1YyVza2h0BUzbNgd1D3KOWgV94ACD3I1PrvxiZo477ArnyA?= =?us-ascii?Q?Gl21QkNoUJND8grls9oDKvljcY6B/8MtoeUJ5h6LGdsfXr15DcsiHgGtqm6t?= =?us-ascii?Q?jGxQaW7ERmitB9i5fsOaNWAoCD0fUOklwS6FxE8q3yh4TufLBMSBqe0NnDRS?= =?us-ascii?Q?FE1A9G6o1uo/Nsb1C953ePhRKhiwofUbT9gTXp7ViRhHf7XYD+PpuxRybf5N?= =?us-ascii?Q?hC0bjaW6ttU/x7ey2xWKQjpOa2zPaxqY+/hZNUGoV//irsBpE8NqC/Ta9fXt?= =?us-ascii?Q?l2vqWZsqrZg6byNZ6XmSMWSSPaeVNL8y1OrDuF4k6yOV4PrVR4hReCD5ZtI4?= =?us-ascii?Q?FhIyfmpor2KSCi/p7fulyMxMBGq2mFf2FYak55jHlC/CgD/5mpUDeLiKM85H?= =?us-ascii?Q?2vTlPoAUdCx5aGUMtUYestGJtXqXqV3kM3iyKL3/l7H7PRrDoTaBUwfuEH1r?= =?us-ascii?Q?0WzvCW2l6ryw1KYCII6G6D/GhUtgEReRphZWJ7XxiYZQAekrQjjPEYDywvpA?= =?us-ascii?Q?6HkOv73q/B6GUfCT7KhwDOkfL0inB7qqqY2A0VQd87448j7Cgli1IhWltqJd?= =?us-ascii?Q?x6xdtu6phRgdDDdNt48IOXA+Gmpp/fkOgxBkSlSs/T6pJB6kPL8S0JPsxuIG?= =?us-ascii?Q?J+RBRY8YrPinAYX1Rs8OKsFRbjGxKhMJ7UTv7pSM4AeG6SK4UlPJHxR+U4vu?= =?us-ascii?Q?O+6anhDBiMU0TCnTSiJ9Z3gc0N3LAnNn7lfuk7m0eqYpzbvPlmR8u//ANz47?= =?us-ascii?Q?OrLp03x/xqKQfz073qYyVqQdjQjkOkENhbRnugnQuU6n5elnyJEhuwRhaXwN?= =?us-ascii?Q?p95NubZgqZVZibbD3Vo5JHfTo7Wc6FxYVhlMlYZVNFjE1cY08A0woUXAPcV/?= =?us-ascii?Q?Wx93jddRjkrXalhap9WGTvqLM8hWzLE6D5PS4dhx2smj9Y8gnAnAzxOWUsG9?= =?us-ascii?Q?Qb/IOobznGknU++3OvfwWIz7E6UjWzZwKDMYlTXgwJYoNXF4D++5C7gVYXL9?= =?us-ascii?Q?X4noLlldBasgVbLzWb6lH4OgJRSE/cw2CiGpWMyhARdqtik2hUx9CLuks2Oq?= =?us-ascii?Q?SAt4+hEa+dDrp/FkVolKWU0isEBQcUQ2i32/TfvT1Tks9+lqGrU0N3N2Ufch?= =?us-ascii?Q?/Xrz1yqJNopDYY0yQKxuGEb3W0E3C9W1MzKzezV7utME+EnP8vhMaS2VWFqW?= =?us-ascii?Q?Hk6Y1ekLAAP9QYWeGE6WZ74/vemTYikxC5UgQ+as9JCKKT8ND23GDXnue6gK?= =?us-ascii?Q?GpaKqGa1lxNZOuHoXITdTbBI3r6wn6CDYL7XUUkVAkRznDQyCX3EtwWosZyH?= =?us-ascii?Q?IJTYhusfIAyBr/wejlHYCCrMM0dxezA57DQztEw/bQS4ODGEsBgkTHPHJJqb?= =?us-ascii?Q?4AW+aTIViNMfkcn+feDpc7uSUv210wB71J+egU+npUs64bfhus8AoT6VhFDm?= =?us-ascii?Q?YzzZccvHUT8nafURElz+FJfxSs9DOg7QxC3lzIWGehwabnvLWP06Bu5wAIKd?= =?us-ascii?Q?mtD2GSChn4uGDhVTq21lae42bOr3Tigap7H7gVBk0NJTQkiXol4KmNy2clS4?= =?us-ascii?Q?CaI80D8vvHK7YAEqFZM=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: 31045a45-480f-4427-0d47-08db3a670e9d X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Apr 2023 08:30:50.3697 (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: F4n6YO8ULP6DDVHQDQTB6liVu+yDzQUpieVYyDVfMVsPdksdenIumuKf+apvxRQUOj2BcT4JuYzmhTqvsP4eBQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7783 Return-Path: ray.ni@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > + > SmmSmramSaveStateLib|UefiCpuPkg/Library/SmmSmramSaveStateLib/Am > dSmmSmramSaveStateLib.inf 1. The lib instance name can be AmdMmSaveStateLib inside X86MmSaveStateLib = folder. > +[Defines] > + INF_VERSION =3D 1.29 > + BASE_NAME =3D AmdSmmSmramSaveStateLib > + FILE_GUID =3D FB7D0A60-E8D4-4EFA-90AA-B357BC56987= 9 > + MODULE_TYPE =3D DXE_SMM_DRIVER 2. The module type can be BASE. > + VERSION_STRING =3D 1.0 > + LIBRARY_CLASS =3D SmmSmramSaveStateLib 3. The LIBRARY_CLASS can be "MmSaveStateLib|DXE_SMM_DRIVER MM_STANDALONE" i= ndicating it supports the two types of modules. > +typedef struct { > + UINT8 Width32; > + UINT8 Width64; > + UINT16 Offset32; > + UINT16 Offset64Lo; > + UINT16 Offset64Hi; 4. With above structure definition "Offset64Lo/Offset64Hi", I realized that= you define AMD_SMRAM_SAVE_STATE_MAP64 in a way to split the lower 32bit and high 32bit to two fields. I think it's only n= eeded when the lower 32bit and higher 32bit are not adjacent. Otherwise, you can just define "UINT64 _IDTRBase" in AMD_SMRAM_SAVE_STATE_M= AP64. (Unfortunately, GdtBase/LdtBase/IdtBase are split to two 32bit fields not a= djacent to each other in Intel 64bit save state. But AMD doesn't need to.) So the recommendation here is to do some simplification to the AMD 64bit sa= ve state definition. 5. Another thought here: Can we remove "AMD_" prefix from the structure nam= e "AMD_SMRAM_SAVE_STATE_MAP64"? Because when we define CPUIDs or MSRs, we don't put "INTEL_" or "AMD_" pref= ix either. The definition file is in "MdePkg/Include/Register/Amd" folder which alread= y indicates it's AMD specific definition. > +**/ > +UINTN > +EFIAPI 6. No need for "EFIAPI". > +SmramSaveStateGetRegisterIndex ( 7. Since it's not an API, can it be renamed as "MmSaveStateLibGetRegisterIn= dex"? > + IN EFI_SMM_SAVE_STATE_REGISTER Register > + ); > + > +EFI_STATUS > +EFIAPI > +SmramSaveStateReadRegisterByIndex ( > + IN UINTN CpuIndex, > + IN UINTN RegisterIndex, > + IN UINTN Width, > + OUT VOID *Buffer > + ) > +{ > + if (RegisterIndex =3D=3D 0) { > + return EFI_NOT_FOUND; > + } > + > + if (SmramSaveStateGetRegisterLma () =3D=3D > EFI_SMM_SAVE_STATE_REGISTER_LMA_32BIT) { > + // > + // If 32-bit mode width is zero, then the specified register can not= be > accessed > + // > + if (mSmmSmramCpuWidthOffset[RegisterIndex].Width32 =3D=3D 0) { > + return EFI_NOT_FOUND; > + } > + > + // > + // If Width is bigger than the 32-bit mode width, then the specified > register can not be accessed > + // > + if (Width > mSmmSmramCpuWidthOffset[RegisterIndex].Width32) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // Write return buffer > + // > + ASSERT (gSmst->CpuSaveState[CpuIndex] !=3D NULL); > + CopyMem (Buffer, (UINT8 *)gSmst->CpuSaveState[CpuIndex] + > mSmmSmramCpuWidthOffset[RegisterIndex].Offset32, Width); > + } else { > + // > + // If 64-bit mode width is zero, then the specified register can not= be > accessed > + // > + if (mSmmSmramCpuWidthOffset[RegisterIndex].Width64 =3D=3D 0) { > + return EFI_NOT_FOUND; > + } > + > + // > + // If Width is bigger than the 64-bit mode width, then the specified > register can not be accessed > + // > + if (Width > mSmmSmramCpuWidthOffset[RegisterIndex].Width64) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // Write lower 32-bits of return buffer > + // > + CopyMem (Buffer, (UINT8 *)gSmst->CpuSaveState[CpuIndex] + > mSmmSmramCpuWidthOffset[RegisterIndex].Offset64Lo, MIN (4, Width)); > + if (Width >=3D 4) { > + // > + // Write upper 32-bits of return buffer > + // > + CopyMem ((UINT8 *)Buffer + 4, (UINT8 *)gSmst- > >CpuSaveState[CpuIndex] + > mSmmSmramCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4); > + } > + } > + > + return EFI_SUCCESS; 8. I feel the above logic that reads from 32bit/64bit SMRAM save state can = be written in a simpler way. But since the logic aligns with the existing one in CpuSmm driver, I am ok = that we do the simplification in other time.