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

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

> -----Original Message-----
> From: Ni, Ruiyu <ruiyu.ni@intel.com>
> Sent: Wednesday, September 12, 2018 9:39 PM
> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs
> prior to MTRR change.
> 
> 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?
> 

Hi Ray,
The MTTR config MSRs are also shared by threads within a core.

> 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