[edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Ni, Ray
ray.ni at intel.com
Wed Feb 24 01:49:41 UTC 2021
Yes! It’s a bug in CpuDxe. I agree with your proposed fix.
3233 – CpuDxe may allocate above 4GB memory for GDT (tianocore.org)<https://bugzilla.tianocore.org/show_bug.cgi?id=3233> was submitted to capture the potential bug.
From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 1:44 PM
To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Ma, Maurice <maurice.ma at intel.com>; Patrick Rudolph <patrick.rudolph at 9elements.com>; Dong, Guo <guo.dong at intel.com>; Dong, Eric <eric.dong at intel.com>
Cc: You, Benjamin <benjamin.you at intel.com>
Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Ray,
BSP's GDT table is setup in CpuDxe and then MpInitLib re-uses BSP's GDT table for APs.
1, UefiCpuPkg\CpuDxe:
gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
.....
gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
2, UefiCpuPkg\Library\MpInitLib\MpLib.c:
//
// Get the BSP's data of GDT and IDT
//
AsmReadGdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->GdtrProfile);
AsmReadIdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->IdtrProfile);
3, UefiCpuPkg\Library\MpInitLib\X64\MpFuncs.nasm:
mov si, GdtrLocation
o32 lgdt [cs:si]
mov si, IdtrLocation
o32 lidt [cs:si]
In CpuDxe, Typecasting gdt to UINT32 is one assumption that GDT table is under 4G space. Thus, we cannot use AllocateRuntimePool (), instead we should use AllocatePages() to make sure GDT table under 4GB space.
gdt = 0xFFFFFFFF;
Status = gBS->AllocatePages (
AllocateMaxAddress,
EfiRuntimeServicesData,
EFI_SIZE_TO_PAGES (sizeof (GdtTemplate) + 8),
&gdt
);
Thanks,
Jeff
From: Ni, Ray<mailto:ray.ni at intel.com>
Date: 2021-02-23 13:21
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; fanjianfeng at byosoft.com.cn<mailto:fanjianfeng at byosoft.com.cn>; Ma, Maurice<mailto:maurice.ma at intel.com>; Patrick Rudolph<mailto:patrick.rudolph at 9elements.com>; Dong, Guo<mailto:guo.dong at intel.com>; Dong, Eric<mailto:eric.dong at intel.com>
CC: You, Benjamin<mailto:benjamin.you at intel.com>
Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
“But we could allocate room under 4G for GDT table directly in CpuDxe.”
The GDT pre-allocated is re-used by AP. Why do you suggest CpuDxe allocate GDT?
Thanks,
Ray
From: devel at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 11:43 AM
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Ma, Maurice <maurice.ma at intel.com<mailto:maurice.ma at intel.com>>; Patrick Rudolph <patrick.rudolph at 9elements.com<mailto:patrick.rudolph at 9elements.com>>; Dong, Guo <guo.dong at intel.com<mailto:guo.dong at intel.com>>; Dong, Eric <eric.dong at intel.com<mailto:eric.dong at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; You, Benjamin <benjamin.you at intel.com<mailto:benjamin.you at intel.com>>
Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Ray,
Yes. You are right. Acutally, x64 IDT table cannot work correctly on protected mode. :-)
But for GDT location, I agree it should be located under 4G space to support AP mode changing. But we could allocate room under 4G for GDT table directly in CpuDxe.
Thanks,
Jeff
From: Ni, Ray<mailto:ray.ni at intel.com>
Date: 2021-02-23 10:52
To: fanjianfeng at byosoft.com.cn<mailto:fanjianfeng at byosoft.com.cn>; devel<mailto:devel at edk2.groups.io>; Ma, Maurice<mailto:maurice.ma at intel.com>; Patrick Rudolph<mailto:patrick.rudolph at 9elements.com>; Dong, Guo<mailto:guo.dong at intel.com>; Dong, Eric<mailto:eric.dong at intel.com>
CC: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; You, Benjamin<mailto:benjamin.you at intel.com>
Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Jeff,
You are right that BSP’s GDT and IDT tables are under 4G memory.
It’s because when AP wakes up, it needs the GDT for entering protected mode. AP cannot access above 4G memory without entering to long mode.
I do agree that the 64bit IDT is not proper for AP when entering protected mode. As long as there is no exception in the short time frame (load 64bit IDT, before entering long mode), it’s still ok.
Thanks,
Ray
From: fanjianfeng at byosoft.com.cn<mailto:fanjianfeng at byosoft.com.cn> <fanjianfeng at byosoft.com.cn<mailto:fanjianfeng at byosoft.com.cn>>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel at edk2.groups.io<mailto:devel at edk2.groups.io>>; Ma, Maurice <maurice.ma at intel.com<mailto:maurice.ma at intel.com>>; Patrick Rudolph <patrick.rudolph at 9elements.com<mailto:patrick.rudolph at 9elements.com>>; Dong, Guo <guo.dong at intel.com<mailto:guo.dong at intel.com>>; Dong, Eric <eric.dong at intel.com<mailto:eric.dong at intel.com>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; You, Benjamin <benjamin.you at intel.com<mailto:benjamin.you at intel.com>>
Subject: Re: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
we will save the current BSP's GDT and IDT for APs at first time APs are waken by BSP as below. APs will start from real mode to protected mode and then to long mode. During protected mode, BSP's GDT/IDT table are working on APs.
In UefiCpuPkg\Library\MpInitLib\MpLib.c,
//
// Get the BSP's data of GDT and IDT
//
AsmReadGdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->GdtrProfile);
AsmReadIdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->IdtrProfile);
It seems to be one bug we have assumption on GDT table and IDT table located under 4G memory space.
Could Ray&Eric help me to confirm it?
Jeff
From: Ma, Maurice<mailto:maurice.ma at intel.com>
Date: 2021-02-23 00:49
To: Patrick Rudolph<mailto:patrick.rudolph at 9elements.com>; Dong, Guo<mailto:guo.dong at intel.com>; Dong, Eric<mailto:eric.dong at intel.com>; Ni, Ray<mailto:ray.ni at intel.com>
CC: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; You, Benjamin<mailto:benjamin.you at intel.com>
Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Hi, Ray and Eric,
Is there any reason why the GDT base was typecast to UINT32 in CpuDxe driver ?
In x64 long mode, the GDT base is actually 64bit. Typecasting will zero out the high 32bit address.
To me the correct code seems to be something like:
gdtPtr.Base = (UINTN)(VOID*) gdt;
Thanks
Maurice
> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph at 9elements.com<mailto:patrick.rudolph at 9elements.com>>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong at intel.com<mailto:guo.dong at intel.com>>
> Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Ma, Maurice <maurice.ma at intel.com<mailto:maurice.ma at intel.com>>; You,
> Benjamin <benjamin.you at intel.com<mailto:benjamin.you at intel.com>>
> Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry:
> Remove 4GB memory WA
>
> Hi Guo,
> I tested on 078400ee15e7b250e4dfafd840c2e0c19835e16b and run it in
> QEMU.
> The problem seems to be here, as gdt is allocated > 4GiB:
> gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
>
> Regards,
> Patrick
>
> On Mon, Feb 22, 2021 at 3:59 PM Dong, Guo <guo.dong at intel.com<mailto:guo.dong at intel.com>> wrote:
> >
> >
> > Hi Patrick,
> > Please make sure you are using latest master when testing this patch.
> > That issue should be fix be this patch:
> > UefiCpuPkg/CpuDxe: Fix boot error (commit:
> > ebfe2d3eb5ac7fd92d74011edb31303a181920c7)
> > And there is similar fix in another place as below:
> > UefiCpuPkg/MpInitLib: Fix a hang in above 4GB case (commit:
> > edd74ad3ad79b855f76d9cf60a96c405cb3e863b)
> >
> > Thanks,
> > Guo
> >
> > > -----Original Message-----
> > > From: devel at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Ma, Maurice <maurice.ma at intel.com<mailto:maurice.ma at intel.com>>
> > > Cc: Dong, Guo <guo.dong at intel.com<mailto:guo.dong at intel.com>>; You, Benjamin
> > > <benjamin.you at intel.com<mailto:benjamin.you at intel.com>>
> > > Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry:
> > > Remove 4GB memory WA
> > >
> > > This patch breaks booting on master.
> > > In CpuDxe.efi / InitGlobalDescriptorTable as the GDT pointer is
> > > casted to 32bits.
> > >
> > > Regards,
> > > Patrick
> > >
> > > On Fri, Feb 19, 2021 at 3:12 AM Ma, Maurice <maurice.ma at intel.com<mailto:maurice.ma at intel.com>>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma at intel.com<mailto:maurice.ma at intel.com>>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong at intel.com<mailto:guo.dong at intel.com>>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>
> > > > > Cc: Ma, Maurice <maurice.ma at intel.com<mailto:maurice.ma at intel.com>>; You, Benjamin
> > > > > <benjamin.you at intel.com<mailto:benjamin.you at intel.com>>
> > > > > Subject: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry:
> > > > > Remove 4GB memory WA
> > > > >
> > > > > Previous it would hang in CpuDxe if DXE drivers are dispatched above
> 4GB.
> > > > > Now remove the work around since the fixed in CpuDxe are merged.
> > > > >
> > > > > Signed-off-by: Guo Dong <guo.dong at intel.com<mailto:guo.dong at intel.com>>
> > > > > ---
> > > > > UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c | 5 -----
> > > > > 1 file changed, 5 deletions(-)
> > > > >
> > > > > diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > > > b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > > > index 805f5448d9..c403b0a80a 100644
> > > > > --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > > > +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > > > @@ -40,11 +40,6 @@ MemInfoCallback (
> > > > > EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> > > > > EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;
> > > > >
> > > > > - if (Base >= BASE_4GB ) {
> > > > > - // Remove tested attribute to avoid DXE core to dispatch driver to
> > > > > memory above 4GB
> > > > > - Attribue &= ~EFI_RESOURCE_ATTRIBUTE_TESTED;
> > > > > - }
> > > > > -
> > > > > BuildResourceDescriptorHob (Type, Attribue,
> > > > > (EFI_PHYSICAL_ADDRESS)Base, Size);
> > > > > DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx,
> > > > > type = 0x%x\n", Base, Size, Type));
> > > > >
> > > > > --
> > > > > 2.16.2.windows.1
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72135): https://edk2.groups.io/g/devel/message/72135
Mute This Topic: https://groups.io/mt/80647875/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210224/0cd068f5/attachment.htm>
More information about the edk2-devel-archive
mailing list