From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=helo; client-ip=2a01:111:f400:fe4d::602; helo=nam04-co1-obe.outbound.protection.outlook.com; envelope-from=leo.duran@amd.com; receiver=edk2-devel@lists.01.org Received: from NAM04-CO1-obe.outbound.protection.outlook.com (mail-co1nam04on0602.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe4d::602]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1574621157428 for ; Tue, 25 Sep 2018 07:29:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amdcloud.onmicrosoft.com; s=selector1-amd-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=KPcSXS7rEUwCto8yn93u1ENEpa63lJqWcCV1pCL/FnU=; b=iu/InL0i60o+fWbex7nIMNxKGMOvYQ4bgWhG8lKfpY9RuDWmYjEb31UCBEkNFvXBSlqPkvH5NCcoAg6Hirru33Fb4XNFLeQ26SgQJAQddKsZEAtPyvP2xYUW6jpy1R955wCWqh7c8E5ZNRSwgBXsFOJs3KLSYWYNQnxuWWM/lrc= Received: from CY4PR12MB1815.namprd12.prod.outlook.com (10.175.63.21) by CY4PR12MB1141.namprd12.prod.outlook.com (10.168.163.149) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1143.18; Tue, 25 Sep 2018 14:29:21 +0000 Received: from CY4PR12MB1815.namprd12.prod.outlook.com ([fe80::ecd7:135f:4e1b:2d82]) by CY4PR12MB1815.namprd12.prod.outlook.com ([fe80::ecd7:135f:4e1b:2d82%3]) with mapi id 15.20.1143.019; Tue, 25 Sep 2018 14:29:20 +0000 From: "Duran, Leo" To: "Ni, Ruiyu" , Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Dong, Eric" Thread-Topic: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change. Thread-Index: AQHUSeX3EbC17fDvXkWWIRAmwJTcUaTrbK6AgAANLJCAAO+ogIAAVCzAgAAzL4CAAAHbwIAAj1YAgAEZC8CAAJxFAIAFdsQggAAHwQCAAQskAIAAaJZwgAEwkgCAA6cmAIAABPgAgAYdhBA= Date: Tue, 25 Sep 2018 14:29:20 +0000 Message-ID: References: <1536680498-6554-1-git-send-email-leo.duran@amd.com> <1536680498-6554-2-git-send-email-leo.duran@amd.com> <17c6d6d1-2655-fe06-a8b9-f48141bfb0d7@redhat.com> <610eaa55-c87b-5e0c-4f87-5c1e79ffc5ba@redhat.com> <12abd990-3b08-9159-e7a9-ffd7eb7282b3@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BE07168@SHSMSX104.ccr.corp.intel.com> <981751ac-68a0-ea2c-7985-2562d1916560@Intel.com> <85b907c0-1d7d-98f1-6e86-6bb3a3f86ffb@redhat.com> <5349de2c-8a15-c599-f966-84b87a517453@Intel.com> <698c833c-163d-ccde-8f4e-eae083997895@Intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=leo.duran@amd.com; x-originating-ip: [165.204.77.1] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; CY4PR12MB1141; 20:9UjVmypI1UJMfgAhk81hMma71MDQULgwHyVhv/XuuLgYhdGnWtNGYSS+g/WE43gNDmlRa0DhIdNl/oSckUlBreyoHQnan7k/UmRNNYPk9bZ2CvRu0n+3mCZqTUpfTwDa02XtlGWa4/kuW6zLjLe+eBqoxObo/Sfbyyc6VK0a8k3O9MINHWv2Bz5m75dWUgZQesrOVHO/scCBKuGhtWDYdCvGWdIrBqXU+WnuCzQx4T1iVCuEbJsyi60hMcpHo1O9 x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 3393ac00-3d8b-49eb-d117-08d622f34898 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(4534165)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:CY4PR12MB1141; x-ms-traffictypediagnostic: CY4PR12MB1141: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(163750095850)(162533806227266)(228905959029699)(17755550239193)(767451399110); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3231355)(944501410)(52105095)(93006095)(93001095)(10201501046)(3002001)(6055026)(149066)(150027)(6041310)(20161123560045)(20161123558120)(20161123564045)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(201708071742011)(7699051); SRVR:CY4PR12MB1141; BCL:0; PCL:0; RULEID:; SRVR:CY4PR12MB1141; x-forefront-prvs: 08062C429B x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(366004)(396003)(39860400002)(346002)(136003)(376002)(13464003)(189003)(199004)(7736002)(478600001)(8936002)(9686003)(110136005)(446003)(106356001)(316002)(81166006)(2900100001)(33656002)(81156014)(14454004)(93886005)(966005)(53936002)(486006)(55016002)(6506007)(34290500001)(97736004)(2501003)(186003)(476003)(7696005)(6306002)(5660300001)(53546011)(25786009)(66066001)(102836004)(8676002)(3846002)(74316002)(305945005)(6246003)(2906002)(6436002)(256004)(229853002)(26005)(99286004)(68736007)(86362001)(4326008)(11346002)(6116002)(5250100002)(105586002)(76176011)(71200400001)(71190400001); DIR:OUT; SFP:1101; SCL:1; SRVR:CY4PR12MB1141; H:CY4PR12MB1815.namprd12.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: amd.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: otQp2sdbqXWv+w/V60UuNBSNweQdP39TyV+l5WwwfoIsXeCRGcntXLwvGP+Jv6/AAdDSZQOZCTXBEJabb3B0btNk6oE7aj5a1Ost4cwX/N0vuC2rD7qA5NJX6Vaw0c7K+FjLtCJYTPviXJddfFQhbd8dhaMz20lIZ/s2S29uUt34B/vpY+IL33+eTLaD1N/sLn2M9l/qp6g5gBRzEUiyA53JbSixGAdNCyu4l/iPu9QmoPkn2PVBuKWUBtX/k+z/j5eH/MVD2S1IT4ZhzyetIRkg5lQyx9vaGVYLLUo36yR5Zt6jdVJwHiP0KHwV2n9Sb15zZXeX7RoKINhQCiVzfowte9u8PBn5MiVBDh138Fk= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3393ac00-3d8b-49eb-d117-08d622f34898 X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Sep 2018 14:29:20.8824 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR12MB1141 Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Sep 2018 14:29:26 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ray & Eric, Where are we on this?... Here are my last two replies: > > Hi Ray, > > Please pardon the late reply. > > The main problem with changes to "caller" code is that dependencies > > are SoC-specific, so the detection code would not scale over time. > > Again, the proposed PCD does not alter existing flow (so existing code > > will continue to work as-is), and would give us a lever we can use in > > platform- specific code (without requiring surgery in EDK2 "caller" co= de). > > > BTW, > If you're concerned that someone may inadvertently set the PCD in their > platform, I can ensure the PCD only applies on AMD (similar to changes I > introduced in the APIC library). > For example, something like this: > // > // Disable MTRRs > // > if (!StandardSignatureIsAuthenticAMD () || !PcdGetBool > (PcdSkipDisableMtrrsOnPreMtrrChangeOnAmd)) { > DefType.Uint64 =3D AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE); > DefType.Bits.E =3D 0; > AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64); } >=20 > Please let me know if that's better, and will submit an updated patch. >=20 > > Thanks, > > Leo. > -----Original Message----- > From: edk2-devel On Behalf Of Duran, > Leo > Sent: Friday, September 21, 2018 12:13 PM > To: Ni, Ruiyu ; Laszlo Ersek ; > edk2-devel@lists.01.org > Cc: Dong, Eric > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disablin= g > MTRRs prior to MTRR change. >=20 >=20 >=20 > > -----Original Message----- > > From: Duran, Leo > > Sent: Friday, September 21, 2018 11:53 AM > > To: 'Ni, Ruiyu' ; Laszlo Ersek > > ; edk2-devel@lists.01.org > > Cc: Dong, Eric > > Subject: RE: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip > > disabling MTRRs prior to MTRR change. > > > > > > > > > -----Original Message----- > > > From: Ni, Ruiyu > > > Sent: Wednesday, September 19, 2018 3:59 AM > > > To: Duran, Leo ; Laszlo Ersek > > > ; edk2-devel@lists.01.org > > > Cc: Dong, Eric > > > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip > > > disabling MTRRs prior to MTRR change. > > > > > > On 9/18/2018 10:57 PM, Duran, Leo wrote: > > > > > > > > > > > >> -----Original Message----- > > > >> From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com] > > > >> Sent: Tuesday, September 18, 2018 3:34 AM > > > >> To: Laszlo Ersek ; Duran, Leo > > > ; > > > >> edk2-devel@lists.01.org > > > >> Cc: Dong, Eric > > > >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip > > > >> disabling MTRRs prior to MTRR change. > > > >> > > > >> On 9/18/2018 12:38 AM, Laszlo Ersek wrote: > > > >>> On 09/17/18 18:20, Duran, Leo wrote: > > > >>>> > > > >>>>> -----Original Message----- > > > >>>>> From: Ni, Ruiyu > > > >>>>> Sent: Thursday, September 13, 2018 11:44 PM > > > >>>>> To: Duran, Leo ; Laszlo Ersek > > > >>>>> ; edk2-devel@lists.01.org > > > >>>>> Cc: Dong, Eric > > > >>>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to > > > >>>>> skip disabling MTRRs prior to MTRR change. > > > >>>>> > > > >>>>> On 9/14/2018 3:31 AM, Duran, Leo wrote: > > > >>>>>> > > > >>>>>> > > > >>>>>>> -----Original Message----- > > > >>>>>>> From: Ni, Ruiyu > > > >>>>>>> Sent: Wednesday, September 12, 2018 9:39 PM > > > >>>>>>> To: Duran, Leo ; Laszlo Ersek > > > >>>>> ; > > > >>>>>>> edk2-devel@lists.01.org > > > >>>>>>> Cc: Dong, Eric > > > >>>>>>> Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip > > > >>>>>>> disabling MTRRs prior to MTRR change. > > > >>>>>>> > > > >>>>>>> Leo, > > > >>>>>>> Sorry I was in leave yesterday so didn't see the mail. > > > >>>>>>> Which MSRs are shared? Only the > > > >>>>> MSR_IA32_MTRR_DEF_TYPE_REGISTER? > > > >>>>>>> Or all the MSRs that configures the CPU MTRR setting? > > > >>>>>>> > > > >>>>>> > > > >>>>>> Hi Ray, > > > >>>>>> The MTTR config MSRs are also shared by threads within a core. > > > >>>>>> > > > >>>>> > > > >>>>> Hi Leo, > > > >>>>> Do you think that fixing the caller is more proper? > > > >>>> > > > >>>> Hi Ray, > > > >>>> Actually, > > > >>>> The proposed PCD is the simplest solution, as that works for us > > > >>>> and does > > > >> not change the existing (default) flow. > > > >>>> > > > >>>> That is, > > > >>>> I'd prefer making a decision about the PCD in platform-specific > > > >>>> code, > > > >> rather than introducing complex detection and heuristics at the > > > >> caller level in > > > >> EDK2 (just for AMD). > > > >>>> > > > >>>> So, please approve the PCD. > > > >> > > > >> Leo, > > > >> I agree with you on the first part "the PCD is the simplest soluti= on". > > > >> But this really looks like a workaround of the real issue. > > > >> For a multiple-socket system, it may contain S sockets, each > > > >> socket contains C cores and each core contains T threads. In > > > >> summary the system contains S * C * T threads. > > > >> As you said all threads inside a core share the MTRR setting. > > > >> Do all cores inside a socket share the MTRR setting? > > > >> Do all sockets share the MTRR setting? > > > >> > > > >> If one of the answer of above questions is "no", how can we > > > >> configure the PCD? > > > >> > > > > [Duran, Leo] > > > > Hi Ray, > > > > The MTTR settings are share by threads within a core (but each > > > > core has its own, etc.) The PCD would be set in our > > > > platform-specific code (e.g., > > > it can be set at build-time in the .DSC file). > > > > > > > > As I mentioned, > > > > We don't need (Mtrr.Enable=3D0) to change MTRR settings, so having > > > > the > > > PCD to skip (Mtrr.Enable=3D0) is reasonable for us. > > > > > > > > Leo. > > > > > > > > > > If the PCD is false, no thread disables the MTRR before programming i= t. > > > Is it safe? Per Intel's SDM, it's not. > > > > > > Maybe it works in AMD's case. But I still suggest we change the > > > caller, which is more natural. > > > At least I'd like to see how potential-ugly the change can be. > > > We can then discuss how to make the ugly change better looking. > > > > > > > Hi Ray, > > Please pardon the late reply. > > The main problem with changes to "caller" code is that dependencies > > are SoC-specific, so the detection code would not scale over time. > > Again, the proposed PCD does not alter existing flow (so existing code > > will continue to work as-is), and would give us a lever we can use in > > platform- specific code (without requiring surgery in EDK2 "caller" co= de). > > > BTW, > If you're concerned that someone may inadvertently set the PCD in their > platform, I can ensure the PCD only applies on AMD (similar to changes I > introduced in the APIC library). > For example, something like this: > // > // Disable MTRRs > // > if (!StandardSignatureIsAuthenticAMD () || !PcdGetBool > (PcdSkipDisableMtrrsOnPreMtrrChangeOnAmd)) { > DefType.Uint64 =3D AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE); > DefType.Bits.E =3D 0; > AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64); } >=20 > Please let me know if that's better, and will submit an updated patch. >=20 > > Thanks, > > Leo. > > > > > >>> > > > >>> - From my side, if it works for you, it works for me. (The > > > >>> general trend has been to avoid adding more PCDs to the "core" > > > >>> package DEC files, but I'm 100% neutral on that.) > > > >>> > > > >>> Laszlo > > > >>> > > > >> > > > >> Laszlo, > > > >> Thanks for pointing out the general trend. Yes less PCDs are very > > > welcomed. > > > >> To me, PCD is no different from protocol. And even worse, because > > > >> it's very easily to be over-used. > > > >> But I am not sure whether a PCD has to be introduced for this issu= e. > > > >> Maybe even we choose to fix the caller, the PCD is still needed. > > > >> I am not sure. > > > >> > > > >> -- > > > >> Thanks, > > > >> Ray > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > > > > > > > > > > -- > > > Thanks, > > > Ray > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel