> -----Original Message-----
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Wednesday, September 12, 2018 12:59 PM
> To: Duran, Leo <leo.du...@amd.com>; edk2-devel@lists.01.org
> Cc: Eric Dong <eric.d...@intel.com>; Ruiyu Ni <ruiyu...@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 <ler...@redhat.com>
> >> Sent: Wednesday, September 12, 2018 4:55 AM
> >> To: Duran, Leo <leo.du...@amd.com>; edk2-devel@lists.01.org
> >> Cc: Eric Dong <eric.d...@intel.com>; Ruiyu Ni <ruiyu...@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 <ler...@redhat.com>
> >>>> Sent: Tuesday, September 11, 2018 1:50 PM
> >>>> To: Duran, Leo <leo.du...@amd.com>; edk2-devel@lists.01.org
> >>>> Cc: Eric Dong <eric.d...@intel.com>; Ruiyu Ni <ruiyu...@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.d...@intel.com>
> >>>>> Cc: Laszlo Ersek <ler...@redhat.com>
> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>>> Signed-off-by: Leo Duran <leo.du...@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 <ler...@redhat.com>
> >>
> >>
> >> Thanks
> >> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
