* [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
@ 2021-02-15 5:13 Guo Dong
2021-02-19 2:12 ` Ma, Maurice
0 siblings, 1 reply; 18+ messages in thread
From: Guo Dong @ 2021-02-15 5:13 UTC (permalink / raw)
To: devel; +Cc: maurice.ma, benjamin.you
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@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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-15 5:13 [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA Guo Dong
@ 2021-02-19 2:12 ` Ma, Maurice
2021-02-22 14:04 ` Patrick Rudolph
0 siblings, 1 reply; 18+ messages in thread
From: Ma, Maurice @ 2021-02-19 2:12 UTC (permalink / raw)
To: Dong, Guo, devel@edk2.groups.io; +Cc: You, Benjamin
Reviewed-by: Maurice Ma <maurice.ma@intel.com>
Regards
Maurice
> -----Original Message-----
> From: Dong, Guo <guo.dong@intel.com>
> Sent: Sunday, February 14, 2021 21:13
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> <benjamin.you@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@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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-19 2:12 ` Ma, Maurice
@ 2021-02-22 14:04 ` Patrick Rudolph
2021-02-22 14:59 ` Guo Dong
0 siblings, 1 reply; 18+ messages in thread
From: Patrick Rudolph @ 2021-02-22 14:04 UTC (permalink / raw)
To: devel, Ma, Maurice; +Cc: Dong, Guo, You, Benjamin
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@intel.com> wrote:
>
> Reviewed-by: Maurice Ma <maurice.ma@intel.com>
>
> Regards
> Maurice
>
> > -----Original Message-----
> > From: Dong, Guo <guo.dong@intel.com>
> > Sent: Sunday, February 14, 2021 21:13
> > To: devel@edk2.groups.io
> > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > <benjamin.you@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@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
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-22 14:04 ` Patrick Rudolph
@ 2021-02-22 14:59 ` Guo Dong
2021-02-22 15:42 ` Patrick Rudolph
0 siblings, 1 reply; 18+ messages in thread
From: Guo Dong @ 2021-02-22 14:59 UTC (permalink / raw)
To: devel@edk2.groups.io, patrick.rudolph@9elements.com, Ma, Maurice
Cc: You, Benjamin
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Patrick
> Rudolph
> Sent: Monday, February 22, 2021 7:04 AM
> To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> <benjamin.you@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@intel.com> wrote:
> >
> > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> >
> > Regards
> > Maurice
> >
> > > -----Original Message-----
> > > From: Dong, Guo <guo.dong@intel.com>
> > > Sent: Sunday, February 14, 2021 21:13
> > > To: devel@edk2.groups.io
> > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > <benjamin.you@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@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
> >
> >
> >
> >
> >
> >
>
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-22 14:59 ` Guo Dong
@ 2021-02-22 15:42 ` Patrick Rudolph
2021-02-22 16:49 ` Ma, Maurice
0 siblings, 1 reply; 18+ messages in thread
From: Patrick Rudolph @ 2021-02-22 15:42 UTC (permalink / raw)
To: Dong, Guo; +Cc: devel@edk2.groups.io, Ma, Maurice, You, Benjamin
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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Patrick
> > Rudolph
> > Sent: Monday, February 22, 2021 7:04 AM
> > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > <benjamin.you@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@intel.com> wrote:
> > >
> > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > >
> > > Regards
> > > Maurice
> > >
> > > > -----Original Message-----
> > > > From: Dong, Guo <guo.dong@intel.com>
> > > > Sent: Sunday, February 14, 2021 21:13
> > > > To: devel@edk2.groups.io
> > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > <benjamin.you@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@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
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-22 15:42 ` Patrick Rudolph
@ 2021-02-22 16:49 ` Ma, Maurice
2021-02-22 17:10 ` Guo Dong
2021-02-23 0:50 ` fanjianfeng
0 siblings, 2 replies; 18+ messages in thread
From: Ma, Maurice @ 2021-02-22 16:49 UTC (permalink / raw)
To: Patrick Rudolph, Dong, Guo, Dong, Eric, Ni, Ray
Cc: devel@edk2.groups.io, You, Benjamin
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@9elements.com>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com>
> Cc: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>; You,
> Benjamin <benjamin.you@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > > <benjamin.you@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@intel.com>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > > <benjamin.you@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@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-22 16:49 ` Ma, Maurice
@ 2021-02-22 17:10 ` Guo Dong
2021-02-22 18:06 ` Patrick Rudolph
2021-02-23 0:50 ` fanjianfeng
1 sibling, 1 reply; 18+ messages in thread
From: Guo Dong @ 2021-02-22 17:10 UTC (permalink / raw)
To: Ma, Maurice, Patrick Rudolph, Dong, Eric, Ni, Ray
Cc: devel@edk2.groups.io, You, Benjamin
Hi Patrick,
Thank you for the test and root-cause the issues.
Could you try if this patch works with "gdtPtr.Base = (UINTN)(VOID*) gdt;"?
I didn't see this issue when I tested SBL with UEFI payload on QEMU and a real platform.
I guess there is something that could make AllocateRuntimePool to allocate memory below 4GB while CpuDxe is dispatched above 4GB.
Thanks,
Guo
> -----Original Message-----
> From: Ma, Maurice <maurice.ma@intel.com>
> Sent: Monday, February 22, 2021 9:50 AM
> To: Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo
> <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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@9elements.com>
> > Sent: Monday, February 22, 2021 7:43
> > To: Dong, Guo <guo.dong@intel.com>
> > Cc: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>; You,
> > Benjamin <benjamin.you@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Patrick Rudolph
> > > > Sent: Monday, February 22, 2021 7:04 AM
> > > > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > > > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > > > <benjamin.you@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@intel.com>
> > wrote:
> > > > >
> > > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > > > >
> > > > > Regards
> > > > > Maurice
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Dong, Guo <guo.dong@intel.com>
> > > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > > > <benjamin.you@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@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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-22 17:10 ` Guo Dong
@ 2021-02-22 18:06 ` Patrick Rudolph
2021-02-22 18:32 ` Guo Dong
0 siblings, 1 reply; 18+ messages in thread
From: Patrick Rudolph @ 2021-02-22 18:06 UTC (permalink / raw)
To: Dong, Guo
Cc: Ma, Maurice, Dong, Eric, Ni, Ray, devel@edk2.groups.io,
You, Benjamin
Hi Guo,
I tested it and it works fine with your patch. The crash is gone.
BTW: Are there are other possible side effects with placing memory
above 4GB? Like 32bit UEFI drivers or Option ROMs malfunctioning?
Regards,
Patrick
On Mon, Feb 22, 2021 at 6:10 PM Dong, Guo <guo.dong@intel.com> wrote:
>
>
> Hi Patrick,
> Thank you for the test and root-cause the issues.
> Could you try if this patch works with "gdtPtr.Base = (UINTN)(VOID*) gdt;"?
> I didn't see this issue when I tested SBL with UEFI payload on QEMU and a real platform.
> I guess there is something that could make AllocateRuntimePool to allocate memory below 4GB while CpuDxe is dispatched above 4GB.
>
> Thanks,
> Guo
>
> > -----Original Message-----
> > From: Ma, Maurice <maurice.ma@intel.com>
> > Sent: Monday, February 22, 2021 9:50 AM
> > To: Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo
> > <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> > <ray.ni@intel.com>
> > Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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@9elements.com>
> > > Sent: Monday, February 22, 2021 7:43
> > > To: Dong, Guo <guo.dong@intel.com>
> > > Cc: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>; You,
> > > Benjamin <benjamin.you@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > Patrick Rudolph
> > > > > Sent: Monday, February 22, 2021 7:04 AM
> > > > > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > > > > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > > > > <benjamin.you@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@intel.com>
> > > wrote:
> > > > > >
> > > > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > > > > >
> > > > > > Regards
> > > > > > Maurice
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Dong, Guo <guo.dong@intel.com>
> > > > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > > > To: devel@edk2.groups.io
> > > > > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > > > > <benjamin.you@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@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
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-22 18:06 ` Patrick Rudolph
@ 2021-02-22 18:32 ` Guo Dong
0 siblings, 0 replies; 18+ messages in thread
From: Guo Dong @ 2021-02-22 18:32 UTC (permalink / raw)
To: devel@edk2.groups.io, patrick.rudolph@9elements.com
Cc: Ma, Maurice, Dong, Eric, Ni, Ray, You, Benjamin
Great. Thanks Patrick for the quick test.
Here is my thought on the UEFI Payload works on 4GB:
UefiPayloadPkg tries to provides a generic UEFI payload impl with minimum hard-coded values.
If UEFI payload could not work since a platform specific limitation, user could change their UEFI payload to work under 4GB. E.g.
a). Mark memory above 4GB as reserved in Payload entry, and update the memory attribute (using memory test driver) later.
b). or set some memory type information PCDs
If UEFI payload has some common issue when it is dispatched above 4GB, we could apply the option B if required.
BTW, current 32bit UEFI payload is not supported, and I don't think it supports memory above 4GB.
Thanks,
Guo
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Patrick
> Rudolph
> Sent: Monday, February 22, 2021 11:06 AM
> To: Dong, Guo <guo.dong@intel.com>
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; You, Benjamin
> <benjamin.you@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove
> 4GB memory WA
>
> Hi Guo,
> I tested it and it works fine with your patch. The crash is gone.
>
> BTW: Are there are other possible side effects with placing memory
> above 4GB? Like 32bit UEFI drivers or Option ROMs malfunctioning?
>
> Regards,
> Patrick
>
> On Mon, Feb 22, 2021 at 6:10 PM Dong, Guo <guo.dong@intel.com> wrote:
> >
> >
> > Hi Patrick,
> > Thank you for the test and root-cause the issues.
> > Could you try if this patch works with "gdtPtr.Base = (UINTN)(VOID*) gdt;"?
> > I didn't see this issue when I tested SBL with UEFI payload on QEMU and a
> real platform.
> > I guess there is something that could make AllocateRuntimePool to allocate
> memory below 4GB while CpuDxe is dispatched above 4GB.
> >
> > Thanks,
> > Guo
> >
> > > -----Original Message-----
> > > From: Ma, Maurice <maurice.ma@intel.com>
> > > Sent: Monday, February 22, 2021 9:50 AM
> > > To: Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo
> > > <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>
> > > Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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@9elements.com>
> > > > Sent: Monday, February 22, 2021 7:43
> > > > To: Dong, Guo <guo.dong@intel.com>
> > > > Cc: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>; You,
> > > > Benjamin <benjamin.you@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > Patrick Rudolph
> > > > > > Sent: Monday, February 22, 2021 7:04 AM
> > > > > > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > > > > > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > > > > > <benjamin.you@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@intel.com>
> > > > wrote:
> > > > > > >
> > > > > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > > > > > >
> > > > > > > Regards
> > > > > > > Maurice
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Dong, Guo <guo.dong@intel.com>
> > > > > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > > > > To: devel@edk2.groups.io
> > > > > > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > > > > > <benjamin.you@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@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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
>
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-22 16:49 ` Ma, Maurice
2021-02-22 17:10 ` Guo Dong
@ 2021-02-23 0:50 ` fanjianfeng
2021-02-23 2:52 ` Ni, Ray
1 sibling, 1 reply; 18+ messages in thread
From: fanjianfeng @ 2021-02-23 0:50 UTC (permalink / raw)
To: devel, maurice.ma, Patrick Rudolph, Dong, Guo, Dong, Eric,
Ni, Ray
Cc: devel@edk2.groups.io, You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 4999 bytes --]
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
Date: 2021-02-23 00:49
To: Patrick Rudolph; Dong, Guo; Dong, Eric; Ni, Ray
CC: devel@edk2.groups.io; You, Benjamin
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@9elements.com>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com>
> Cc: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>; You,
> Benjamin <benjamin.you@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > > <benjamin.you@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@intel.com>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > > <benjamin.you@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@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 9141 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 0:50 ` fanjianfeng
@ 2021-02-23 2:52 ` Ni, Ray
2021-02-23 3:42 ` Jeff Fan
0 siblings, 1 reply; 18+ messages in thread
From: Ni, Ray @ 2021-02-23 2:52 UTC (permalink / raw)
To: fanjianfeng@byosoft.com.cn, devel, Ma, Maurice, Patrick Rudolph,
Dong, Guo, Dong, Eric
Cc: devel@edk2.groups.io, You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 6669 bytes --]
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@byosoft.com.cn <fanjianfeng@byosoft.com.cn>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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@intel.com>
Date: 2021-02-23 00:49
To: Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@9elements.com<mailto:patrick.rudolph@9elements.com>>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You,
> Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > Cc: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; You, Benjamin
> > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:maurice.ma@intel.com>>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You, Benjamin
> > > > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 39216 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 2:52 ` Ni, Ray
@ 2021-02-23 3:42 ` Jeff Fan
2021-02-23 5:21 ` Ni, Ray
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Fan @ 2021-02-23 3:42 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray, maurice.ma, Patrick Rudolph,
Dong, Guo, Dong, Eric
Cc: devel@edk2.groups.io, You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 6481 bytes --]
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
Date: 2021-02-23 10:52
To: fanjianfeng@byosoft.com.cn; devel; Ma, Maurice; Patrick Rudolph; Dong, Guo; Dong, Eric
CC: devel@edk2.groups.io; You, Benjamin
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@byosoft.com.cn <fanjianfeng@byosoft.com.cn>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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
Date: 2021-02-23 00:49
To: Patrick Rudolph; Dong, Guo; Dong, Eric; Ni, Ray
CC: devel@edk2.groups.io; You, Benjamin
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@9elements.com>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com>
> Cc: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>; You,
> Benjamin <benjamin.you@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > > <benjamin.you@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@intel.com>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > > <benjamin.you@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@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 52982 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 3:42 ` Jeff Fan
@ 2021-02-23 5:21 ` Ni, Ray
2021-02-23 5:44 ` Jeff Fan
2021-02-23 7:25 ` Ma, Maurice
0 siblings, 2 replies; 18+ messages in thread
From: Ni, Ray @ 2021-02-23 5:21 UTC (permalink / raw)
To: devel@edk2.groups.io, fanjianfeng@byosoft.com.cn, Ma, Maurice,
Patrick Rudolph, Dong, Guo, Dong, Eric
Cc: You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 8449 bytes --]
“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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 11:43 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>
Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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@intel.com>
Date: 2021-02-23 10:52
To: fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; devel<mailto:devel@edk2.groups.io>; Ma, Maurice<mailto:maurice.ma@intel.com>; Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn> <fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 00:49
To: Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@9elements.com<mailto:patrick.rudolph@9elements.com>>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You,
> Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > Cc: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; You, Benjamin
> > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:maurice.ma@intel.com>>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You, Benjamin
> > > > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 50762 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 5:21 ` Ni, Ray
@ 2021-02-23 5:44 ` Jeff Fan
2021-02-24 1:49 ` Ni, Ray
2021-02-23 7:25 ` Ma, Maurice
1 sibling, 1 reply; 18+ messages in thread
From: Jeff Fan @ 2021-02-23 5:44 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray, maurice.ma, Patrick Rudolph,
Dong, Guo, Dong, Eric
Cc: You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 8507 bytes --]
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
Date: 2021-02-23 13:21
To: devel@edk2.groups.io; fanjianfeng@byosoft.com.cn; Ma, Maurice; Patrick Rudolph; Dong, Guo; Dong, Eric
CC: You, Benjamin
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 11:43 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>
Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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
Date: 2021-02-23 10:52
To: fanjianfeng@byosoft.com.cn; devel; Ma, Maurice; Patrick Rudolph; Dong, Guo; Dong, Eric
CC: devel@edk2.groups.io; You, Benjamin
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@byosoft.com.cn <fanjianfeng@byosoft.com.cn>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io; You, Benjamin <benjamin.you@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
Date: 2021-02-23 00:49
To: Patrick Rudolph; Dong, Guo; Dong, Eric; Ni, Ray
CC: devel@edk2.groups.io; You, Benjamin
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@9elements.com>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com>
> Cc: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>; You,
> Benjamin <benjamin.you@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@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io; Ma, Maurice <maurice.ma@intel.com>
> > > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > > <benjamin.you@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@intel.com>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com>; You, Benjamin
> > > > > <benjamin.you@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@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 70457 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 5:21 ` Ni, Ray
2021-02-23 5:44 ` Jeff Fan
@ 2021-02-23 7:25 ` Ma, Maurice
2021-02-23 9:07 ` Ni, Ray
1 sibling, 1 reply; 18+ messages in thread
From: Ma, Maurice @ 2021-02-23 7:25 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, fanjianfeng@byosoft.com.cn,
Patrick Rudolph, Dong, Guo, Dong, Eric
Cc: You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 9442 bytes --]
Could we decouple BSP and AP GDT in early waking up stage ?
For example, AP uses temporary GDT (below 4GB) just for mode switching. Once AP is in the final stage, AP can reload GDT to match BSP. In this way, we don’t have assumption on the final GDT location.
Thanks
Maurice
From: Ni, Ray <ray.ni@intel.com>
Sent: Monday, February 22, 2021 21:22
To: devel@edk2.groups.io; fanjianfeng@byosoft.com.cn; Ma, Maurice <maurice.ma@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>
Cc: You, Benjamin <benjamin.you@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 11:43 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 10:52
To: fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; devel<mailto:devel@edk2.groups.io>; Ma, Maurice<mailto:maurice.ma@intel.com>; Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn> <fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 00:49
To: Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@9elements.com<mailto:patrick.rudolph@9elements.com>>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You,
> Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > Cc: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; You, Benjamin
> > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:maurice.ma@intel.com>>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You, Benjamin
> > > > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 52571 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 7:25 ` Ma, Maurice
@ 2021-02-23 9:07 ` Ni, Ray
2021-02-23 14:12 ` Ma, Maurice
0 siblings, 1 reply; 18+ messages in thread
From: Ni, Ray @ 2021-02-23 9:07 UTC (permalink / raw)
To: Ma, Maurice, devel@edk2.groups.io, fanjianfeng@byosoft.com.cn,
Patrick Rudolph, Dong, Guo, Dong, Eric
Cc: You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 10235 bytes --]
Maurice,
It’s doable 😊
I prefer to accept today’s below 4G limitation to maintain simple implementation.
Thanks,
Ray
From: Ma, Maurice <maurice.ma@intel.com>
Sent: Tuesday, February 23, 2021 3:26 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; fanjianfeng@byosoft.com.cn; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>
Cc: You, Benjamin <benjamin.you@intel.com>
Subject: RE: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Could we decouple BSP and AP GDT in early waking up stage ?
For example, AP uses temporary GDT (below 4GB) just for mode switching. Once AP is in the final stage, AP can reload GDT to match BSP. In this way, we don’t have assumption on the final GDT location.
Thanks
Maurice
From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Sent: Monday, February 22, 2021 21:22
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 11:43 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 10:52
To: fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; devel<mailto:devel@edk2.groups.io>; Ma, Maurice<mailto:maurice.ma@intel.com>; Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn> <fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 00:49
To: Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@9elements.com<mailto:patrick.rudolph@9elements.com>>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You,
> Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > Cc: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; You, Benjamin
> > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:maurice.ma@intel.com>>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You, Benjamin
> > > > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 54187 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 9:07 ` Ni, Ray
@ 2021-02-23 14:12 ` Ma, Maurice
0 siblings, 0 replies; 18+ messages in thread
From: Ma, Maurice @ 2021-02-23 14:12 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, fanjianfeng@byosoft.com.cn,
Patrick Rudolph, Dong, Guo, Dong, Eric
Cc: You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 11035 bytes --]
Hi, Ray, Eric and Jeff,
Sure, I am OK with that too.
Thank you all for looking into it and provide a solution quickly.
Regards
Maurice
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, February 23, 2021 1:08
To: Ma, Maurice <maurice.ma@intel.com>; devel@edk2.groups.io; fanjianfeng@byosoft.com.cn; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>
Cc: You, Benjamin <benjamin.you@intel.com>
Subject: RE: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Maurice,
It’s doable 😊
I prefer to accept today’s below 4G limitation to maintain simple implementation.
Thanks,
Ray
From: Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
Sent: Tuesday, February 23, 2021 3:26 PM
To: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@intel.com>>
Subject: RE: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
Could we decouple BSP and AP GDT in early waking up stage ?
For example, AP uses temporary GDT (below 4GB) just for mode switching. Once AP is in the final stage, AP can reload GDT to match BSP. In this way, we don’t have assumption on the final GDT location.
Thanks
Maurice
From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Sent: Monday, February 22, 2021 21:22
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 11:43 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 10:52
To: fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; devel<mailto:devel@edk2.groups.io>; Ma, Maurice<mailto:maurice.ma@intel.com>; Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn> <fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 00:49
To: Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@9elements.com<mailto:patrick.rudolph@9elements.com>>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You,
> Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > Cc: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; You, Benjamin
> > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:maurice.ma@intel.com>>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You, Benjamin
> > > > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 55861 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA
2021-02-23 5:44 ` Jeff Fan
@ 2021-02-24 1:49 ` Ni, Ray
0 siblings, 0 replies; 18+ messages in thread
From: Ni, Ray @ 2021-02-24 1:49 UTC (permalink / raw)
To: devel@edk2.groups.io, fanjianfeng@byosoft.com.cn, Ma, Maurice,
Patrick Rudolph, Dong, Guo, Dong, Eric
Cc: You, Benjamin
[-- Attachment #1: Type: text/plain, Size: 11040 bytes --]
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 1:44 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>; Dong, Guo <guo.dong@intel.com>; Dong, Eric <eric.dong@intel.com>
Cc: You, Benjamin <benjamin.you@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@intel.com>
Date: 2021-02-23 13:21
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; Ma, Maurice<mailto:maurice.ma@intel.com>; Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>
CC: You, Benjamin<mailto:benjamin.you@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Jeff Fan
Sent: Tuesday, February 23, 2021 11:43 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 10:52
To: fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>; devel<mailto:devel@edk2.groups.io>; Ma, Maurice<mailto:maurice.ma@intel.com>; Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn> <fanjianfeng@byosoft.com.cn<mailto:fanjianfeng@byosoft.com.cn>>
Sent: Tuesday, February 23, 2021 8:50 AM
To: devel <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; Patrick Rudolph <patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>>; Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com>
Date: 2021-02-23 00:49
To: Patrick Rudolph<mailto:patrick.rudolph@9elements.com>; Dong, Guo<mailto:guo.dong@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
CC: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; You, Benjamin<mailto:benjamin.you@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@9elements.com<mailto:patrick.rudolph@9elements.com>>
> Sent: Monday, February 22, 2021 7:43
> To: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You,
> Benjamin <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of
> > > Patrick Rudolph
> > > Sent: Monday, February 22, 2021 7:04 AM
> > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > Cc: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>; You, Benjamin
> > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:maurice.ma@intel.com>>
> wrote:
> > > >
> > > > Reviewed-by: Maurice Ma <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
> > > >
> > > > Regards
> > > > Maurice
> > > >
> > > > > -----Original Message-----
> > > > > From: Dong, Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
> > > > > Sent: Sunday, February 14, 2021 21:13
> > > > > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > > > > Cc: Ma, Maurice <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>; You, Benjamin
> > > > > <benjamin.you@intel.com<mailto:benjamin.you@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@intel.com<mailto:guo.dong@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
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
[-- Attachment #2: Type: text/html, Size: 66134 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-02-24 1:50 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-15 5:13 [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Remove 4GB memory WA Guo Dong
2021-02-19 2:12 ` Ma, Maurice
2021-02-22 14:04 ` Patrick Rudolph
2021-02-22 14:59 ` Guo Dong
2021-02-22 15:42 ` Patrick Rudolph
2021-02-22 16:49 ` Ma, Maurice
2021-02-22 17:10 ` Guo Dong
2021-02-22 18:06 ` Patrick Rudolph
2021-02-22 18:32 ` Guo Dong
2021-02-23 0:50 ` fanjianfeng
2021-02-23 2:52 ` Ni, Ray
2021-02-23 3:42 ` Jeff Fan
2021-02-23 5:21 ` Ni, Ray
2021-02-23 5:44 ` Jeff Fan
2021-02-24 1:49 ` Ni, Ray
2021-02-23 7:25 ` Ma, Maurice
2021-02-23 9:07 ` Ni, Ray
2021-02-23 14:12 ` Ma, Maurice
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox