Laszlo,

I agree that the DEBUG() messages for this are very valuable to debug 
MTRR cache settings.

Another option is to add logic to detect if the calling CPU is the BSP or
not and only invoke DEBUG() macros if the caller is the BSP.  That would 
not require any changes to the MtrrLib APIs or calling code.

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, July 12, 2016 11:21 PM
> To: Fan, Jeff <jeff....@intel.com>; edk2-de...@ml01.01.org
> Cc: Tian, Feng <feng.t...@intel.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>
> Subject: Re: [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from 
> MtrrSetAllMtrrs()
> 
> On 07/13/16 02:33, Jeff Fan wrote:
> > MtrrSetAllMtrrs() maybe used by APs to sync BSP's MTRR settings. BSP's MTRR
> > setting should be displayed if EFI_D_CACHE flag is set when MTRR updated. In
> > MtrrSetAllMtrrs(), it's not necessary to display MTRR setting again due to 
> > the
> > MTRR settings should be always same among BSP/APs. This updating could avoid
> > APs output MTRR setting at the same time and make display message corrupted.
> >
> > Cc: Feng Tian <feng.t...@intel.com>
> > Cc: Michael Kinney <michael.d.kin...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jeff Fan <jeff....@intel.com>
> > ---
> >  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> > index 6a6bf76..f667a8f 100644
> > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> > @@ -2103,8 +2103,6 @@ MtrrSetAllMtrrs (
> >
> >    PostMtrrChangeEnableCache (&MtrrContext);
> >
> > -  MtrrDebugPrintAllMtrrs ();
> > -
> >    return MtrrSetting;
> >  }
> >
> >
> 
> I think I must have misunderstood your previous email about this -- I
> apologize for that!
> 
> I realize now that you meant that the BSP is expected to set up its
> MTRRs using other APIs *only*. Those APIs would still print the MTRR
> settings (with EFI_D_CACHE) and then it would not be necessary for the
> APs to print the same settings with MtrrSetAllMtrrs().
> 
> Unfortunately, MtrrSetAllMtrrs() is used by the BSP as well, for setting
> up its MTRRs from the scratch. Some examples:
> 
> - OvmfPkg/PlatformPei/MemDetect.c -- setting up the initial MTRRs
> - UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c -- this restores MTRRs on S3, and
>   the BSP also uses MtrrSetAllMtrrs() for that
> - Vlv2TbltDevicePkg/PlatformInitPei/MemoryPeim.c -- I'm not very
>   familiar with this module, but it seems to invoke MtrrSetAllMtrrs()
>   on the BSP too
> 
> So I think restricting MtrrSetAllMtrrs() to APs only would be an
> incompatible change for this library. I apologize again for wasting your
> time by misunderstanding your previous email. :(
> 
> * I propose the following:
> - introduce a new library class interface that does the same as
>   MtrrSetAllMtrrs(), but takes an additional BOOLEAN parameter called
>   ThreadSafe
> - update the "UefiCpuPkg/Include/Library/MtrrLib.h" library class
>   header to state that the library interfaces are unsafe for being
>   called from APs, *except* the new function, when ThreadSafe=TRUE
> - the MtrrSetAllMtrrs() function would be specified to be equivalent to
>   the new function with ThreadSafe=FALSE
> - implement the new function for the single library instance that
>   exists now. The implementation wouldn't be hard -- if ThreadSafe, then
>   omit MtrrDebugPrintAllMtrrs().
> - Audit all current uses of MtrrSetAllMtrrs(), and whenever it is
>   called from an AP, change it to the new library API, with
>   ThreadSafe=TRUE.
> 
> Now I realize you could consider this a lot of work for nothing, so I
> don't "insist" :) Here's an alternative suggestion (should be much easier):
> 
> * I just noticed that MtrrDebugPrintAllMtrrs() is a library class API!
> It's not an internal-only function. That's great; it allows the following:
> - include this patch in the series, as is -- patch #1
> - update the "UefiCpuPkg/Include/Library/MtrrLib.h" library class
>   header to state that the library interfaces are unsafe for being
>   called from APs, *except* MtrrDebugPrintAllMtrrs() -- patch #2
> - Audit all current uses of MtrrSetAllMtrrs(), and wherever it is
>   obviously called from the BSP, append the following explicit code:
> 
>   DEBUG_CODE (MtrrDebugPrintAllMtrrs());
> 
>   In other words, this would move the debug printing from the
>   MtrrSetAllMtrrs() function to the call sites.
> 
> What do you think?
> 
> I don't frequently use EFI_D_CACHE, but I *have* used it before, and it
> is extremely useful. I wouldn't like it to vanish for when
> MtrrDebugPrintAllMtrrs() is called from the BSP. I think the patch is
> good, but we should add the debug prints (like above) to the obvious
> call sites. What do you think?
> 
> Sorry again for not understanding your intent from your previous email.
> 
> Thanks!
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to