* [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer initialization
@ 2018-01-24 2:08 Jian J Wang
2018-01-24 15:29 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Jian J Wang @ 2018-01-24 2:08 UTC (permalink / raw)
To: edk2-devel; +Cc: Ruiyu Ni, Eric Dong, Laszlo Ersek
To fix an issue in which enabling NX feature will mark the AP wakeup
buffer as non-executable and fail the AP init, the buffer was split
into two part: the lower part in memory within 1MB and the higher part
within allocated executable memory (EfiBootServicesCode). But the
address of higher part memory was stored in lower part memory, which
is actually shared with legacy components and will be overwritten by
LegacyBiosDxe driver if CSM is enabled.
This patch fixes this issue by storing the address of higher part
memory in CpuMpData instead of ExchangeInfo.
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 20 ++++++++++----------
UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 6231968c74..42011d6231 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -823,19 +823,20 @@ FillExchangeInfoData (
// Copy all 32-bit code and 64-bit code into memory with type of
// EfiBootServicesCode to avoid page fault if NX memory protection is enabled.
//
- if (ExchangeInfo->ModeTransitionMemory != 0) {
+ if (CpuMpData->WakeupBufferHigh != 0) {
Size = CpuMpData->AddressMap.RendezvousFunnelSize -
CpuMpData->AddressMap.ModeTransitionOffset;
CopyMem (
- (VOID *)(UINTN)ExchangeInfo->ModeTransitionMemory,
+ (VOID *)CpuMpData->WakeupBufferHigh,
CpuMpData->AddressMap.RendezvousFunnelAddress +
CpuMpData->AddressMap.ModeTransitionOffset,
Size
);
- ExchangeInfo->ModeHighMemory = ExchangeInfo->ModeTransitionMemory;
- ExchangeInfo->ModeHighMemory += (UINT32)ExchangeInfo->ModeOffset -
- (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
+ ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData->WakeupBufferHigh;
+ ExchangeInfo->ModeHighMemory = (UINT32)CpuMpData->WakeupBufferHigh +
+ (UINT32)ExchangeInfo->ModeOffset -
+ (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment;
} else {
ExchangeInfo->ModeTransitionMemory = (UINT32)
@@ -916,11 +917,10 @@ AllocateResetVector (
CpuMpData->WakeupBuffer = GetWakeupBuffer (ApResetVectorSize);
CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *) (UINTN)
(CpuMpData->WakeupBuffer + CpuMpData->AddressMap.RendezvousFunnelSize);
- CpuMpData->MpCpuExchangeInfo->ModeTransitionMemory = (UINT32)
- GetModeTransitionBuffer (
- CpuMpData->AddressMap.RendezvousFunnelSize -
- CpuMpData->AddressMap.ModeTransitionOffset
- );
+ CpuMpData->WakeupBufferHigh = GetModeTransitionBuffer (
+ CpuMpData->AddressMap.RendezvousFunnelSize -
+ CpuMpData->AddressMap.ModeTransitionOffset
+ );
}
BackupAndPrepareWakeupBuffer (CpuMpData);
}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 0232fe896a..e7f9a4de0a 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -208,6 +208,7 @@ struct _CPU_MP_DATA {
UINTN CpuApStackSize;
MP_ASSEMBLY_ADDRESS_MAP AddressMap;
UINTN WakeupBuffer;
+ UINTN WakeupBufferHigh;
UINTN BackupBuffer;
UINTN BackupBufferSize;
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer initialization
2018-01-24 2:08 [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer initialization Jian J Wang
@ 2018-01-24 15:29 ` Laszlo Ersek
2018-01-25 1:23 ` Wang, Jian J
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2018-01-24 15:29 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Eric Dong
On 01/24/18 03:08, Jian J Wang wrote:
> To fix an issue in which enabling NX feature will mark the AP wakeup
> buffer as non-executable and fail the AP init, the buffer was split
> into two part: the lower part in memory within 1MB and the higher part
> within allocated executable memory (EfiBootServicesCode). But the
> address of higher part memory was stored in lower part memory, which
> is actually shared with legacy components and will be overwritten by
> LegacyBiosDxe driver if CSM is enabled.
>
> This patch fixes this issue by storing the address of higher part
> memory in CpuMpData instead of ExchangeInfo.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 20 ++++++++++----------
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
> 2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6231968c74..42011d6231 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -823,19 +823,20 @@ FillExchangeInfoData (
> // Copy all 32-bit code and 64-bit code into memory with type of
> // EfiBootServicesCode to avoid page fault if NX memory protection is enabled.
> //
> - if (ExchangeInfo->ModeTransitionMemory != 0) {
> + if (CpuMpData->WakeupBufferHigh != 0) {
> Size = CpuMpData->AddressMap.RendezvousFunnelSize -
> CpuMpData->AddressMap.ModeTransitionOffset;
> CopyMem (
> - (VOID *)(UINTN)ExchangeInfo->ModeTransitionMemory,
> + (VOID *)CpuMpData->WakeupBufferHigh,
> CpuMpData->AddressMap.RendezvousFunnelAddress +
> CpuMpData->AddressMap.ModeTransitionOffset,
> Size
> );
>
> - ExchangeInfo->ModeHighMemory = ExchangeInfo->ModeTransitionMemory;
> - ExchangeInfo->ModeHighMemory += (UINT32)ExchangeInfo->ModeOffset -
> - (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
> + ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData->WakeupBufferHigh;
> + ExchangeInfo->ModeHighMemory = (UINT32)CpuMpData->WakeupBufferHigh +
> + (UINT32)ExchangeInfo->ModeOffset -
> + (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
> ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment;
> } else {
> ExchangeInfo->ModeTransitionMemory = (UINT32)
> @@ -916,11 +917,10 @@ AllocateResetVector (
> CpuMpData->WakeupBuffer = GetWakeupBuffer (ApResetVectorSize);
> CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *) (UINTN)
> (CpuMpData->WakeupBuffer + CpuMpData->AddressMap.RendezvousFunnelSize);
> - CpuMpData->MpCpuExchangeInfo->ModeTransitionMemory = (UINT32)
> - GetModeTransitionBuffer (
> - CpuMpData->AddressMap.RendezvousFunnelSize -
> - CpuMpData->AddressMap.ModeTransitionOffset
> - );
> + CpuMpData->WakeupBufferHigh = GetModeTransitionBuffer (
> + CpuMpData->AddressMap.RendezvousFunnelSize -
> + CpuMpData->AddressMap.ModeTransitionOffset
> + );
> }
> BackupAndPrepareWakeupBuffer (CpuMpData);
> }
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 0232fe896a..e7f9a4de0a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -208,6 +208,7 @@ struct _CPU_MP_DATA {
> UINTN CpuApStackSize;
> MP_ASSEMBLY_ADDRESS_MAP AddressMap;
> UINTN WakeupBuffer;
> + UINTN WakeupBufferHigh;
> UINTN BackupBuffer;
> UINTN BackupBufferSize;
>
>
As far as I see, all the values stored by FillExchangeInfoData() remain
the same. The difference is that "ModeTransitionMemory" becomes an
*output* field for FillExchangeInfoData(), from being an input field. We
now explicitly store the value to it that we previously *assumed* it
would hold (preserve), from AllocateResetVector().
The input now comes through WakeupBufferHigh, from the
AllocateResetVector() function.
Thus, I think the patch could have been written a bit more simply: we
could have added the assignment
ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData->WakeupBufferHigh
before the "if" in FillExchangeInfoData(), and then the rest of
FillExchangeInfoData() would have needed no changes. But perhaps that
would have been harder to understand.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer initialization
2018-01-24 15:29 ` Laszlo Ersek
@ 2018-01-25 1:23 ` Wang, Jian J
0 siblings, 0 replies; 3+ messages in thread
From: Wang, Jian J @ 2018-01-25 1:23 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Dong, Eric
Considering that there's a similar assignment in "else" block
ExchangeInfo->ModeTransitionMemory = (UINT32)
(ExchangeInfo->BufferStart + CpuMpData->AddressMap.ModeTransitionOffset);
I would rather keep the assignment statement you mentioned inside "if" block.
Actually there's another issue in this part of code. To fix it, the assignment to
ModeHighMemory and ModeHighSegment will be moved outside the "if" block.
From this fix perspective, it'd be also better to keep all assignments to
ModeTransitionMemory inside "if/else" block. You'll see it in a new patch later.
Anyway, thanks for the comments.
Regards,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, January 24, 2018 11:30 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer
> initialization
>
> On 01/24/18 03:08, Jian J Wang wrote:
> > To fix an issue in which enabling NX feature will mark the AP wakeup
> > buffer as non-executable and fail the AP init, the buffer was split
> > into two part: the lower part in memory within 1MB and the higher part
> > within allocated executable memory (EfiBootServicesCode). But the
> > address of higher part memory was stored in lower part memory, which
> > is actually shared with legacy components and will be overwritten by
> > LegacyBiosDxe driver if CSM is enabled.
> >
> > This patch fixes this issue by storing the address of higher part
> > memory in CpuMpData instead of ExchangeInfo.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 20 ++++++++++----------
> > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
> > 2 files changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 6231968c74..42011d6231 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -823,19 +823,20 @@ FillExchangeInfoData (
> > // Copy all 32-bit code and 64-bit code into memory with type of
> > // EfiBootServicesCode to avoid page fault if NX memory protection is
> enabled.
> > //
> > - if (ExchangeInfo->ModeTransitionMemory != 0) {
> > + if (CpuMpData->WakeupBufferHigh != 0) {
> > Size = CpuMpData->AddressMap.RendezvousFunnelSize -
> > CpuMpData->AddressMap.ModeTransitionOffset;
> > CopyMem (
> > - (VOID *)(UINTN)ExchangeInfo->ModeTransitionMemory,
> > + (VOID *)CpuMpData->WakeupBufferHigh,
> > CpuMpData->AddressMap.RendezvousFunnelAddress +
> > CpuMpData->AddressMap.ModeTransitionOffset,
> > Size
> > );
> >
> > - ExchangeInfo->ModeHighMemory = ExchangeInfo-
> >ModeTransitionMemory;
> > - ExchangeInfo->ModeHighMemory += (UINT32)ExchangeInfo->ModeOffset
> -
> > - (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
> > + ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData-
> >WakeupBufferHigh;
> > + ExchangeInfo->ModeHighMemory = (UINT32)CpuMpData-
> >WakeupBufferHigh +
> > + (UINT32)ExchangeInfo->ModeOffset -
> > + (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
> > ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment;
> > } else {
> > ExchangeInfo->ModeTransitionMemory = (UINT32)
> > @@ -916,11 +917,10 @@ AllocateResetVector (
> > CpuMpData->WakeupBuffer = GetWakeupBuffer (ApResetVectorSize);
> > CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *) (UINTN)
> > (CpuMpData->WakeupBuffer + CpuMpData-
> >AddressMap.RendezvousFunnelSize);
> > - CpuMpData->MpCpuExchangeInfo->ModeTransitionMemory = (UINT32)
> > - GetModeTransitionBuffer (
> > - CpuMpData->AddressMap.RendezvousFunnelSize -
> > - CpuMpData->AddressMap.ModeTransitionOffset
> > - );
> > + CpuMpData->WakeupBufferHigh = GetModeTransitionBuffer (
> > + CpuMpData->AddressMap.RendezvousFunnelSize -
> > + CpuMpData->AddressMap.ModeTransitionOffset
> > + );
> > }
> > BackupAndPrepareWakeupBuffer (CpuMpData);
> > }
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 0232fe896a..e7f9a4de0a 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -208,6 +208,7 @@ struct _CPU_MP_DATA {
> > UINTN CpuApStackSize;
> > MP_ASSEMBLY_ADDRESS_MAP AddressMap;
> > UINTN WakeupBuffer;
> > + UINTN WakeupBufferHigh;
> > UINTN BackupBuffer;
> > UINTN BackupBufferSize;
> >
> >
>
> As far as I see, all the values stored by FillExchangeInfoData() remain
> the same. The difference is that "ModeTransitionMemory" becomes an
> *output* field for FillExchangeInfoData(), from being an input field. We
> now explicitly store the value to it that we previously *assumed* it
> would hold (preserve), from AllocateResetVector().
>
> The input now comes through WakeupBufferHigh, from the
> AllocateResetVector() function.
>
> Thus, I think the patch could have been written a bit more simply: we
> could have added the assignment
>
> ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData-
> >WakeupBufferHigh
>
> before the "if" in FillExchangeInfoData(), and then the rest of
> FillExchangeInfoData() would have needed no changes. But perhaps that
> would have been harder to understand.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-25 1:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-24 2:08 [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer initialization Jian J Wang
2018-01-24 15:29 ` Laszlo Ersek
2018-01-25 1:23 ` Wang, Jian J
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox