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

[Laszlo Ersek <lersek@redhat.com>]

On 09/12/18 17:17, Duran, Leo wrote:
> Laszlo, et al,
> 
> Perhaps it would be best if I provide an example of the problem I'm trying to solve, and perhaps we may chime in with suggestions.
> 
> Again,
> The fundamental issue has to do with shared MTRR control by set of threads within a core with SMT enabled.
> So ideally only one thread (the first thread that wakes up) from a set would configure the MSR, and other threads in the set would not need to.
> 
> The problem with the existing code is that once the first thread configures the MSR, another thread in the set follows and clears the ENABLE bit in MtrrLibPreMtrrChange().

Right, I think I understood that. What I don't understand is:

> (Basically, the second thread pulls the rug from under the first thread).

why it is a problem that, when the second thread wakes up on the same
core, it repeats the whole MTRR configuration? It does clear the Enable
bit temporarily, yes, but in the end it re-stores the exact same MTRR
config. And while this happens, the first thread on the core (already
past the MTRR setup) shouldn't be doing anything at all, because all
logical CPUs should sit tight until all of them complete the MTRR setup.


Now, of course, if the MTRR setup runs in parallel on all logical CPUs
(I can't recall off-hand whether they are started up in parallel or
one-by-one -- I think it's "free for all" though), and on various CPU
models this modifies resources that are shared, then we have a more
serious problem. Because then two threads on the same core may modify
parts of the shared MTRR settings *other* than just the Enable bit.

Perhaps this should be solved by adding suitable exclusion, so that only
one thread on each core configure the MTRR. In other words, continue to
permit full parallelism between cores, but restrict each core to just
one logical thread, when doing the MTRR changes.

I wonder if the right approach is to discover the CPU topology up-front
(based on the initial APIC IDs perhaps?), and to modify the MTRR setup
callback for StartupAllAPs -- on the affected CPU models -- so that the
callback returns without doing anything on the initially "blacklisted"
threads.

By the time we are in MtrrLib, it's likely too late to catch the
non-favored theads.

(I'd very much like others to chime in too.)

Thanks
Laszlo

> 
> I hope that helps explain what I'm after.
> Leo.
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, September 12, 2018 4:55 AM
>> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
>> Cc: Eric Dong <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>
>> Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs
>> prior to MTRR change.
>>
>> On 09/11/18 21:47, Duran, Leo wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> Sent: Tuesday, September 11, 2018 1:50 PM
>>>> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
>>>> Cc: Eric Dong <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
>>>> MTRRs prior to MTRR change.
>>>>
>>>> On 09/11/18 17:41, Leo Duran wrote:
>>>>> The default behavior is to disable MTRRs prior to an MTRR change.
>>>>> However, on SMT platforms with shared CPU resources it may be
>>>>> desirable to skip the default behavior, and leave the current state
>>>>> of the
>>>> Enable bit.
>>>>>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Leo Duran <leo.duran@amd.com>
>>>>> ---
>>>>>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c   | 10 +++++++---
>>>>>  UefiCpuPkg/Library/MtrrLib/MtrrLib.inf |  3 +++
>>>>>  UefiCpuPkg/UefiCpuPkg.dec              |  7 +++++++
>>>>>  3 files changed, 17 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
>>>>> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
>>>>> index dfce9a9..baf9a0f 100644
>>>>> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
>>>>> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
>>>>> @@ -6,6 +6,8 @@
>>>>>      except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR
>>>>> setting to
>>>> APs.
>>>>>
>>>>>    Copyright (c) 2008 - 2018, Intel Corporation. All rights
>>>>> reserved.<BR>
>>>>> +  Copyright (c) 2018, AMD Inc. All rights reserved.<BR>
>>>>> +
>>>>>    This program and the accompanying materials
>>>>>    are licensed and made available under the terms and conditions of
>>>>> the
>>>> BSD License
>>>>>    which accompanies this distribution.  The full text of the
>>>>> license may be found at @@ -310,9 +312,11 @@ MtrrLibPreMtrrChange
>> (
>>>>>    //
>>>>>    // Disable MTRRs
>>>>>    //
>>>>> -  DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
>>>>> -  DefType.Bits.E = 0;
>>>>> -  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
>>>>> +  if (!PcdGetBool (PcdSkipDisableMtrrsOnPreMtrrChange)) {
>>>>> +    DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
>>>>> +    DefType.Bits.E = 0;
>>>>> +    AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);  }
>>>>>  }
>>>>>
>>>>>  /**
>>>>> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
>>>> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
>>>>> index a97cc61..06f33e8 100644
>>>>> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
>>>>> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
>>>>> @@ -2,6 +2,8 @@
>>>>>  #  MTRR library provides APIs for MTRR operation.
>>>>>  #
>>>>>  #  Copyright (c) 2006 - 2018, Intel Corporation. All rights
>>>>> reserved.<BR>
>>>>> +#  Copyright (c) 2018, AMD Inc. All rights reserved.<BR> #
>>>>>  #  This program and the accompanying materials  #  are licensed and
>>>>> made available under the terms and conditions of the
>>>> BSD License
>>>>>  #  which accompanies this distribution.  The full text of the
>>>>> license may be
>>>> found at
>>>>> @@ -43,4 +45,5 @@
>>>>>
>>>>>  [Pcd]
>>>>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuNumberOfReservedVariableMtrrs
>>>> ## SOMETIMES_CONSUMES
>>>>> +  gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange
>>>> ## CONSUMES
>>>>>
>>>>> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
>>>>> index 69d777a..64ec763 100644
>>>>> --- a/UefiCpuPkg/UefiCpuPkg.dec
>>>>> +++ b/UefiCpuPkg/UefiCpuPkg.dec
>>>>> @@ -2,6 +2,7 @@
>>>>>  # This Package provides UEFI compatible CPU modules and libraries.
>>>>>  #
>>>>>  # Copyright (c) 2007 - 2017, Intel Corporation. All rights
>>>>> reserved.<BR>
>>>>> +# Copyright (c) 2018, AMD Inc. All rights reserved.<BR>
>>>>>  #
>>>>>  # This program and the accompanying materials are licensed and made
>>>> available under
>>>>>  # the terms and conditions of the BSD License which accompanies
>>>>> this
>>>> distribution.
>>>>> @@ -273,6 +274,12 @@
>>>>>    # @Prompt Current boot is a power-on reset.
>>>>>
>>>>
>> gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x0000
>>>> 001B
>>>>>
>>>>> +  ## Indicates desired behavior for disabling MTRRs prior to MTRR
>>>> change.<BR><BR>
>>>>> +  #   TRUE  - Skip disabling MTRRs prior to MTRR change.<BR>
>>>>> +  #   FALSE - Disable MTRRs prior to MTRR change.<BR>
>>>>> +  # @Prompt Desired behavior for disabling MTRRs prior to MTRR
>> change.
>>>>> +
>>>>
>> gUefiCpuPkgTokenSpaceGuid.PcdSkipDisableMtrrsOnPreMtrrChange|FALSE
>>>> |BOOLEAN|0x0000001E
>>>>> +
>>>>>  [PcdsDynamic, PcdsDynamicEx]
>>>>>    ## Contains the pointer to a CPU S3 data buffer of structure
>>>> ACPI_CPU_DATA.
>>>>>    # @Prompt The pointer to a CPU S3 data buffer.
>>>>>
>>>>
>>>> Recently, Ray has written several & significant patches for MtrrLib;
>>>> I'm adding him.
>>>>
>>>> I don't understand the motivation behind this patch. As far as I
>>>> remember (which is admittedly "quite vaguely"), the SDM requires all
>>>> logical processors to program their MTRRs identically in parallel.
>>>> That is, there should be a start line where all the CPUs wait for
>>>> each other, then they all set up their MTRRs, then they all wait
>>>> until they all finish, then they all go their merry ways. AIUI the
>>>> CPU MP PPI and protocol implement this already. I don't understand in
>>>> what situation you'd have one thread of a core manipulating MTRR,
>>>> with the sibling thread *not* manipulating MTRR (i.e., doing
>>>> something else). That doesn't seem to match what the SDM dictates
>>>> (or, well, what my memories of the SDM are :) ).
>>>>
>>>> I see that the default behavior doesn't change, and I'm not against
>>>> the patch; I just suspect that, for introducing a new PCD, more
>>>> concrete / practical justification could be needed.
>>>>
>>>
>>> In our case multiple processors (threads) share MTTR settings, hence the
>> Enable bit is shared.
>>>
>>>> More questions:
>>>>
>>>> - Don't you need a similar (symmetric) change in
>>>> MtrrLibPostMtrrChange()? If not, why not? Can you add that to the
>>>> commit message?
>>>
>>> No need. The "Post" code just restores.
>>> Sure, I can add a comment.
>>
>> Sure, functionally it doesn't hurt I guess, but you could avoid a RMW to
>> MSR_IA32_MTRR_DEF_TYPE.
>>
>> Or is it necessary to rewrite the FE bit? (It doesn't look necessary to me, but I
>> could be wrong.)
>>
>> Or maybe you need to write to the MSR the *first* time that
>> MtrrLibPostMtrrChange() is called...
>>
>> Anyway, I'm OK with the patch, I'll let Eric and Ray decide.
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>
>>
>> Thanks
>> Laszlo