Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Fan, Jeff 
Brian,

Good point on BSP bit in MSR_IA32_APIC_BASE register.  But some processor(for 
example, Quark) does not support this MSR.
//
// CPUs with a FamilyId of 0x04 or 0x05 do not support the 
// Local APIC Base Address MSR
//

I think the generic solution is still to remove MTRRs display from 
MtrrSetAllMtrrs(). 

Thanks!
Jeff

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Brian J. 
Johnson
Sent: Thursday, July 14, 2016 2:36 AM
To: Laszlo Ersek; Kinney, Michael D; Fan, Jeff; edk2-de...@ml01.01.org
Cc: Tian, Feng
Subject: Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from 
MtrrSetAllMtrrs()

On 07/13/2016 11:19 AM, Laszlo Ersek wrote:
> On 07/13/16 17:46, Kinney, Michael D wrote:
>> 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.
>
> I thought of that, but it would (I believe) introduce a dependency on 
> the MP protocol / PPI (for the WhoAmI() member), and that seems too 
> much for a base library like MtrrLib.
>
> I guess two new library instances could be created: one for PEIM and 
> another for DXE phase client modules. The MtrrLib functions could 
> check if the MP PPI / protocol were available. Negative answer: assume 
> we are running on the BSP, and debug-log away. Positive answer: call 
> WhoAmI(), and act accordingly.
>
> This would be usable -- I think -- for general thread-safety concerns, 
> not just debugging, but it's quite a bit of churn.
>
> ... Although, I'm not even fully sure whether (WhoAmI() == 0) can be 
> taken as "BSP". Given SwitchBSP() especially.
>
> Do you have better ideas for internally determining whether the 
> processor is an AP or a BSP? Something with the APIC IDs perhaps?
>
> Thanks
> Laszlo
>

There's the BSP flag (bit 8) in the MSR_IA32_APIC_BASE register.  It doesn't 
seem to be exposed nicely by BaseXApicX2ApicLib, but it's easy enough to read 
directly.
-- 

Brian J. Johnson



   My statements are my own, are not authorized by SGI, and do not
   necessarily represent SGI's positions.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Brian J. Johnson 

On 07/13/2016 11:19 AM, Laszlo Ersek wrote:

On 07/13/16 17:46, Kinney, Michael D wrote:

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.


I thought of that, but it would (I believe) introduce a dependency on
the MP protocol / PPI (for the WhoAmI() member), and that seems too much
for a base library like MtrrLib.

I guess two new library instances could be created: one for PEIM and
another for DXE phase client modules. The MtrrLib functions could check
if the MP PPI / protocol were available. Negative answer: assume we are
running on the BSP, and debug-log away. Positive answer: call WhoAmI(),
and act accordingly.

This would be usable -- I think -- for general thread-safety concerns,
not just debugging, but it's quite a bit of churn.

... Although, I'm not even fully sure whether (WhoAmI() == 0) can be
taken as "BSP". Given SwitchBSP() especially.

Do you have better ideas for internally determining whether the
processor is an AP or a BSP? Something with the APIC IDs perhaps?

Thanks
Laszlo



There's the BSP flag (bit 8) in the MSR_IA32_APIC_BASE register.  It 
doesn't seem to be exposed nicely by BaseXApicX2ApicLib, but it's easy 
enough to read directly.

--

Brian J. Johnson



  My statements are my own, are not authorized by SGI, and do not
  necessarily represent SGI’s positions.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Laszlo Ersek 
On 07/13/16 18:13, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> I missed this other part of this thread.  I agree with the direction here.
> 
> Reviewed-by: Michael Kinney <michael.d.kin...@intel.com>
> 
> If anyone notices a loss of messages from BSP when DEBUG_CACHE is enabled, 
> then the correct fix is to add an explicit call to MtrrDebugPrintAllMtrrs().

Yeah, that will work for me, certainly.

Thanks!
Laszlo

>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Wednesday, July 13, 2016 5:00 AM
>> To: Fan, Jeff <jeff@intel.com>; edk2-de...@ml01.01.org
>> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Tian, Feng 
>> <feng.t...@intel.com>
>> Subject: Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from
>> MtrrSetAllMtrrs()
>>
>> On 07/13/16 10:27, Fan, Jeff wrote:
>>> Laszlo,
>>>
>>> Yes. You are correct. MtrrSetAllMtrrs() could be used by BSP to make whole 
>>> MTRR
>> updating.
>>> I agree this patch will be feature drop on MtrrSetAllMtrrs() on MTRR 
>>> setting display.
>>>
>>> I reviewed the example you listed.
>>> - OvmfPkg/PlatformPei/MemDetect.c-- MTRR setting will be displayed 
>>> in
>> MtrrSetMemoryAttribute() invoking after MtrrSetAllMtrrs() invoked.
>>>
>>> - UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c -- The MTRR setting is saved in normal 
>>> boot path
>> and should have been displayed in normal boot already.
>>>
>>> - Vlv2TbltDevicePkg/PlatformInitPei/MemoryPeim.c -- This is the case you 
>>> suggested to
>> invoke MtrrDebugPrintAllMtrrs () explicitly. This is also what I suggested.
>>
>> Makes sense. I can certainly live with the pattern that an explicit
>> debug print is only necessary when no other MtrrLib function prints the
>> settings.
>>
>>> And I agree we need to state some services are not safe for Aps. But I am 
>>> not sure if
>> we need to add this notes in UefiCpuPkg/Include/Library/MtrrLib.h. Because 
>> this is just
>> limitation of MtrrLib instance implementation.
>>> Maybe, we could add this note in UefiCpuPkg/Library/MtrrLib/MtrrLib.c.
>>
>> Good point! Do you want to follow up with a patch that adds the note to
>> the library instance?
>>
>>> How do you think it?
>>
>> From the OvmfPkg point of view, I'm okay with this patch then. I agree
>> that in PlatformPei no debug information is lost after all.
>>
>> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>>
>> Not sure about the other client modules of MtrrLib, but their
>> maintainers are welcome to speak up in this thread :)
>>
>> Thanks
>> Laszlo
>>
>>
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>> Laszlo Ersek
>>> Sent: Wednesday, July 13, 2016 2:21 PM
>>> To: Fan, Jeff; edk2-de...@ml01.01.org
>>> Cc: Kinney, Michael D; Tian, Feng
>>> Subject: Re: [edk2] [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 ();
>>>>
>>>> -  MtrrDebugPrintAllMtrrs ();
>>>> -

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Laszlo Ersek 
On 07/13/16 17:46, Kinney, Michael D wrote:
> 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.

I thought of that, but it would (I believe) introduce a dependency on
the MP protocol / PPI (for the WhoAmI() member), and that seems too much
for a base library like MtrrLib.

I guess two new library instances could be created: one for PEIM and
another for DXE phase client modules. The MtrrLib functions could check
if the MP PPI / protocol were available. Negative answer: assume we are
running on the BSP, and debug-log away. Positive answer: call WhoAmI(),
and act accordingly.

This would be usable -- I think -- for general thread-safety concerns,
not just debugging, but it's quite a bit of churn.

... Although, I'm not even fully sure whether (WhoAmI() == 0) can be
taken as "BSP". Given SwitchBSP() especially.

Do you have better ideas for internally determining whether the
processor is an AP or a BSP? Something with the APIC IDs perhaps?

Thanks
Laszlo

>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Tuesday, July 12, 2016 11:21 PM
>> To: Fan, Jeff ; edk2-de...@ml01.01.org
>> Cc: Tian, Feng ; Kinney, Michael D 
>> 
>> 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 
>>> Cc: Michael Kinney 
>>> Cc: Laszlo Ersek 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Jeff Fan 
>>> ---
>>>  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 ();
>>>
>>> -  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

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Kinney, Michael D 
Hi Laszlo,

I missed this other part of this thread.  I agree with the direction here.

Reviewed-by: Michael Kinney <michael.d.kin...@intel.com>

If anyone notices a loss of messages from BSP when DEBUG_CACHE is enabled, 
then the correct fix is to add an explicit call to MtrrDebugPrintAllMtrrs().

Mike

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, July 13, 2016 5:00 AM
> To: Fan, Jeff <jeff@intel.com>; edk2-de...@ml01.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Tian, Feng 
> <feng.t...@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from
> MtrrSetAllMtrrs()
> 
> On 07/13/16 10:27, Fan, Jeff wrote:
> > Laszlo,
> >
> > Yes. You are correct. MtrrSetAllMtrrs() could be used by BSP to make whole 
> > MTRR
> updating.
> > I agree this patch will be feature drop on MtrrSetAllMtrrs() on MTRR 
> > setting display.
> >
> > I reviewed the example you listed.
> > - OvmfPkg/PlatformPei/MemDetect.c-- MTRR setting will be displayed 
> > in
> MtrrSetMemoryAttribute() invoking after MtrrSetAllMtrrs() invoked.
> >
> > - UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c -- The MTRR setting is saved in normal 
> > boot path
> and should have been displayed in normal boot already.
> >
> > - Vlv2TbltDevicePkg/PlatformInitPei/MemoryPeim.c -- This is the case you 
> > suggested to
> invoke MtrrDebugPrintAllMtrrs () explicitly. This is also what I suggested.
> 
> Makes sense. I can certainly live with the pattern that an explicit
> debug print is only necessary when no other MtrrLib function prints the
> settings.
> 
> > And I agree we need to state some services are not safe for Aps. But I am 
> > not sure if
> we need to add this notes in UefiCpuPkg/Include/Library/MtrrLib.h. Because 
> this is just
> limitation of MtrrLib instance implementation.
> > Maybe, we could add this note in UefiCpuPkg/Library/MtrrLib/MtrrLib.c.
> 
> Good point! Do you want to follow up with a patch that adds the note to
> the library instance?
> 
> > How do you think it?
> 
> From the OvmfPkg point of view, I'm okay with this patch then. I agree
> that in PlatformPei no debug information is lost after all.
> 
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> 
> Not sure about the other client modules of MtrrLib, but their
> maintainers are welcome to speak up in this thread :)
> 
> Thanks
> Laszlo
> 
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> > Laszlo Ersek
> > Sent: Wednesday, July 13, 2016 2:21 PM
> > To: Fan, Jeff; edk2-de...@ml01.01.org
> > Cc: Kinney, Michael D; Tian, Feng
> > Subject: Re: [edk2] [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 ();
> >>
> >> -  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 p

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Kinney, Michael D 
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 ; edk2-de...@ml01.01.org
> Cc: Tian, Feng ; Kinney, Michael D 
> 
> 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 
> > Cc: Michael Kinney 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jeff Fan 
> > ---
> >  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 ();
> >
> > -  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

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Laszlo Ersek 
On 07/13/16 10:27, Fan, Jeff wrote:
> Laszlo,
> 
> Yes. You are correct. MtrrSetAllMtrrs() could be used by BSP to make whole 
> MTRR updating. 
> I agree this patch will be feature drop on MtrrSetAllMtrrs() on MTRR setting 
> display.
> 
> I reviewed the example you listed.
> - OvmfPkg/PlatformPei/MemDetect.c-- MTRR setting will be displayed in 
> MtrrSetMemoryAttribute() invoking after MtrrSetAllMtrrs() invoked.
> 
> - UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c -- The MTRR setting is saved in normal 
> boot path and should have been displayed in normal boot already.
> 
> - Vlv2TbltDevicePkg/PlatformInitPei/MemoryPeim.c -- This is the case you 
> suggested to invoke MtrrDebugPrintAllMtrrs() explicitly. This is also what I 
> suggested.

Makes sense. I can certainly live with the pattern that an explicit
debug print is only necessary when no other MtrrLib function prints the
settings.

> And I agree we need to state some services are not safe for Aps. But I am not 
> sure if we need to add this notes in UefiCpuPkg/Include/Library/MtrrLib.h. 
> Because this is just limitation of MtrrLib instance implementation.
> Maybe, we could add this note in UefiCpuPkg/Library/MtrrLib/MtrrLib.c.

Good point! Do you want to follow up with a patch that adds the note to
the library instance?

> How do you think it?

>From the OvmfPkg point of view, I'm okay with this patch then. I agree
that in PlatformPei no debug information is lost after all.

Reviewed-by: Laszlo Ersek <ler...@redhat.com>

Not sure about the other client modules of MtrrLib, but their
maintainers are welcome to speak up in this thread :)

Thanks
Laszlo



> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Wednesday, July 13, 2016 2:21 PM
> To: Fan, Jeff; edk2-de...@ml01.01.org
> Cc: Kinney, Michael D; Tian, Feng
> Subject: Re: [edk2] [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 ();
>>  
>> -  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
>

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Fan, Jeff 
Laszlo,

Yes. You are correct. MtrrSetAllMtrrs() could be used by BSP to make whole MTRR 
updating. 
I agree this patch will be feature drop on MtrrSetAllMtrrs() on MTRR setting 
display.

I reviewed the example you listed.
- OvmfPkg/PlatformPei/MemDetect.c-- MTRR setting will be displayed in 
MtrrSetMemoryAttribute() invoking after MtrrSetAllMtrrs() invoked.

- UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c -- The MTRR setting is saved in normal boot 
path and should have been displayed in normal boot already.

- Vlv2TbltDevicePkg/PlatformInitPei/MemoryPeim.c -- This is the case you 
suggested to invoke MtrrDebugPrintAllMtrrs() explicitly. This is also what I 
suggested.

And I agree we need to state some services are not safe for Aps. But I am not 
sure if we need to add this notes in UefiCpuPkg/Include/Library/MtrrLib.h. 
Because this is just limitation of MtrrLib instance implementation.
Maybe, we could add this note in UefiCpuPkg/Library/MtrrLib/MtrrLib.c.

How do you think it?

Thanks!
Jeff

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Wednesday, July 13, 2016 2:21 PM
To: Fan, Jeff; edk2-de...@ml01.01.org
Cc: Kinney, Michael D; Tian, Feng
Subject: Re: [edk2] [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 ();
>  
> -  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

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Laszlo Ersek 
On 07/13/16 08:20, Laszlo Ersek wrote:

> - Audit all current uses of MtrrSetAllMtrrs(), and wherever it is
>   obviously called from the BSP, append the following explicit code:
> 
>   DEBUG_CODE (MtrrDebugPrintAllMtrrs());

Sorry, that should be

  DEBUG_CODE (MtrrDebugPrintAllMtrrs ());

(whitespace correction)

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-13 Thread Laszlo Ersek 
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 
> Cc: Michael Kinney 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan 
> ---
>  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 ();
>  
> -  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

[edk2] [Patch] UefiCpuPkg/MtrrLib: Remove MTRRs display from MtrrSetAllMtrrs()

2016-07-12 Thread Jeff Fan 
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 
Cc: Michael Kinney 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan 
---
 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 ();
 
-  MtrrDebugPrintAllMtrrs ();
-
   return MtrrSetting;
 }
 
-- 
2.7.4.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel