On 11/22/23 18:06, Laszlo Ersek wrote:
> On 11/22/23 18:03, Laszlo Ersek wrote:
>> On 11/21/23 08:39, Yuanhao Xie wrote:
>>> Detect and apply Microcode on BSP, store BSP's MTRR setting only when
>>> CpuCount > 1.
>>>
>>> The purpose of this patch is to enhance the review process.
>>> The separation in this patch is aimed at facilitating a more
>>> straightforward review, with the ultimate goal of eliminating the
>>> microcode loading functionality for the second time Mp initialization
>>>
>>> Cc: Ray Ni <ray...@intel.com>
>>> Cc: Eric Dong <eric.d...@intel.com>
>>> Cc: Rahul Kumar <rahul1.ku...@intel.com>
>>> Cc: Tom Lendacky <thomas.lenda...@amd.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Signed-off-by: Yuanhao Xie <yuanhao....@intel.com>
>>> ---
>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 +++++++++---------
>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> index e8ffb99874..bb5d4188f0 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> @@ -2236,19 +2236,19 @@ MpInitLibInitialize (
>>> ShadowMicrocodeUpdatePatch (CpuMpData);
>>> }
>>>
>>> - //
>>> - // Detect and apply Microcode on BSP
>>> - //
>>> - MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
>>> - //
>>> - // Store BSP's MTRR setting
>>> - //
>>> - MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
>>> -
>>> //
>>> // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
>>> //
>>> if (CpuMpData->CpuCount > 1) {
>>> + //
>>> + // Detect and apply Microcode on BSP
>>> + //
>>> + MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
>>> + //
>>> + // Store BSP's MTRR setting
>>> + //
>>> + MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
>>> +
>>> if (MpHandOff != NULL) {
>>> //
>>> // Only needs to use this flag for DXE phase to update the wake up
>>
>> I can't very well gauge the effect that this patch has on
>> MicrocodeDetect(). MicrocodeDetect() is a function that is not obvious
>> to me. However, microcode detection is disabled on OVMF, both via HOB
>> and via PCD (zero region size). So, I have nothing against *restricting*
>> microcode detection "further".
>>
>> Regarding MtrrTable: it seems that this field is only consumed in
>> ApMtrrSync() and ApInitializeSync(). I suppose those functions never run
>> when CpuMpData->CpuCount is 1.
[*]
> Thus restricting MtrrGetAllMtrrs() should
>> be fine as well.
>
> sorry, small mistake: when writing this, I was already looking at the
> final state of the tree. Right after this patch is applied, only
> ApInitializeSync() exists!
>
> But that fact only makes my argument stronger -- so my R-b for this
> patch stands.
My assumption at [*] is correct, in fact; ApInitializeSync() is only
called from the context updated in patch#1! And after patch#3, which
introduces ApMtrrSync(), the same applies to ApMtrrSync() too.
So all is fine.
Thanks
Laszlo
>
>>
>> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>>
>
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111620): https://edk2.groups.io/g/devel/message/111620
Mute This Topic: https://groups.io/mt/102724547/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
