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

["Duran, Leo" <leo.duran@amd.com>]

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

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

Yes, seems fine in our case.

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

NP.

> Thanks
> Laszlo