From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id 174947803E5 for ; Thu, 23 May 2024 14:39:53 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=CSAmiJejNUIlAtYywndRYIPvdAbGADbhpPRXsClpF0E=; c=relaxed/simple; d=groups.io; h=From:To:Subject:Thread-Topic:Thread-Index:Date:Message-ID:References:In-Reply-To:Accept-Language:msip_labels:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type; s=20240206; t=1716475192; v=1; b=hsmEjD4zqXYmXnjsajd8kG90UdVXdVL7483GBvyNRVnnn1Oogp5/yFRPv7QG8q4/3/lpOP95 xTXwUMeWAItWn9ZULpx6HmqphOs2QE0KUIYMW+iarlTrO7AWhgAQB1yXTXXnXfXhh5eQvwhc2DM N6QrzUb/O1DXkQdqWMc4yRh15ljAcBqoaAUfJyTQyWLgA+NAN2xVnn6j7Zy4weROIkxo106FuM4 OEbtlyX6d0EW+i6AqcvqFDyqvdsVeW3xujp4uNS96zL3MmAmGlL634590BsTprfYmtw5ZJSC0bF T+9pXTVgZzQUpNN++lsm2NzmKfloq+gdHxy3clueGeRfw== X-Received: by 127.0.0.2 with SMTP id 6rwrYY7687511x6H6gg0ksmk; Thu, 23 May 2024 07:39:52 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.21]) by mx.groups.io with SMTP id smtpd.web11.215.1716475191514189066 for ; Thu, 23 May 2024 07:39:51 -0700 X-CSE-ConnectionGUID: HSFJExpeRYSfF6hIVR2ucA== X-CSE-MsgGUID: QvaMZfF+QmuPbW60fTq0vw== X-IronPort-AV: E=McAfee;i="6600,9927,11081"; a="12735867" X-IronPort-AV: E=Sophos;i="6.08,182,1712646000"; d="scan'208,217";a="12735867" X-Received: from orviesa002.jf.intel.com ([10.64.159.142]) by orvoesa113.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 May 2024 07:39:51 -0700 X-CSE-ConnectionGUID: ODo/yeDaSlaheWn5v9n4AQ== X-CSE-MsgGUID: iTh08VD7S6yP5yKtiGDlAg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,182,1712646000"; d="scan'208,217";a="64523831" X-Received: from fmsmsx603.amr.corp.intel.com ([10.18.126.83]) by orviesa002.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 23 May 2024 07:39:51 -0700 X-Received: from fmsmsx611.amr.corp.intel.com (10.18.126.91) by fmsmsx603.amr.corp.intel.com (10.18.126.83) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.39; Thu, 23 May 2024 07:39:50 -0700 X-Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) by fmsmsx611.amr.corp.intel.com (10.18.126.91) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.39; Thu, 23 May 2024 07:39:50 -0700 X-Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by fmsmsx612.amr.corp.intel.com (10.18.126.92) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.39 via Frontend Transport; Thu, 23 May 2024 07:39:50 -0700 X-Received: from NAM10-BN7-obe.outbound.protection.outlook.com (104.47.70.101) by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Thu, 23 May 2024 07:39:49 -0700 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by CO1PR11MB5105.namprd11.prod.outlook.com (2603:10b6:303:9f::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7611.20; Thu, 23 May 2024 14:39:47 +0000 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::41a4:c775:32e6:76a8]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::41a4:c775:32e6:76a8%4]) with mapi id 15.20.7587.035; Thu, 23 May 2024 14:39:47 +0000 From: "Ni, Ray" To: "Feng, Ning" , "devel@edk2.groups.io" Subject: Re: [edk2-devel] [PATCH] Pkg-Module:UefiCpuPkg/MpLib Thread-Topic: [PATCH] Pkg-Module:UefiCpuPkg/MpLib Thread-Index: AQHarPT4M0yqOR98Ukuoi3gjR1tM8rGk48UA Date: Thu, 23 May 2024 14:39:47 +0000 Message-ID: References: <20240523180235.3841139-1-ning.feng@intel.com> In-Reply-To: <20240523180235.3841139-1-ning.feng@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|CO1PR11MB5105:EE_ x-ms-office365-filtering-correlation-id: b6443fc9-978e-45a2-623d-08dc7b3631ca x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: =?us-ascii?Q?X12J6pW9OLlvlVOYiFIhQfjlNKxbRzf03MsRK3lHLY0ODzH1ZMZuoj89cDqM?= =?us-ascii?Q?IgOlwXs/XpVyzmyHf2rRfGAKpCpI6gwetu4SC9g8J92tIInsl30QfmB4HtuD?= =?us-ascii?Q?OQtsIG/YcSWCRa/FSiqZEzYQOZQVq4iULRjkATvIGQXPhwAuVvyMQG985ZCj?= =?us-ascii?Q?03oLQ+/HZVj6olRtjX5pHPX2DETaxhmnRDUm00HJlXILSAQojF9tof+o15MR?= =?us-ascii?Q?Pdx4OXv86b3Y8SRJR++y6AqgGkdcyNstoNzJVStFFdpHF9pLDUD1nf2/iH+X?= =?us-ascii?Q?IZNKb0zXVTyK0ZwlDr+WBEJkA/7z9DtkRDGdkNVVD3QN6LdzQKQp/tAJHcLe?= =?us-ascii?Q?jNGuniOOkqdNoOKS8C7KUFN1yXMA3Wn75GkBRAkVkJtveE1NzmUmhpcu8JJm?= =?us-ascii?Q?sIvnWEbAytkeRPzVJsbKNH7RFxAw2pCBQzUF50ycR+TfdC2Woxrg6D29Y7gA?= =?us-ascii?Q?dDOE5uaKb9olNm0X4J8KZTnQH2c5Omti333ETlQLP9b8tpUdgbTnQDqpqonY?= =?us-ascii?Q?czoRcJx+KQEQfA+RBtUJxW7xzOoYREyrwvIyqIJZnbCtr+9LF8s4SZvhEAri?= =?us-ascii?Q?4yo2pvXRpXW4Ch06nLmELkUJz4t4MmWgIHFhmeHgEQM2pGBeSH8iNCO6wxUx?= =?us-ascii?Q?P0lkw1Fx8Ytz67adjasXRDnR0mQAwFTXMviaLSRl5r85Rx6CVZAJXbRHKET/?= =?us-ascii?Q?tOahUMuNRuOaNIH4nwZf3k9ThyOfGwf41UBuA2CQ+PXzzcrbweYiOLETHPYE?= =?us-ascii?Q?2Tluf24i8xJRs1X4L6IhvxZauyyEh2S8NJLgd+LINAmJWEosHlO2yjuP5YHe?= =?us-ascii?Q?ISr+SBmcm9kH2gMecEeE6E46huiMXKDg8nwwP4nmdSrGHIb4byvda5nMKVs3?= =?us-ascii?Q?xrL+HcTIiSkmzlttKq4sx9nwKHAzcVPvYQZUrinkXMwglsS0xkiG6GweHThy?= =?us-ascii?Q?TYfjdo2fqYK91IAqm0zL1q20HhRWwNKzCFONozAyA49NhgE11XlfPALG5c4S?= =?us-ascii?Q?TTULItXADjlqjhFTddBGwNetgvlQTVhmPuzewp3UBndTgwqb/TE1gcDbq4Kt?= =?us-ascii?Q?xtxiXH0OIELEDF0TzEjRrN0hRMUcadM3RR9lqt0d4i0U2GeIn3tJeBY/c5TD?= =?us-ascii?Q?MyOIRYNvLSyfDNawTodDi9xB4/+YoF7k/tWxLnhv4A4EVJkfAnomHwOqijjl?= =?us-ascii?Q?LgDO3og4Kbv0mqYFBBKhkY2Tp4NtALh7PyLXyuvMdhot6uyAiI28hN78DIrZ?= =?us-ascii?Q?8xEpLxuJz3Yk71QZh76HHRd79BTTiUCGr2oH9lEUgg=3D=3D?= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?P58/NoiQOeXDYXS4Cq5UEOorsb4Aifr8lUZlzaQB0OpvP6rgD5SrBwEPhRAR?= =?us-ascii?Q?GdOPUQaUuUGpoYz9rLkMEQGCY1Ak2CJixD+GT2RwtdJWxOIRbWn7BH7s2eNc?= =?us-ascii?Q?ARC1cr/pswevvDOjHJlzrCtUHSnbYql1ymsm4IYBP9AlntiLLN4rTebDsspy?= =?us-ascii?Q?Q47KNqUURpCYizat1jVlTUNLD5njF2w8Dr8E4IO/IhlH1XiHBuBRXaPwoG95?= =?us-ascii?Q?YL4fokYZDb6ND8ntOCbWYZr7eGTR29LTSaFEfRaNv/NqPvUNPB9Bi1Zre+5y?= =?us-ascii?Q?hpKW4lnscXUOqRHkbqN3sh4JvufZSCsaBJ0mTdRYPgPteL6Epdt0LHzsifOt?= =?us-ascii?Q?fhh6PPdJwgdXwrGnkjwn3mbEamF5Z5F6r4CeCbBMJoRzp3nC0fpjiFxHonha?= =?us-ascii?Q?eTHunFvHoFUp5pVaAbbdNTtuBnSE8QZdtvKjo9dYIt8n1FeE+7srXyNhZXvl?= =?us-ascii?Q?L/PGqHyT7wWmvhgIYnUtFnVuUGsSGJaoAFc0g+mTqVUrYSUx5tmp5JgvSjFb?= =?us-ascii?Q?hQ2R2Ah/cvtA+FgKk8ErmrB1Z4wcG80B1bvw8+I0Q1/hDh/zf+e50D4D1xDU?= =?us-ascii?Q?hjwkmYRMnKoB/wrq7/eH6ZWwTg1Cb5BK8iFO56wMC1/Rchv1Q3E2IiakrMdN?= =?us-ascii?Q?EaIsNqZLf69EG4+qoNx9Zgg8kxqhhGPgbR9pUZf9+2f6ZqPhpPB0qF3Bv2dn?= =?us-ascii?Q?4wrnSwNBNux4uBtEwggyLJis0eR0pE0StX+skT5f0ac1Wb5ZBeUx3j9vdPm/?= =?us-ascii?Q?f1ogR14MjyzM9/qyftQjTMrTzMYxSWDfi0Qfj1sz2l3NcPx+D+1OKBuX8jxD?= =?us-ascii?Q?u29jGMns2/zHHW0E4wYEb2CVZQrloOaSkLflE0Mv26vnEa53FH6TljpPUCXA?= =?us-ascii?Q?ouIVr+tVNy3lzXRWmhR0HuV3WoGhAIy3kUutkWtQKqA1omzVC+kMygPES6cT?= =?us-ascii?Q?+hDPtCkzbHUswfrxrYB0n4aNFKBGxHKAz8AcaRfz9uOeDu1EP/0CaL6WFiuj?= =?us-ascii?Q?JbI81irkyvGvmYvyWL9rTUh+EnMoOzgVeoCDPTJJfNB+ubnJCvV3eiqxkEjB?= =?us-ascii?Q?0tOyXSUdm04g9YdLM9m7IVtX0SN5LcR9+xzhrD590tosn6BEMdt8AlnKd5xY?= =?us-ascii?Q?oJfub668h6rq5BcNg1CF7GwPJxXhGXiwVNO9k/BMrrYKkB7prILYCYuJa4W6?= =?us-ascii?Q?wIGm6aBMH4zGGOX1zbH1U42H0sdC45LEyqQC4xi4qpoOy5WONuvwhBhFKE9F?= =?us-ascii?Q?W8tzEEAVGii0on35+QF8M5IBiTuhAHbtSLX4xI+EEeWlsLIdZgHzftyDP58M?= =?us-ascii?Q?Rb2QzQ5S84IDltFlqStJ+bJs22CV5eSYZovNDbFX9GvEjicUctAocI0Qa7Ou?= =?us-ascii?Q?VPvDk+Y531vB4cLP+lTEUDf4rDzf78rNJqQJV4NHdD/ZOchuhxdFp2BJv4/4?= =?us-ascii?Q?UTkndNnLzg3Nt7b5W/Oust2dIIZ7Akwj3sSxDZAknY8kFEZrRxR7fmOKjcA1?= =?us-ascii?Q?iBwnzGY0P1DEPIgclXcoqIU6G5gmljyVCYMJJ2CqrlG0Qq7YZjcpqilx/xbJ?= =?us-ascii?Q?uMsN14SPVofIaSTlToY=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: b6443fc9-978e-45a2-623d-08dc7b3631ca X-MS-Exchange-CrossTenant-originalarrivaltime: 23 May 2024 14:39:47.3102 (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: bOFXMepHxklTIKQZOm/rjaJRGnThq79iTyTekuSwZYTbkG4cY9r/i7t5jz5wJ/lKFMi49rOR9KA4xF0zGgCGBQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB5105 X-OriginatorOrg: intel.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Thu, 23 May 2024 07:39:51 -0700 Resent-From: ray.ni@intel.com Reply-To: devel@edk2.groups.io,ray.ni@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: X2zNrGo2DsA7oUrlfPTGFfv4x7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB82440080CA97B3593C36D9958CF42MN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=hsmEjD4z; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=intel.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io --_000_MN6PR11MB82440080CA97B3593C36D9958CF42MN6PR11MB8244namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Ning, The patch looks good to me. But it seems you did not change the patch title to a more specific message. [Ray.1] The subject should be more specific. E.g.: UefiCpuPkg/MpInitLib: Do= not assume BSP is #0. Thanks, Ray ________________________________ From: Feng, Ning Sent: Friday, May 24, 2024 2:02 To: devel@edk2.groups.io Cc: Feng, Ning ; Ni, Ray Subject: [PATCH] Pkg-Module:UefiCpuPkg/MpLib REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D4778 MPInitlib have wrong expectation that BSP index should always be 0 in MpInitLibInitialize(), SwitchBsp(),ApWakeupFunction(). That will cause the data mismatch, if the initial BSP is not 0. Cc: Ray Ni Signed-off-by: Ning Feng --- UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 ++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpIn= itLib/MpLib.c index d724456502..ae279c6ceb 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -114,6 +114,10 @@ FutureBSPProc ( SaveVolatileRegisters (&DataInHob->APInfo.VolatileRegisters); AsmExchangeRole (&DataInHob->APInfo, &DataInHob->BSPInfo); RestoreVolatileRegisters (&DataInHob->APInfo.VolatileRegisters, FALSE); + // + // Restore VolatileReg saved in CpuMpData->CpuData + // + CopyMem (&DataInHob->CpuData[DataInHob->BspNumber].VolatileRegisters, &D= ataInHob->APInfo.VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS)); } /** @@ -761,11 +765,11 @@ ApWakeupFunction ( BistData =3D (UINT32)ApStackData->Bist; // - // CpuMpData->CpuData[0].VolatileRegisters is initialized based on B= SP environment, + // CpuMpData->CpuData[BspNumber].VolatileRegisters is initialized ba= sed on BSP environment, // to initialize AP in InitConfig path. - // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters= points to a different IDT shared by all APs. + // NOTE: IDTR.BASE stored in CpuMpData->CpuData[BspNumber].VolatileR= egisters points to a different IDT shared by all APs. // - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, = FALSE); + RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].= VolatileRegisters, FALSE); InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack= ); ApStartupSignalBuffer =3D CpuMpData->CpuData[ProcessorNumber].Startu= pApSignal; } else { @@ -798,10 +802,10 @@ ApWakeupFunction ( // 1. AP is re-enabled after it's disabled, in either PEI or DXE p= hase. // 2. AP is initialized in DXE phase. // In either case, use the volatile registers value derived from B= SP. - // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegiste= rs points to a + // NOTE: IDTR.BASE stored in CpuMpData->CpuData[BspNumber].Volatil= eRegisters points to a // different IDT shared by all APs. // - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters= , FALSE); + RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber= ].VolatileRegisters, FALSE); } else { if (CpuMpData->ApLoopMode =3D=3D ApInHltLoop) { // @@ -927,7 +931,7 @@ DxeApEntryPoint ( AsmWriteMsr64 (MSR_IA32_EFER, EferMsr.Uint64); } - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALS= E); + RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].Vola= tileRegisters, FALSE); InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); PlaceAPInMwaitLoopOrRunLoop ( CpuMpData->ApLoopMode, @@ -2151,7 +2155,12 @@ MpInitLibInitialize ( CpuMpData->BackupBufferSize =3D ApResetVectorSizeBelow1Mb; CpuMpData->WakeupBuffer =3D (UINTN)-1; CpuMpData->CpuCount =3D 1; - CpuMpData->BspNumber =3D 0; + if (MpHandOff =3D=3D NULL) { + CpuMpData->BspNumber =3D 0; + } else { + CpuMpData->BspNumber =3D GetBspNumber (MpHandOff); + } + CpuMpData->WaitEvent =3D NULL; CpuMpData->SwitchBspFlag =3D FALSE; CpuMpData->CpuData =3D (CPU_AP_DATA *)(CpuMpData + 1); @@ -2186,11 +2195,11 @@ MpInitLibInitialize ( // Don't pass BSP's TR to APs to avoid AP init failure. // VolatileRegisters.Tr =3D 0; - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, s= izeof (VolatileRegisters)); + CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &V= olatileRegisters, sizeof (VolatileRegisters)); // // Set BSP basic information // - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize); + InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, CpuMpData->Buffer = + ApStackSize * (CpuMpData->BspNumber + 1)); // // Save assembly code information // @@ -2615,7 +2624,12 @@ SwitchBSPWorker ( SaveVolatileRegisters (&CpuMpData->BSPInfo.VolatileRegisters); AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData->APInfo); RestoreVolatileRegisters (&CpuMpData->BSPInfo.VolatileRegisters, FALSE); - + // + // Restore VolatileRegs saved in CpuMpData->CpuData + // Don't pass BSP's TR to APs to avoid AP init failure. + // + CopyMem (&CpuMpData->CpuData[CpuMpData->NewBspNumber].VolatileRegisters,= &CpuMpData->BSPInfo.VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS)); + CpuMpData->CpuData[CpuMpData->NewBspNumber].VolatileRegisters.Tr =3D 0; // // Set the BSP bit of MSR_IA32_APIC_BASE on new BSP // -- 2.25.1 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#119160): https://edk2.groups.io/g/devel/message/119160 Mute This Topic: https://groups.io/mt/106256300/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --_000_MN6PR11MB82440080CA97B3593C36D9958CF42MN6PR11MB8244namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
Ning,
The patch looks good to me.
But it seems you did not change the patch title to a more specific message.=

[Ray.1] The subject should be more specific. E.g.: UefiCpuPkg/MpInitLib: Do= not assume BSP is #0.




Thanks,
Ray
From: Feng, Ning <ning.f= eng@intel.com>
Sent: Friday, May 24, 2024 2:02
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Feng, Ning <ning.feng@intel.com>; Ni, Ray <ray.ni@intel= .com>
Subject: [PATCH] Pkg-Module:UefiCpuPkg/MpLib
 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id= =3D4778
MPInitlib have wrong expectation that BSP index should always be 0 in
MpInitLibInitialize(), SwitchBsp(),ApWakeupFunction().
That will cause the data mismatch, if the initial BSP is not 0.
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Ning Feng <ning.feng@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 ++++++++++++++++++++-------= -
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpIn= itLib/MpLib.c
index d724456502..ae279c6ceb 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -114,6 +114,10 @@ FutureBSPProc (
   SaveVolatileRegisters (&DataInHob->APInfo.VolatileRegis= ters);

   AsmExchangeRole (&DataInHob->APInfo, &DataInHob->= ;BSPInfo);

   RestoreVolatileRegisters (&DataInHob->APInfo.VolatileRe= gisters, FALSE);

+  //

+  // Restore VolatileReg saved in CpuMpData->CpuData

+  //

+  CopyMem (&DataInHob->CpuData[DataInHob->BspNumber].Volati= leRegisters, &DataInHob->APInfo.VolatileRegisters, sizeof (CPU_VOLAT= ILE_REGISTERS));

 }

 

 /**

@@ -761,11 +765,11 @@ ApWakeupFunction (
       BistData     =3D (= UINT32)ApStackData->Bist;

 

       //

-      // CpuMpData->CpuData[0].VolatileRegiste= rs is initialized based on BSP environment,

+      // CpuMpData->CpuData[BspNumber].Volatil= eRegisters is initialized based on BSP environment,

       //   to initialize AP in Ini= tConfig path.

-      // NOTE: IDTR.BASE stored in CpuMpData->= CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
+      // NOTE: IDTR.BASE stored in CpuMpData->= CpuData[BspNumber].VolatileRegisters points to a different IDT shared by al= l APs.

       //

-      RestoreVolatileRegisters (&CpuMpData-&g= t;CpuData[0].VolatileRegisters, FALSE);

+      RestoreVolatileRegisters (&CpuMpData-&g= t;CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);

       InitializeApData (CpuMpData, Processor= Number, BistData, ApTopOfStack);

       ApStartupSignalBuffer =3D CpuMpData-&g= t;CpuData[ProcessorNumber].StartupApSignal;

     } else {

@@ -798,10 +802,10 @@ ApWakeupFunction (
         // 1. AP is re-enabled aft= er it's disabled, in either PEI or DXE phase.

         // 2. AP is initialized in= DXE phase.

         // In either case, use the= volatile registers value derived from BSP.

-        // NOTE: IDTR.BASE stored in Cp= uMpData->CpuData[0].VolatileRegisters points to a

+        // NOTE: IDTR.BASE stored in Cp= uMpData->CpuData[BspNumber].VolatileRegisters points to a

         //   different I= DT shared by all APs.

         //

-        RestoreVolatileRegisters (&= CpuMpData->CpuData[0].VolatileRegisters, FALSE);

+        RestoreVolatileRegisters (&= CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
       } else {

         if (CpuMpData->ApLoopMo= de =3D=3D ApInHltLoop) {

           //

@@ -927,7 +931,7 @@ DxeApEntryPoint (
     AsmWriteMsr64 (MSR_IA32_EFER, EferMsr.Uint64);

   }

 

-  RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileReg= isters, FALSE);

+  RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->= BspNumber].VolatileRegisters, FALSE);

   InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCou= nt);

   PlaceAPInMwaitLoopOrRunLoop (

     CpuMpData->ApLoopMode,

@@ -2151,7 +2155,12 @@ MpInitLibInitialize (
   CpuMpData->BackupBufferSize =3D ApResetVectorSizeBelow1Mb;<= br>
   CpuMpData->WakeupBuffer     =3D (UINTN)= -1;

   CpuMpData->CpuCount      &nbs= p;  =3D 1;

-  CpuMpData->BspNumber        = =3D 0;

+  if (MpHandOff =3D=3D NULL) {

+    CpuMpData->BspNumber =3D 0;

+  } else {

+    CpuMpData->BspNumber =3D GetBspNumber (MpHandOff);
+  }

+

   CpuMpData->WaitEvent      &nb= sp; =3D NULL;

   CpuMpData->SwitchBspFlag    =3D FALSE;

   CpuMpData->CpuData       = ;   =3D (CPU_AP_DATA *)(CpuMpData + 1);

@@ -2186,11 +2195,11 @@ MpInitLibInitialize (
   // Don't pass BSP's TR to APs to avoid AP init failure.

   //

   VolatileRegisters.Tr =3D 0;

-  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &Vola= tileRegisters, sizeof (VolatileRegisters));

+  CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].Volati= leRegisters, &VolatileRegisters, sizeof (VolatileRegisters));

   //

   // Set BSP basic information

   //

-  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSi= ze);

+  InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, CpuMpData-= >Buffer + ApStackSize * (CpuMpData->BspNumber + 1));

   //

   // Save assembly code information

   //

@@ -2615,7 +2624,12 @@ SwitchBSPWorker (
   SaveVolatileRegisters (&CpuMpData->BSPInfo.VolatileRegi= sters);

   AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData-&g= t;APInfo);

   RestoreVolatileRegisters (&CpuMpData->BSPInfo.VolatileR= egisters, FALSE);

-

+  //

+  // Restore VolatileRegs saved in CpuMpData->CpuData

+  // Don't pass BSP's TR to APs to avoid AP init failure.

+  //

+  CopyMem (&CpuMpData->CpuData[CpuMpData->NewBspNumber].Vol= atileRegisters, &CpuMpData->BSPInfo.VolatileRegisters, sizeof (CPU_V= OLATILE_REGISTERS));

+  CpuMpData->CpuData[CpuMpData->NewBspNumber].VolatileRegisters= .Tr =3D 0;

   //

   // Set the BSP bit of MSR_IA32_APIC_BASE on new BSP

   //

--
2.25.1

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#119160) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--_000_MN6PR11MB82440080CA97B3593C36D9958CF42MN6PR11MB8244namp_--