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

[Laszlo Ersek <lersek@redhat.com>]

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