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

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

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?

I also agree with Laszlo's comments to fix the caller if all MSRs relating to MTRR are shared.
That will be to fix MpInitLib and CpuDxe driver.

Thanks/Ray

> -----Original Message-----
> From: Duran, Leo <leo.duran@amd.com>
> Sent: Thursday, September 13, 2018 2:22 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs
> prior to MTRR change.
> 
> 
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Wednesday, September 12, 2018 12:59 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/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.
> >
> 
> I also think it's a 'free for all"
> (My understanding is that the BSP issues a broadcast to the APs.)
> 
> Perhaps if the second thread waits for the first thread to complete and get
> safely parked, the first thread may be then immune to the subsequent
> changes.
> 
> > 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.
> >
> 
> Yes, agreed!
> That would be ideal for the case where threads in a core share MTRR
> resources.
> 
> Unfortunately, MtrrSetAllMtrrs() gets called from quite a few places, so
> orchestrating the "only one logical thread" logic may be quite complicated.
> Again, I presume that while the worker thread makes the MTRR changes, the
> other threads must be safely parked and immune to toggling of the Enable
> bit.
> 
> > 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