Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change.

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
> On 09/17/18 18:20, Duran, Leo wrote:
>>
>>> -----Original Message-----
>>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
>>> Sent: Thursday, September 13, 2018 11:44 PM
>>> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek <lersek@redhat.com>;
>>> edk2-devel@lists.01.org
>>> Cc: Dong, Eric <eric.dong@intel.com>
>>> 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 <ruiyu.ni@intel.com>
>>>>> Sent: Wednesday, September 12, 2018 9:39 PM
>>>>> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek
>>> <lersek@redhat.com>;
>>>>> edk2-devel@lists.01.org
>>>>> Cc: Dong, Eric <eric.dong@intel.com>
>>>>> 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?

> 
> - 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