[PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[Ruiyu Ni]

Today's PiSmmIpl implementation initially sets SMRAM to WB to speed
up the SMM core/modules loading before SMM CPU driver runs.
When SMM CPU driver runs, PiSmmIpl resets the SMRAM to UC. It's done
in SmmIplDxeDispatchEventNotify(). COMM_BUFFER_SMM_DISPATCH_RESTART
is returned from SMM core that SMM CPU driver is just dispatched.

Since now the SMRR is widely used to control the SMRAM cache setting.
It's not needed to reset the SMRAM to UC anymore.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index f8cbe1704b..dc0d9a70b0 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -672,19 +672,6 @@ SmmIplDxeDispatchEventNotify (
       return;
     }
 
-    //
-    // Attempt to reset SMRAM cacheability to UC
-    // Assume CPU AP is available at this time
-    //
-    Status = gDS->SetMemorySpaceAttributes(
-                    mSmramCacheBase,
-                    mSmramCacheSize,
-                    EFI_MEMORY_UC
-                    );
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_WARN, "SMM IPL failed to reset SMRAM window to EFI_MEMORY_UC\n"));
-    }
-
     //
     // Close all SMRAM ranges to protect SMRAM
     //
-- 
2.16.1.windows.1

Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[Laszlo Ersek]

On 10/22/18 11:03, Ruiyu Ni wrote:
> Today's PiSmmIpl implementation initially sets SMRAM to WB to speed
> up the SMM core/modules loading before SMM CPU driver runs.
> When SMM CPU driver runs, PiSmmIpl resets the SMRAM to UC. It's done
> in SmmIplDxeDispatchEventNotify(). COMM_BUFFER_SMM_DISPATCH_RESTART
> is returned from SMM core that SMM CPU driver is just dispatched.
> 
> Since now the SMRR is widely used to control the SMRAM cache setting.
> It's not needed to reset the SMRAM to UC anymore.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index f8cbe1704b..dc0d9a70b0 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -672,19 +672,6 @@ SmmIplDxeDispatchEventNotify (
>        return;
>      }
>  
> -    //
> -    // Attempt to reset SMRAM cacheability to UC
> -    // Assume CPU AP is available at this time
> -    //
> -    Status = gDS->SetMemorySpaceAttributes(
> -                    mSmramCacheBase,
> -                    mSmramCacheSize,
> -                    EFI_MEMORY_UC
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_WARN, "SMM IPL failed to reset SMRAM window to EFI_MEMORY_UC\n"));
> -    }
> -
>      //
>      // Close all SMRAM ranges to protect SMRAM
>      //
> 

I vaguely remember this code from commit b07ea4c198a4 ("MdeModulePkg:
SmmIplEntry(): don't suppress SMM core startup failure", 2015-05-12). So
here I'm asking just out of curiosity -- because SMRR is not necessary
to handle on QEMU/KVM.

I assume that the original UC setting was part of the SMRAM protection
that starts at the next line -- the comment is visible in the context,
saying "Close all SMRAM ranges to protect SMRAM". The commit message
suggests that explicit SMRR management has now taken that role.


(1) Can you add more details to the commit message where that explicit
management happens? I.e., what modules call what library interfaces? I
believe the relevant APIs are from SmmCpuFeaturesLib.


(2) Also, can you elaborate on "widely"? Does that mean

  in most edk2 modules for which SMRAM caching is relevant

or

  in most edk2-based platform firmware that is shipped in production

?


(3) You forgot to mention the bugzilla in the commit message:

  https://bugzilla.tianocore.org/show_bug.cgi?id=1268


(4) From commit b07ea4c198a4 that I mention above, it seems that we have
another instance of this UC setting in the SMM IPL runtime DXE driver;
namely in the SmmIplEntry() function. Should we remove that too?

Now, I understand that the situation at that site is different, because
in that case, we are resetting the cacheability after *failure* to
launch the SMM Core, so there may have been no chance for any module to
manage SMRR. And so we have to roll back our own initial WB setting. Do
I understand that right?


(5) I like that this patch removes a "CPU AP" comment, because the
comment is misleading. In this context, "CPU AP" means "CPU
Architectural Protocol" (which underlies
gDS->SetMemorySpaceAttributes()), and not "Application Processor". It's
good that the patch removes such a confusing comment.

However, other potential abuses of the shorthand "CPU AP" remain in the
code. Would you consider submitting a separate patch that replaces all
remaining instances of "CPU AP" with "CPU Arch Protocol", in
"MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c"?


... I originally intended to test this patch at once, but given my
question (4), you might want to modify the code as well, not just the
commit message, and so I figured I'd postpone the testing until your answer.

Thanks!
Laszlo

Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[Lohr, Paul A]

Hello,

Code to remove SMRAM = UC (line 650-ish) looks good.  I would suggest adding some debug comments in the area it was removed.  Thanks.

Per #4, I also "think" the second SMRAM = UC should be removed, in addition to the SMRAM = WB in the same area. But this is under investigation now.   Expect an update in 24-48 hours.

Per #5, I too do not like the "CPU AP" comment, as I initially thought it referred to Application Processor  😊  Yes, please clean that AP comment up!


Paul A. Lohr – Server Firmware Enabling
512.239.9073 (cell)
512.794.5044 (work)

-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Laszlo Ersek
Sent: Monday, October 22, 2018 9:30 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

On 10/22/18 11:03, Ruiyu Ni wrote:
> Today's PiSmmIpl implementation initially sets SMRAM to WB to speed up 
> the SMM core/modules loading before SMM CPU driver runs.
> When SMM CPU driver runs, PiSmmIpl resets the SMRAM to UC. It's done 
> in SmmIplDxeDispatchEventNotify(). COMM_BUFFER_SMM_DISPATCH_RESTART is 
> returned from SMM core that SMM CPU driver is just dispatched.
> 
> Since now the SMRR is widely used to control the SMRAM cache setting.
> It's not needed to reset the SMRAM to UC anymore.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c 
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index f8cbe1704b..dc0d9a70b0 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -672,19 +672,6 @@ SmmIplDxeDispatchEventNotify (
>        return;
>      }
>  
> -    //
> -    // Attempt to reset SMRAM cacheability to UC
> -    // Assume CPU AP is available at this time
> -    //
> -    Status = gDS->SetMemorySpaceAttributes(
> -                    mSmramCacheBase,
> -                    mSmramCacheSize,
> -                    EFI_MEMORY_UC
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_WARN, "SMM IPL failed to reset SMRAM window to EFI_MEMORY_UC\n"));
> -    }
> -
>      //
>      // Close all SMRAM ranges to protect SMRAM
>      //
> 

I vaguely remember this code from commit b07ea4c198a4 ("MdeModulePkg:
SmmIplEntry(): don't suppress SMM core startup failure", 2015-05-12). So here I'm asking just out of curiosity -- because SMRR is not necessary to handle on QEMU/KVM.

I assume that the original UC setting was part of the SMRAM protection that starts at the next line -- the comment is visible in the context, saying "Close all SMRAM ranges to protect SMRAM". The commit message suggests that explicit SMRR management has now taken that role.


(1) Can you add more details to the commit message where that explicit management happens? I.e., what modules call what library interfaces? I believe the relevant APIs are from SmmCpuFeaturesLib.


(2) Also, can you elaborate on "widely"? Does that mean

  in most edk2 modules for which SMRAM caching is relevant

or

  in most edk2-based platform firmware that is shipped in production

?


(3) You forgot to mention the bugzilla in the commit message:

  https://bugzilla.tianocore.org/show_bug.cgi?id=1268


(4) From commit b07ea4c198a4 that I mention above, it seems that we have another instance of this UC setting in the SMM IPL runtime DXE driver; namely in the SmmIplEntry() function. Should we remove that too?

Now, I understand that the situation at that site is different, because in that case, we are resetting the cacheability after *failure* to launch the SMM Core, so there may have been no chance for any module to manage SMRR. And so we have to roll back our own initial WB setting. Do I understand that right?


(5) I like that this patch removes a "CPU AP" comment, because the comment is misleading. In this context, "CPU AP" means "CPU Architectural Protocol" (which underlies
gDS->SetMemorySpaceAttributes()), and not "Application Processor". It's
good that the patch removes such a confusing comment.

However, other potential abuses of the shorthand "CPU AP" remain in the code. Would you consider submitting a separate patch that replaces all remaining instances of "CPU AP" with "CPU Arch Protocol", in "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c"?


... I originally intended to test this patch at once, but given my question (4), you might want to modify the code as well, not just the commit message, and so I figured I'd postpone the testing until your answer.

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

Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[Ni, Ruiyu]

On 10/23/2018 11:12 AM, Lohr, Paul A wrote:
> Hello,
> 
> Code to remove SMRAM = UC (line 650-ish) looks good.  I would suggest adding some debug comments in the area it was removed.  Thanks.

debug message or comments?
I guess you'd like to have some comments to say "SMRR is enabled by CPU 
SMM driver so no need to reset the SMRAM to UC in MTRR". Correct?

> 
> Per #4, I also "think" the second SMRAM = UC should be removed, in addition to the SMRAM = WB in the same area. But this is under investigation now.   Expect an update in 24-48 hours.

I did some search in close source platform code and found out that the 
SMRAM is set to WB by default.
So it turns out setting SMRAM to WB is unnecessary here. But this 
introduces a platform assumption. If platform doesn't set SMRAM to WB, 
removing the WB set here will cause system boot performance downgrade.

Above is my understanding.

Please post update here once you finish the investigation.

> 
> Per #5, I too do not like the "CPU AP" comment, as I initially thought it referred to Application Processor  😊  Yes, please clean that AP comment up!

Sure I will do that.

> 
> 
> Paul A. Lohr – Server Firmware Enabling
> 512.239.9073 (cell)
> 512.794.5044 (work)
> 

Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[Laszlo Ersek]

On 10/23/18 11:36, Ni, Ruiyu wrote:
> On 10/23/2018 11:12 AM, Lohr, Paul A wrote:
>> Hello,
>>
>> Code to remove SMRAM = UC (line 650-ish) looks good.  I would suggest
>> adding some debug comments in the area it was removed.  Thanks.
> 
> debug message or comments?
> I guess you'd like to have some comments to say "SMRR is enabled by CPU
> SMM driver so no need to reset the SMRAM to UC in MTRR". Correct?

Such a comment sounds great to me, just please include: "by calling
SmmCpuFeaturesInitializeProcessor from SmmCpuFeaturesLib".

[...]

Thanks!
Laszlo

Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[Lohr, Paul A]

Hello - update on this issue:

1) SMRAM = UC in SmmIplDxeDispatchEventNotify() around line 650 can be removed, yes.  Code never modified anything, so there is nothing to "undo."
2) SMRAM = WB \ SMRAM = UC in SmmIplEntry() around lines 1600-1675 is still being considered. 

Please separate these check-ins and begin checking in #1.  We can continue the discussion on #2.

Paul A. Lohr – Server Firmware Enabling
512.239.9073 (cell)
512.794.5044 (work)

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Tuesday, October 23, 2018 4:44 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; Lohr, Paul A <paul.a.lohr@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

On 10/23/18 11:36, Ni, Ruiyu wrote:
> On 10/23/2018 11:12 AM, Lohr, Paul A wrote:
>> Hello,
>>
>> Code to remove SMRAM = UC (line 650-ish) looks good.  I would suggest 
>> adding some debug comments in the area it was removed.  Thanks.
> 
> debug message or comments?
> I guess you'd like to have some comments to say "SMRR is enabled by 
> CPU SMM driver so no need to reset the SMRAM to UC in MTRR". Correct?

Such a comment sounds great to me, just please include: "by calling SmmCpuFeaturesInitializeProcessor from SmmCpuFeaturesLib".

[...]

Thanks!
Laszlo

Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

[Ni, Ruiyu]

On 10/22/2018 10:30 PM, Laszlo Ersek wrote:
> On 10/22/18 11:03, Ruiyu Ni wrote:
>> Today's PiSmmIpl implementation initially sets SMRAM to WB to speed
>> up the SMM core/modules loading before SMM CPU driver runs.
>> When SMM CPU driver runs, PiSmmIpl resets the SMRAM to UC. It's done
>> in SmmIplDxeDispatchEventNotify(). COMM_BUFFER_SMM_DISPATCH_RESTART
>> is returned from SMM core that SMM CPU driver is just dispatched.
>>
>> Since now the SMRR is widely used to control the SMRAM cache setting.
>> It's not needed to reset the SMRAM to UC anymore.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Michael Kinney <michael.d.kinney@intel.com>
>> ---
>>   MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 -------------
>>   1 file changed, 13 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> index f8cbe1704b..dc0d9a70b0 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> @@ -672,19 +672,6 @@ SmmIplDxeDispatchEventNotify (
>>         return;
>>       }
>>   
>> -    //
>> -    // Attempt to reset SMRAM cacheability to UC
>> -    // Assume CPU AP is available at this time
>> -    //
>> -    Status = gDS->SetMemorySpaceAttributes(
>> -                    mSmramCacheBase,
>> -                    mSmramCacheSize,
>> -                    EFI_MEMORY_UC
>> -                    );
>> -    if (EFI_ERROR (Status)) {
>> -      DEBUG ((DEBUG_WARN, "SMM IPL failed to reset SMRAM window to EFI_MEMORY_UC\n"));
>> -    }
>> -
>>       //
>>       // Close all SMRAM ranges to protect SMRAM
>>       //
>>
> 
> I vaguely remember this code from commit b07ea4c198a4 ("MdeModulePkg:
> SmmIplEntry(): don't suppress SMM core startup failure", 2015-05-12). So
> here I'm asking just out of curiosity -- because SMRR is not necessary
> to handle on QEMU/KVM.
> 
> I assume that the original UC setting was part of the SMRAM protection
> that starts at the next line -- the comment is visible in the context,
> saying "Close all SMRAM ranges to protect SMRAM". The commit message
> suggests that explicit SMRR management has now taken that role.
> 
> 
> (1) Can you add more details to the commit message where that explicit
> management happens? I.e., what modules call what library interfaces? I
> believe the relevant APIs are from SmmCpuFeaturesLib.

Yes. SmmCpuFeaturesInitializeProcessor() from SmmCpuFeaturesLib enables 
SMRR

> 
> 
> (2) Also, can you elaborate on "widely"? Does that mean
> 
>    in most edk2 modules for which SMRAM caching is relevant
> 
> or
> 
>    in most edk2-based platform firmware that is shipped in production
> 
> ?

Most (ALL maybe) edk2-based platform firmware that is shipped in production.

> 
> 
> (3) You forgot to mention the bugzilla in the commit message:
> 
>    https://bugzilla.tianocore.org/show_bug.cgi?id=1268
> 
> 
> (4) From commit b07ea4c198a4 that I mention above, it seems that we have
> another instance of this UC setting in the SMM IPL runtime DXE driver;
> namely in the SmmIplEntry() function. Should we remove that too?
> 
> Now, I understand that the situation at that site is different, because
> in that case, we are resetting the cacheability after *failure* to
> launch the SMM Core, so there may have been no chance for any module to
> manage SMRR. And so we have to roll back our own initial WB setting. Do
> I understand that right?

Yes. I don't want to change that piece of code.

> 
> (5) I like that this patch removes a "CPU AP" comment, because the
> comment is misleading. In this context, "CPU AP" means "CPU
> Architectural Protocol" (which underlies
> gDS->SetMemorySpaceAttributes()), and not "Application Processor". It's
> good that the patch removes such a confusing comment.
> 
> However, other potential abuses of the shorthand "CPU AP" remain in the
> code. Would you consider submitting a separate patch that replaces all
> remaining instances of "CPU AP" with "CPU Arch Protocol", in
> "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c"?

Sure the "AP" also confused me in the beginning. I will post a separate 
patch to change it.

> 
> 
> ... I originally intended to test this patch at once, but given my
> question (4), you might want to modify the code as well, not just the
> commit message, and so I figured I'd postpone the testing until your answer.

Let me check Paul's mail then decide what to do next.

> 
> Thanks!
> Laszlo
> 


-- 
Thanks,
Ray