[PATCH] Add flag to skip disabling MTRRs

[PATCH] Add flag to skip disabling MTRRs

[Leo Duran]

This patch adds a flag that will allow us to skip disabling MTRRs on SMT
platforms where the MTRR Enable bit is shared across threads in a CPU core.

The default behavior is unchanged, so existing implementations are not
affected by this patch.

Leo Duran (1):
  UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR
    change.

 UefiCpuPkg/Library/MtrrLib/MtrrLib.c   | 10 +++++++---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.inf |  3 +++
 UefiCpuPkg/UefiCpuPkg.dec              |  7 +++++++
 3 files changed, 17 insertions(+), 3 deletions(-)

-- 
2.7.4

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

[Leo Duran]

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

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

[Laszlo Ersek]

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

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

[Duran, Leo]

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

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

[Laszlo Ersek]

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

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

[Duran, Leo]

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().
(Basically, the second thread pulls the rug from under the first thread).

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

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

[Laszlo Ersek]

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.

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.

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

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

[Duran, Leo]

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

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

[Ni, Ruiyu]

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

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

[Duran, Leo]

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

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

[Ni, Ruiyu]

On 9/14/2018 3:31 AM, Duran, Leo wrote:
> 
> 
>> -----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.
> 

Hi Leo,
Do you think that fixing the caller is more proper?

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

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

[Duran, Leo]

> -----Original Message-----
> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> MTRRs prior to MTRR change.
> 
> On 9/14/2018 3:31 AM, Duran, Leo wrote:
> >
> >
> >> -----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.
> >
> 
> Hi Leo,
> Do you think that fixing the caller is more proper?

Hi Ray,
Actually,
The proposed PCD is the simplest solution, as that works for us and does not change the existing (default) flow.

That is,
I'd prefer making a decision about the PCD in platform-specific code, rather than introducing complex detection and heuristics at the caller level in EDK2 (just for AMD).

So, please approve the PCD.

Thanks,
Leo

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

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

[Laszlo Ersek]

On 09/17/18 18:20, Duran, Leo wrote:
> 
>> -----Original Message-----
>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
>> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
>> MTRRs prior to MTRR change.
>>
>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
>>>
>>>
>>>> -----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.
>>>
>>
>> Hi Leo,
>> Do you think that fixing the caller is more proper?
> 
> Hi Ray,
> Actually,
> The proposed PCD is the simplest solution, as that works for us and does not change the existing (default) flow.
> 
> That is,
> I'd prefer making a decision about the PCD in platform-specific code, rather than introducing complex detection and heuristics at the caller level in EDK2 (just for AMD).
> 
> So, please approve the PCD.

- From my side, if it works for you, it works for me. (The general trend
has been to avoid adding more PCDs to the "core" package DEC files, but
I'm 100% neutral on that.)

Laszlo

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

[Ni, Ruiyu]

On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
> On 09/17/18 18:20, Duran, Leo wrote:
>>
>>> -----Original Message-----
>>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
>>> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
>>> MTRRs prior to MTRR change.
>>>
>>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
>>>>
>>>>
>>>>> -----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.
>>>>
>>>
>>> Hi Leo,
>>> Do you think that fixing the caller is more proper?
>>
>> Hi Ray,
>> Actually,
>> The proposed PCD is the simplest solution, as that works for us and does not change the existing (default) flow.
>>
>> That is,
>> I'd prefer making a decision about the PCD in platform-specific code, rather than introducing complex detection and heuristics at the caller level in EDK2 (just for AMD).
>>
>> So, please approve the PCD.

Leo,
I agree with you on the first part "the PCD is the simplest solution".
But this really looks like a workaround of the real issue.
For a multiple-socket system, it may contain S sockets, each socket 
contains C cores and each core contains T threads. In summary the system 
contains S * C * T threads.
As you said all threads inside a core share the MTRR setting.
Do all cores inside a socket share the MTRR setting?
Do all sockets share the MTRR setting?

If one of the answer of above questions is "no", how can we configure 
the PCD?

> 
> - From my side, if it works for you, it works for me. (The general trend
> has been to avoid adding more PCDs to the "core" package DEC files, but
> I'm 100% neutral on that.)
> 
> Laszlo
> 

Laszlo,
Thanks for pointing out the general trend. Yes less PCDs are very 
welcomed. To me, PCD is no different from protocol. And even worse, 
because it's very easily to be over-used.
But I am not sure whether a PCD has to be introduced for this issue. 
Maybe even we choose to fix the caller, the PCD is still needed. I am 
not sure.

-- 
Thanks,
Ray

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

[Duran, Leo]

> -----Original Message-----
> From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com]
> Sent: Tuesday, September 18, 2018 3:34 AM
> To: Laszlo Ersek <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> MTRRs prior to MTRR change.
> 
> On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
> > On 09/17/18 18:20, Duran, Leo wrote:
> >>
> >>> -----Original Message-----
> >>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> >>> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> >>> disabling MTRRs prior to MTRR change.
> >>>
> >>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
> >>>>
> >>>>
> >>>>> -----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.
> >>>>
> >>>
> >>> Hi Leo,
> >>> Do you think that fixing the caller is more proper?
> >>
> >> Hi Ray,
> >> Actually,
> >> The proposed PCD is the simplest solution, as that works for us and does
> not change the existing (default) flow.
> >>
> >> That is,
> >> I'd prefer making a decision about the PCD in platform-specific code,
> rather than introducing complex detection and heuristics at the caller level in
> EDK2 (just for AMD).
> >>
> >> So, please approve the PCD.
> 
> Leo,
> I agree with you on the first part "the PCD is the simplest solution".
> But this really looks like a workaround of the real issue.
> For a multiple-socket system, it may contain S sockets, each socket contains C
> cores and each core contains T threads. In summary the system contains S *
> C * T threads.
> As you said all threads inside a core share the MTRR setting.
> Do all cores inside a socket share the MTRR setting?
> Do all sockets share the MTRR setting?
> 
> If one of the answer of above questions is "no", how can we configure the
> PCD?
> 
[Duran, Leo] 
Hi Ray,
The MTTR settings are share by threads within a core (but each core has its own, etc.)
The PCD would be set in our platform-specific code (e.g., it can be set at build-time in the .DSC file).

As I mentioned, 
We don't need (Mtrr.Enable=0) to change MTRR settings, so having the PCD to skip (Mtrr.Enable=0) is reasonable for us.

Leo.

> >
> > - From my side, if it works for you, it works for me. (The general
> > trend has been to avoid adding more PCDs to the "core" package DEC
> > files, but I'm 100% neutral on that.)
> >
> > Laszlo
> >
> 
> Laszlo,
> Thanks for pointing out the general trend. Yes less PCDs are very welcomed.
> To me, PCD is no different from protocol. And even worse, because it's very
> easily to be over-used.
> But I am not sure whether a PCD has to be introduced for this issue.
> Maybe even we choose to fix the caller, the PCD is still needed. I am not
> sure.
> 
> --
> Thanks,
> Ray

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

[Ni, Ruiyu]

On 9/18/2018 10:57 PM, Duran, Leo wrote:
> 
> 
>> -----Original Message-----
>> From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com]
>> Sent: Tuesday, September 18, 2018 3:34 AM
>> To: Laszlo Ersek <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>;
>> edk2-devel@lists.01.org
>> Cc: Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
>> MTRRs prior to MTRR change.
>>
>> On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
>>> On 09/17/18 18:20, Duran, Leo wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
>>>>> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
>>>>> disabling MTRRs prior to MTRR change.
>>>>>
>>>>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
>>>>>>
>>>>>>
>>>>>>> -----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.
>>>>>>
>>>>>
>>>>> Hi Leo,
>>>>> Do you think that fixing the caller is more proper?
>>>>
>>>> Hi Ray,
>>>> Actually,
>>>> The proposed PCD is the simplest solution, as that works for us and does
>> not change the existing (default) flow.
>>>>
>>>> That is,
>>>> I'd prefer making a decision about the PCD in platform-specific code,
>> rather than introducing complex detection and heuristics at the caller level in
>> EDK2 (just for AMD).
>>>>
>>>> So, please approve the PCD.
>>
>> Leo,
>> I agree with you on the first part "the PCD is the simplest solution".
>> But this really looks like a workaround of the real issue.
>> For a multiple-socket system, it may contain S sockets, each socket contains C
>> cores and each core contains T threads. In summary the system contains S *
>> C * T threads.
>> As you said all threads inside a core share the MTRR setting.
>> Do all cores inside a socket share the MTRR setting?
>> Do all sockets share the MTRR setting?
>>
>> If one of the answer of above questions is "no", how can we configure the
>> PCD?
>>
> [Duran, Leo]
> Hi Ray,
> The MTTR settings are share by threads within a core (but each core has its own, etc.)
> The PCD would be set in our platform-specific code (e.g., it can be set at build-time in the .DSC file).
> 
> As I mentioned,
> We don't need (Mtrr.Enable=0) to change MTRR settings, so having the PCD to skip (Mtrr.Enable=0) is reasonable for us.
> 
> Leo.
> 

If the PCD is false, no thread disables the MTRR before programming it.
Is it safe? Per Intel's SDM, it's not.

Maybe it works in AMD's case. But I still suggest we change the caller, 
which is more natural.
At least I'd like to see how potential-ugly the change can be.
We can then discuss how to make the ugly change better looking.

>>>
>>> - From my side, if it works for you, it works for me. (The general
>>> trend has been to avoid adding more PCDs to the "core" package DEC
>>> files, but I'm 100% neutral on that.)
>>>
>>> Laszlo
>>>
>>
>> Laszlo,
>> Thanks for pointing out the general trend. Yes less PCDs are very welcomed.
>> To me, PCD is no different from protocol. And even worse, because it's very
>> easily to be over-used.
>> But I am not sure whether a PCD has to be introduced for this issue.
>> Maybe even we choose to fix the caller, the PCD is still needed. I am not
>> sure.
>>
>> --
>> Thanks,
>> Ray
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 
Thanks,
Ray

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

[Duran, Leo]

> -----Original Message-----
> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> Sent: Wednesday, September 19, 2018 3:59 AM
> 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> MTRRs prior to MTRR change.
> 
> On 9/18/2018 10:57 PM, Duran, Leo wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com]
> >> Sent: Tuesday, September 18, 2018 3:34 AM
> >> To: Laszlo Ersek <lersek@redhat.com>; Duran, Leo
> <leo.duran@amd.com>;
> >> edk2-devel@lists.01.org
> >> Cc: Dong, Eric <eric.dong@intel.com>
> >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> >> disabling MTRRs prior to MTRR change.
> >>
> >> On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
> >>> On 09/17/18 18:20, Duran, Leo wrote:
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> >>>>> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> >>>>> disabling MTRRs prior to MTRR change.
> >>>>>
> >>>>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----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.
> >>>>>>
> >>>>>
> >>>>> Hi Leo,
> >>>>> Do you think that fixing the caller is more proper?
> >>>>
> >>>> Hi Ray,
> >>>> Actually,
> >>>> The proposed PCD is the simplest solution, as that works for us and
> >>>> does
> >> not change the existing (default) flow.
> >>>>
> >>>> That is,
> >>>> I'd prefer making a decision about the PCD in platform-specific
> >>>> code,
> >> rather than introducing complex detection and heuristics at the
> >> caller level in
> >> EDK2 (just for AMD).
> >>>>
> >>>> So, please approve the PCD.
> >>
> >> Leo,
> >> I agree with you on the first part "the PCD is the simplest solution".
> >> But this really looks like a workaround of the real issue.
> >> For a multiple-socket system, it may contain S sockets, each socket
> >> contains C cores and each core contains T threads. In summary the
> >> system contains S * C * T threads.
> >> As you said all threads inside a core share the MTRR setting.
> >> Do all cores inside a socket share the MTRR setting?
> >> Do all sockets share the MTRR setting?
> >>
> >> If one of the answer of above questions is "no", how can we configure
> >> the PCD?
> >>
> > [Duran, Leo]
> > Hi Ray,
> > The MTTR settings are share by threads within a core (but each core
> > has its own, etc.) The PCD would be set in our platform-specific code (e.g.,
> it can be set at build-time in the .DSC file).
> >
> > As I mentioned,
> > We don't need (Mtrr.Enable=0) to change MTRR settings, so having the
> PCD to skip (Mtrr.Enable=0) is reasonable for us.
> >
> > Leo.
> >
> 
> If the PCD is false, no thread disables the MTRR before programming it.
> Is it safe? Per Intel's SDM, it's not.
> 
> Maybe it works in AMD's case. But I still suggest we change the caller, which
> is more natural.
> At least I'd like to see how potential-ugly the change can be.
> We can then discuss how to make the ugly change better looking.
> 

Hi Ray,
Please pardon the late reply.
The main problem with changes to "caller" code is that dependencies are SoC-specific, so the detection code would not scale over time.
Again, the proposed PCD does not alter existing flow (so existing code will continue to work as-is),
and would give us a lever we can use in platform-specific code  (without requiring surgery in EDK2 "caller" code).

Thanks,
Leo.

> >>>
> >>> - From my side, if it works for you, it works for me. (The general
> >>> trend has been to avoid adding more PCDs to the "core" package DEC
> >>> files, but I'm 100% neutral on that.)
> >>>
> >>> Laszlo
> >>>
> >>
> >> Laszlo,
> >> Thanks for pointing out the general trend. Yes less PCDs are very
> welcomed.
> >> To me, PCD is no different from protocol. And even worse, because
> >> it's very easily to be over-used.
> >> But I am not sure whether a PCD has to be introduced for this issue.
> >> Maybe even we choose to fix the caller, the PCD is still needed. I am
> >> not sure.
> >>
> >> --
> >> Thanks,
> >> Ray
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> 
> 
> --
> Thanks,
> Ray

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

[Duran, Leo]

> -----Original Message-----
> From: Duran, Leo
> Sent: Friday, September 21, 2018 11:53 AM
> To: 'Ni, Ruiyu' <ruiyu.ni@Intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> MTRRs prior to MTRR change.
> 
> 
> 
> > -----Original Message-----
> > From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> > Sent: Wednesday, September 19, 2018 3:59 AM
> > 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> > disabling MTRRs prior to MTRR change.
> >
> > On 9/18/2018 10:57 PM, Duran, Leo wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com]
> > >> Sent: Tuesday, September 18, 2018 3:34 AM
> > >> To: Laszlo Ersek <lersek@redhat.com>; Duran, Leo
> > <leo.duran@amd.com>;
> > >> edk2-devel@lists.01.org
> > >> Cc: Dong, Eric <eric.dong@intel.com>
> > >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> > >> disabling MTRRs prior to MTRR change.
> > >>
> > >> On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
> > >>> On 09/17/18 18:20, Duran, Leo wrote:
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> > >>>>> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> > >>>>> disabling MTRRs prior to MTRR change.
> > >>>>>
> > >>>>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>>> -----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.
> > >>>>>>
> > >>>>>
> > >>>>> Hi Leo,
> > >>>>> Do you think that fixing the caller is more proper?
> > >>>>
> > >>>> Hi Ray,
> > >>>> Actually,
> > >>>> The proposed PCD is the simplest solution, as that works for us
> > >>>> and does
> > >> not change the existing (default) flow.
> > >>>>
> > >>>> That is,
> > >>>> I'd prefer making a decision about the PCD in platform-specific
> > >>>> code,
> > >> rather than introducing complex detection and heuristics at the
> > >> caller level in
> > >> EDK2 (just for AMD).
> > >>>>
> > >>>> So, please approve the PCD.
> > >>
> > >> Leo,
> > >> I agree with you on the first part "the PCD is the simplest solution".
> > >> But this really looks like a workaround of the real issue.
> > >> For a multiple-socket system, it may contain S sockets, each socket
> > >> contains C cores and each core contains T threads. In summary the
> > >> system contains S * C * T threads.
> > >> As you said all threads inside a core share the MTRR setting.
> > >> Do all cores inside a socket share the MTRR setting?
> > >> Do all sockets share the MTRR setting?
> > >>
> > >> If one of the answer of above questions is "no", how can we
> > >> configure the PCD?
> > >>
> > > [Duran, Leo]
> > > Hi Ray,
> > > The MTTR settings are share by threads within a core (but each core
> > > has its own, etc.) The PCD would be set in our platform-specific
> > > code (e.g.,
> > it can be set at build-time in the .DSC file).
> > >
> > > As I mentioned,
> > > We don't need (Mtrr.Enable=0) to change MTRR settings, so having the
> > PCD to skip (Mtrr.Enable=0) is reasonable for us.
> > >
> > > Leo.
> > >
> >
> > If the PCD is false, no thread disables the MTRR before programming it.
> > Is it safe? Per Intel's SDM, it's not.
> >
> > Maybe it works in AMD's case. But I still suggest we change the
> > caller, which is more natural.
> > At least I'd like to see how potential-ugly the change can be.
> > We can then discuss how to make the ugly change better looking.
> >
> 
> Hi Ray,
> Please pardon the late reply.
> The main problem with changes to "caller" code is that dependencies are
> SoC-specific, so the detection code would not scale over time.
> Again, the proposed PCD does not alter existing flow (so existing code will
> continue to work as-is), and would give us a lever we can use in platform-
> specific code  (without requiring surgery in EDK2 "caller" code).
> 
BTW,
If you're concerned that someone may inadvertently set the PCD in their platform, I can ensure the PCD only applies on AMD (similar to changes I introduced in the APIC library).
For example, something like this:
  //
  // Disable MTRRs
  //
  if (!StandardSignatureIsAuthenticAMD () || !PcdGetBool (PcdSkipDisableMtrrsOnPreMtrrChangeOnAmd)) {
    DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
    DefType.Bits.E = 0;
    AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
 }

Please let me know if that's better, and will submit an updated patch.

> Thanks,
> Leo.
> 
> > >>>
> > >>> - From my side, if it works for you, it works for me. (The general
> > >>> trend has been to avoid adding more PCDs to the "core" package DEC
> > >>> files, but I'm 100% neutral on that.)
> > >>>
> > >>> Laszlo
> > >>>
> > >>
> > >> Laszlo,
> > >> Thanks for pointing out the general trend. Yes less PCDs are very
> > welcomed.
> > >> To me, PCD is no different from protocol. And even worse, because
> > >> it's very easily to be over-used.
> > >> But I am not sure whether a PCD has to be introduced for this issue.
> > >> Maybe even we choose to fix the caller, the PCD is still needed. I
> > >> am not sure.
> > >>
> > >> --
> > >> Thanks,
> > >> Ray
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > >
> >
> >
> > --
> > Thanks,
> > Ray

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

[Duran, Leo]

Hi Ray & Eric,

Where are we on this?... Here are my last two replies:

> > Hi Ray,
> > Please pardon the late reply.
> > The main problem with changes to "caller" code is that dependencies
> > are SoC-specific, so the detection code would not scale over time.
> > Again, the proposed PCD does not alter existing flow (so existing code
> > will continue to work as-is), and would give us a lever we can use in
> > platform- specific code  (without requiring surgery in EDK2 "caller" code).
> >
> BTW,
> If you're concerned that someone may inadvertently set the PCD in their
> platform, I can ensure the PCD only applies on AMD (similar to changes I
> introduced in the APIC library).
> For example, something like this:
>   //
>   // Disable MTRRs
>   //
>   if (!StandardSignatureIsAuthenticAMD () || !PcdGetBool
> (PcdSkipDisableMtrrsOnPreMtrrChangeOnAmd)) {
>     DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
>     DefType.Bits.E = 0;
>     AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);  }
> 
> Please let me know if that's better, and will submit an updated patch.
> 
> > Thanks,
> > Leo.

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Duran,
> Leo
> Sent: Friday, September 21, 2018 12:13 PM
> To: Ni, Ruiyu <ruiyu.ni@Intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> MTRRs prior to MTRR change.
> 
> 
> 
> > -----Original Message-----
> > From: Duran, Leo
> > Sent: Friday, September 21, 2018 11:53 AM
> > To: 'Ni, Ruiyu' <ruiyu.ni@Intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> > disabling MTRRs prior to MTRR change.
> >
> >
> >
> > > -----Original Message-----
> > > From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> > > Sent: Wednesday, September 19, 2018 3:59 AM
> > > 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> > > disabling MTRRs prior to MTRR change.
> > >
> > > On 9/18/2018 10:57 PM, Duran, Leo wrote:
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com]
> > > >> Sent: Tuesday, September 18, 2018 3:34 AM
> > > >> To: Laszlo Ersek <lersek@redhat.com>; Duran, Leo
> > > <leo.duran@amd.com>;
> > > >> edk2-devel@lists.01.org
> > > >> Cc: Dong, Eric <eric.dong@intel.com>
> > > >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> > > >> disabling MTRRs prior to MTRR change.
> > > >>
> > > >> On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
> > > >>> On 09/17/18 18:20, Duran, Leo wrote:
> > > >>>>
> > > >>>>> -----Original Message-----
> > > >>>>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> > > >>>>> Sent: Thursday, September 13, 2018 11:44 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: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to
> > > >>>>> skip disabling MTRRs prior to MTRR change.
> > > >>>>>
> > > >>>>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>> -----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.
> > > >>>>>>
> > > >>>>>
> > > >>>>> Hi Leo,
> > > >>>>> Do you think that fixing the caller is more proper?
> > > >>>>
> > > >>>> Hi Ray,
> > > >>>> Actually,
> > > >>>> The proposed PCD is the simplest solution, as that works for us
> > > >>>> and does
> > > >> not change the existing (default) flow.
> > > >>>>
> > > >>>> That is,
> > > >>>> I'd prefer making a decision about the PCD in platform-specific
> > > >>>> code,
> > > >> rather than introducing complex detection and heuristics at the
> > > >> caller level in
> > > >> EDK2 (just for AMD).
> > > >>>>
> > > >>>> So, please approve the PCD.
> > > >>
> > > >> Leo,
> > > >> I agree with you on the first part "the PCD is the simplest solution".
> > > >> But this really looks like a workaround of the real issue.
> > > >> For a multiple-socket system, it may contain S sockets, each
> > > >> socket contains C cores and each core contains T threads. In
> > > >> summary the system contains S * C * T threads.
> > > >> As you said all threads inside a core share the MTRR setting.
> > > >> Do all cores inside a socket share the MTRR setting?
> > > >> Do all sockets share the MTRR setting?
> > > >>
> > > >> If one of the answer of above questions is "no", how can we
> > > >> configure the PCD?
> > > >>
> > > > [Duran, Leo]
> > > > Hi Ray,
> > > > The MTTR settings are share by threads within a core (but each
> > > > core has its own, etc.) The PCD would be set in our
> > > > platform-specific code (e.g.,
> > > it can be set at build-time in the .DSC file).
> > > >
> > > > As I mentioned,
> > > > We don't need (Mtrr.Enable=0) to change MTRR settings, so having
> > > > the
> > > PCD to skip (Mtrr.Enable=0) is reasonable for us.
> > > >
> > > > Leo.
> > > >
> > >
> > > If the PCD is false, no thread disables the MTRR before programming it.
> > > Is it safe? Per Intel's SDM, it's not.
> > >
> > > Maybe it works in AMD's case. But I still suggest we change the
> > > caller, which is more natural.
> > > At least I'd like to see how potential-ugly the change can be.
> > > We can then discuss how to make the ugly change better looking.
> > >
> >
> > Hi Ray,
> > Please pardon the late reply.
> > The main problem with changes to "caller" code is that dependencies
> > are SoC-specific, so the detection code would not scale over time.
> > Again, the proposed PCD does not alter existing flow (so existing code
> > will continue to work as-is), and would give us a lever we can use in
> > platform- specific code  (without requiring surgery in EDK2 "caller" code).
> >
> BTW,
> If you're concerned that someone may inadvertently set the PCD in their
> platform, I can ensure the PCD only applies on AMD (similar to changes I
> introduced in the APIC library).
> For example, something like this:
>   //
>   // Disable MTRRs
>   //
>   if (!StandardSignatureIsAuthenticAMD () || !PcdGetBool
> (PcdSkipDisableMtrrsOnPreMtrrChangeOnAmd)) {
>     DefType.Uint64 = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
>     DefType.Bits.E = 0;
>     AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);  }
> 
> Please let me know if that's better, and will submit an updated patch.
> 
> > Thanks,
> > Leo.
> >
> > > >>>
> > > >>> - From my side, if it works for you, it works for me. (The
> > > >>> general trend has been to avoid adding more PCDs to the "core"
> > > >>> package DEC files, but I'm 100% neutral on that.)
> > > >>>
> > > >>> Laszlo
> > > >>>
> > > >>
> > > >> Laszlo,
> > > >> Thanks for pointing out the general trend. Yes less PCDs are very
> > > welcomed.
> > > >> To me, PCD is no different from protocol. And even worse, because
> > > >> it's very easily to be over-used.
> > > >> But I am not sure whether a PCD has to be introduced for this issue.
> > > >> Maybe even we choose to fix the caller, the PCD is still needed.
> > > >> I am not sure.
> > > >>
> > > >> --
> > > >> Thanks,
> > > >> Ray
> > > > _______________________________________________
> > > > edk2-devel mailing list
> > > > edk2-devel@lists.01.org
> > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > > Ray
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel