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

[Laszlo Ersek <lersek@redhat.com>]

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|0x0000001B
>  
> +  ## 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.

More questions:

- Don't you need a similar (symmetric) change in
MtrrLibPostMtrrChange()? If not, why not? Can you add that to the commit
message?

- I don't know how exactly the current (sophisticated?) MTRR update
algorithm works (developed by Ray); are you sure it's safe to let it
massage "armed" MSRs while DefType.Bits.E is set?

(Hmmm, sorry if this "review" is more centered on my own lack of
information than on your patch...)

Thanks
Laszlo