From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.120; helo=mga04.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DC1242114398A for ; Wed, 19 Sep 2018 01:58:10 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Sep 2018 01:58:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,393,1531810800"; d="scan'208";a="93028796" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.8]) ([10.239.9.8]) by orsmga002.jf.intel.com with ESMTP; 19 Sep 2018 01:58:00 -0700 To: "Duran, Leo" , Laszlo Ersek , "edk2-devel@lists.01.org" Cc: "Dong, Eric" 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> From: "Ni, Ruiyu" Message-ID: <698c833c-163d-ccde-8f4e-eae083997895@Intel.com> Date: Wed, 19 Sep 2018 16:58:54 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: 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: Wed, 19 Sep 2018 08:58:11 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 solution". >> 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=0) to change MTRR settings, so having the PCD to skip (Mtrr.Enable=0) is reasonable for us. > > Leo. > If the PCD is false, no thread disables the MTRR before programming it. 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. >>> >>> - 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 issue. >> 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