Because CpuS3Data memory will be copy to smram at SmmReadyToLock point,
the memory type no need to be ACPI NVS type, also the address not
limit to below 4G.
This change remove the limit of ACPI NVS memory type and below 4G.
Pass OS boot and resume from S3 test.
Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
Cc: Fan Jeff <vanjeff_919@hotmail.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 43 +++++++++++++++++++++++++++++++------
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
index dccb406b8d..5b99a6e759 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -81,6 +81,38 @@ AllocateAcpiNvsMemoryBelow4G (
return Buffer;
}
+/**
+ Allocate EfiBootService memory.
+
+ @param[in] Size Size of memory to allocate.
+
+ @return Allocated address for output.
+
+**/
+VOID *
+AllocateBootServiceMemory (
+ IN UINTN Size
+ )
+{
+ EFI_PHYSICAL_ADDRESS Address;
+ EFI_STATUS Status;
+ VOID *Buffer;
+
+ Status = gBS->AllocatePages (
+ AllocateAnyPages,
+ EfiBootServicesData,
+ EFI_SIZE_TO_PAGES (Size),
+ &Address
+ );
+ if (EFI_ERROR (Status)) {
+ return NULL;
+ }
+
+ Buffer = (VOID *)(UINTN)Address;
+ ZeroMem (Buffer, Size);
+
+ return Buffer;
+}
/**
Callback function executed when the EndOfDxe event group is signaled.
@@ -171,10 +203,7 @@ CpuS3DataInitialize (
//
OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
- //
- // Allocate ACPI NVS memory below 4G memory for use on ACPI S3 resume.
- //
- AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof (ACPI_CPU_DATA_EX));
+ AcpiCpuDataEx = AllocateBootServiceMemory (sizeof (ACPI_CPU_DATA_EX));
ASSERT (AcpiCpuDataEx != NULL);
AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
@@ -223,11 +252,11 @@ CpuS3DataInitialize (
AsmReadIdtr (&AcpiCpuDataEx->IdtrProfile);
//
- // Allocate GDT and IDT in ACPI NVS and copy current GDT and IDT contents
+ // Allocate GDT and IDT and copy current GDT and IDT contents
//
GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
- Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
+ Gdt = AllocateBootServiceMemory (GdtSize + IdtSize);
ASSERT (Gdt != NULL);
Idt = (VOID *)((UINTN)Gdt + GdtSize);
CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
@@ -243,7 +272,7 @@ CpuS3DataInitialize (
// Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
//
TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
- RegisterTable = (CPU_REGISTER_TABLE *)AllocateAcpiNvsMemoryBelow4G (TableSize);
+ RegisterTable = (CPU_REGISTER_TABLE *)AllocateBootServiceMemory (TableSize);
ASSERT (RegisterTable != NULL);
for (Index = 0; Index < NumberOfCpus; Index++) {
--
2.15.0.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 08/10/18 06:19, Eric Dong wrote:
> Because CpuS3Data memory will be copy to smram at SmmReadyToLock point,
> the memory type no need to be ACPI NVS type, also the address not
> limit to below 4G.
>
> This change remove the limit of ACPI NVS memory type and below 4G.
>
> Pass OS boot and resume from S3 test.
>
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Cc: Fan Jeff <vanjeff_919@hotmail.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 43 +++++++++++++++++++++++++++++++------
> 1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..5b99a6e759 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -81,6 +81,38 @@ AllocateAcpiNvsMemoryBelow4G (
> return Buffer;
> }
>
> +/**
> + Allocate EfiBootService memory.
> +
> + @param[in] Size Size of memory to allocate.
> +
> + @return Allocated address for output.
> +
> +**/
> +VOID *
> +AllocateBootServiceMemory (
> + IN UINTN Size
> + )
> +{
> + EFI_PHYSICAL_ADDRESS Address;
> + EFI_STATUS Status;
> + VOID *Buffer;
> +
> + Status = gBS->AllocatePages (
> + AllocateAnyPages,
> + EfiBootServicesData,
> + EFI_SIZE_TO_PAGES (Size),
> + &Address
> + );
> + if (EFI_ERROR (Status)) {
> + return NULL;
> + }
> +
> + Buffer = (VOID *)(UINTN)Address;
> + ZeroMem (Buffer, Size);
> +
> + return Buffer;
> +}
(1) If I remember correctly, we discussed AllocateZeroPages() for this.
Why did you decide against it?
CpuS3DataDxe is a DXE_DRIVER, and the matching MemoryAllocationLib
instance would allocate Boot Services Data type memory.
> /**
> Callback function executed when the EndOfDxe event group is signaled.
>
> @@ -171,10 +203,7 @@ CpuS3DataInitialize (
> //
> OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
>
> - //
> - // Allocate ACPI NVS memory below 4G memory for use on ACPI S3 resume.
> - //
> - AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof (ACPI_CPU_DATA_EX));
> + AcpiCpuDataEx = AllocateBootServiceMemory (sizeof (ACPI_CPU_DATA_EX));
> ASSERT (AcpiCpuDataEx != NULL);
> AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
>
> @@ -223,11 +252,11 @@ CpuS3DataInitialize (
> AsmReadIdtr (&AcpiCpuDataEx->IdtrProfile);
>
(2) In the previous patch, we lifted the 4GB limitation on the stack
address (while preserving the memory type restriction as AcpiNVS).
However, you continue to allocate the stack with
AllocateAcpiNvsMemoryBelow4G().
I don't think that's consistent with the purpose of this patch set, or
with the documentation change in the previous patch. We should allocate
the stack as AcpiNVS without address limitation.
And then we can remove the AllocateAcpiNvsMemoryBelow4G() function
altogether.
> //
> - // Allocate GDT and IDT in ACPI NVS and copy current GDT and IDT contents
> + // Allocate GDT and IDT and copy current GDT and IDT contents
> //
> GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
> IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> - Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> + Gdt = AllocateBootServiceMemory (GdtSize + IdtSize);
> ASSERT (Gdt != NULL);
> Idt = (VOID *)((UINTN)Gdt + GdtSize);
> CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
> @@ -243,7 +272,7 @@ CpuS3DataInitialize (
> // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> //
> TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> - RegisterTable = (CPU_REGISTER_TABLE *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> + RegisterTable = (CPU_REGISTER_TABLE *)AllocateBootServiceMemory (TableSize);
> ASSERT (RegisterTable != NULL);
>
> for (Index = 0; Index < NumberOfCpus; Index++) {
>
Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 08/10/18 18:12, Laszlo Ersek wrote: > On 08/10/18 06:19, Eric Dong wrote: >> Because CpuS3Data memory will be copy to smram at SmmReadyToLock point, >> the memory type no need to be ACPI NVS type, also the address not >> limit to below 4G. >> >> This change remove the limit of ACPI NVS memory type and below 4G. >> >> Pass OS boot and resume from S3 test. [snip] > (2) In the previous patch, we lifted the 4GB limitation on the stack > address (while preserving the memory type restriction as AcpiNVS). > However, you continue to allocate the stack with > AllocateAcpiNvsMemoryBelow4G(). > > I don't think that's consistent with the purpose of this patch set, or > with the documentation change in the previous patch. We should allocate > the stack as AcpiNVS without address limitation. > > And then we can remove the AllocateAcpiNvsMemoryBelow4G() function > altogether. Nevermind, I'm just seeing the next patch. (You might want to add a hint about the next patch to the commit message of this patch -- "we'll handle the stack in the next patch".) So only my question (1) remains for this patch, i.e. why not use AllocateZeroPages(). Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Laszlo, I checked the code base and don't find the API AllocateZeroPages (), and I agree to Zero the memory before using it. So I think update the original function is the best way now. Thanks, Eric > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Saturday, August 11, 2018 12:15 AM > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff > <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: Re: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type > and address limitation. > > On 08/10/18 18:12, Laszlo Ersek wrote: > > On 08/10/18 06:19, Eric Dong wrote: > >> Because CpuS3Data memory will be copy to smram at SmmReadyToLock > >> point, the memory type no need to be ACPI NVS type, also the address > >> not limit to below 4G. > >> > >> This change remove the limit of ACPI NVS memory type and below 4G. > >> > >> Pass OS boot and resume from S3 test. > > [snip] > > > (2) In the previous patch, we lifted the 4GB limitation on the stack > > address (while preserving the memory type restriction as AcpiNVS). > > However, you continue to allocate the stack with > > AllocateAcpiNvsMemoryBelow4G(). > > > > I don't think that's consistent with the purpose of this patch set, or > > with the documentation change in the previous patch. We should > > allocate the stack as AcpiNVS without address limitation. > > > > And then we can remove the AllocateAcpiNvsMemoryBelow4G() function > > altogether. > > Nevermind, I'm just seeing the next patch. > > (You might want to add a hint about the next patch to the commit message of > this patch -- "we'll handle the stack in the next patch".) > > So only my question (1) remains for this patch, i.e. why not use > AllocateZeroPages(). > > Thanks, > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Eric, I think you could change: AcpiCpuDataEx = AllocateBootServiceMemory (sizeof (ACPI_CPU_DATA_EX)); -> AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA_EX))); So the AllocateBootServiceMemory() is not needed. Thanks/Ray > -----Original Message----- > From: Dong, Eric > Sent: Monday, August 13, 2018 9:51 AM > To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff > <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: RE: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory > Type and address limitation. > > Hi Laszlo, > > I checked the code base and don't find the API AllocateZeroPages (), and I > agree to Zero the memory before using it. So I think update the original > function is the best way now. > > Thanks, > Eric > > > -----Original Message----- > > From: Laszlo Ersek [mailto:lersek@redhat.com] > > Sent: Saturday, August 11, 2018 12:15 AM > > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff > > <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > Subject: Re: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory > > Type and address limitation. > > > > On 08/10/18 18:12, Laszlo Ersek wrote: > > > On 08/10/18 06:19, Eric Dong wrote: > > >> Because CpuS3Data memory will be copy to smram at SmmReadyToLock > > >> point, the memory type no need to be ACPI NVS type, also the > > >> address not limit to below 4G. > > >> > > >> This change remove the limit of ACPI NVS memory type and below 4G. > > >> > > >> Pass OS boot and resume from S3 test. > > > > [snip] > > > > > (2) In the previous patch, we lifted the 4GB limitation on the stack > > > address (while preserving the memory type restriction as AcpiNVS). > > > However, you continue to allocate the stack with > > > AllocateAcpiNvsMemoryBelow4G(). > > > > > > I don't think that's consistent with the purpose of this patch set, > > > or with the documentation change in the previous patch. We should > > > allocate the stack as AcpiNVS without address limitation. > > > > > > And then we can remove the AllocateAcpiNvsMemoryBelow4G() > function > > > altogether. > > > > Nevermind, I'm just seeing the next patch. > > > > (You might want to add a hint about the next patch to the commit > > message of this patch -- "we'll handle the stack in the next patch".) > > > > So only my question (1) remains for this patch, i.e. why not use > > AllocateZeroPages(). > > > > Thanks, > > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Ignore my previous mail. You could use AllocatePages() instead of directly calling gBS->AllocatePages(). Then rename the AllocateBootServiceMemory() to AllocateZeroPages() to better reflect the function behavior. Thanks/Ray > -----Original Message----- > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Ni, Ruiyu > Sent: Monday, August 13, 2018 1:39 PM > To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; > edk2-devel@lists.01.org > Subject: Re: [edk2] [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change > Memory Type and address limitation. > > Eric, > I think you could change: > AcpiCpuDataEx = AllocateBootServiceMemory (sizeof > (ACPI_CPU_DATA_EX)); > -> > AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof > (ACPI_CPU_DATA_EX))); > > So the AllocateBootServiceMemory() is not needed. > > Thanks/Ray > > > -----Original Message----- > > From: Dong, Eric > > Sent: Monday, August 13, 2018 9:51 AM > > To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org > > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff > > <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > Subject: RE: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory > > Type and address limitation. > > > > Hi Laszlo, > > > > I checked the code base and don't find the API AllocateZeroPages (), > > and I agree to Zero the memory before using it. So I think update the > > original function is the best way now. > > > > Thanks, > > Eric > > > > > -----Original Message----- > > > From: Laszlo Ersek [mailto:lersek@redhat.com] > > > Sent: Saturday, August 11, 2018 12:15 AM > > > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > > > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff > > > <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > > Subject: Re: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory > > > Type and address limitation. > > > > > > On 08/10/18 18:12, Laszlo Ersek wrote: > > > > On 08/10/18 06:19, Eric Dong wrote: > > > >> Because CpuS3Data memory will be copy to smram at > SmmReadyToLock > > > >> point, the memory type no need to be ACPI NVS type, also the > > > >> address not limit to below 4G. > > > >> > > > >> This change remove the limit of ACPI NVS memory type and below 4G. > > > >> > > > >> Pass OS boot and resume from S3 test. > > > > > > [snip] > > > > > > > (2) In the previous patch, we lifted the 4GB limitation on the > > > > stack address (while preserving the memory type restriction as > AcpiNVS). > > > > However, you continue to allocate the stack with > > > > AllocateAcpiNvsMemoryBelow4G(). > > > > > > > > I don't think that's consistent with the purpose of this patch > > > > set, or with the documentation change in the previous patch. We > > > > should allocate the stack as AcpiNVS without address limitation. > > > > > > > > And then we can remove the AllocateAcpiNvsMemoryBelow4G() > > function > > > > altogether. > > > > > > Nevermind, I'm just seeing the next patch. > > > > > > (You might want to add a hint about the next patch to the commit > > > message of this patch -- "we'll handle the stack in the next > > > patch".) > > > > > > So only my question (1) remains for this patch, i.e. why not use > > > AllocateZeroPages(). > > > > > > Thanks, > > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 08/13/18 07:52, Ni, Ruiyu wrote: > Ignore my previous mail. > > You could use AllocatePages() instead of directly calling gBS->AllocatePages(). > Then rename the AllocateBootServiceMemory() to AllocateZeroPages() to better > reflect the function behavior. > > Thanks/Ray > >> -----Original Message----- >> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Ni, Ruiyu >> Sent: Monday, August 13, 2018 1:39 PM >> To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; >> edk2-devel@lists.01.org >> Subject: Re: [edk2] [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change >> Memory Type and address limitation. >> >> Eric, >> I think you could change: >> AcpiCpuDataEx = AllocateBootServiceMemory (sizeof >> (ACPI_CPU_DATA_EX)); >> -> >> AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof >> (ACPI_CPU_DATA_EX))); >> >> So the AllocateBootServiceMemory() is not needed. >> >> Thanks/Ray >> >>> -----Original Message----- >>> From: Dong, Eric >>> Sent: Monday, August 13, 2018 9:51 AM >>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org >>> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff >>> <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com> >>> Subject: RE: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory >>> Type and address limitation. >>> >>> Hi Laszlo, >>> >>> I checked the code base and don't find the API AllocateZeroPages (), Sorry, I was misled by AllocateZeroPages() in [IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c]. >>> and I agree to Zero the memory before using it. So I think update the >>> original function is the best way now. If you wouldn't like to do what Ray suggests, or you would like to do it later / separately: Reviewed-by: Laszlo Ersek <lersek@redhat.com> because both of my earlier questions for this patch have been explained. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.