.../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++-- 1 file changed, 54 insertions(+), 3 deletions(-)
Current code logic can't confirm CpuS3DataDxe driver start before
CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not
valid. Add implementation for AllocateAcpiCpuData function to remove
this assumption.
Pass OS boot and resume from S3 test.
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>
---
.../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++--
1 file changed, 54 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
index 902a339529..0722db6c64 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
@@ -207,11 +207,62 @@ AllocateAcpiCpuData (
VOID
)
{
+ EFI_STATUS Status;
+ UINTN NumberOfCpus;
+ UINTN NumberOfEnabledProcessors;
+ ACPI_CPU_DATA *AcpiCpuData;
+ EFI_PHYSICAL_ADDRESS Address;
+ UINTN TableSize;
+ CPU_REGISTER_TABLE *RegisterTable;
+ UINTN Index;
+ EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
+
+ Address = BASE_4GB - 1;
+ Status = gBS->AllocatePages (
+ AllocateMaxAddress,
+ EfiACPIMemoryNVS,
+ EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)),
+ &Address
+ );
+ ASSERT_EFI_ERROR (Status);
+ AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address;
+ ASSERT (AcpiCpuData != NULL);
+
+ GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+ AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus;
+
//
- // CpuS3DataDxe will do it.
+ // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
//
- ASSERT (FALSE);
- return NULL;
+ TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
+ Address = BASE_4GB - 1;
+ Status = gBS->AllocatePages (
+ AllocateMaxAddress,
+ EfiACPIMemoryNVS,
+ EFI_SIZE_TO_PAGES (TableSize),
+ &Address
+ );
+ ASSERT_EFI_ERROR (Status);
+ RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address;
+
+ for (Index = 0; Index < NumberOfCpus; Index++) {
+ Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
+ ASSERT_EFI_ERROR (Status);
+
+ RegisterTable[Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
+ RegisterTable[Index].TableLength = 0;
+ RegisterTable[Index].AllocatedSize = 0;
+ RegisterTable[Index].RegisterTableEntry = 0;
+
+ RegisterTable[NumberOfCpus + Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
+ RegisterTable[NumberOfCpus + Index].TableLength = 0;
+ RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
+ RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
+ }
+ AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
+ AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
+
+ return AcpiCpuData;
}
/**
--
2.15.0.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hello Eric,
OVMF does not include CpuFeaturesPei / CpuFeaturesDxe, and so it doesn't
consume this library. I can't provide test results, but I have some
superficial comments.
First, I'm adding Marvin and Jeff -- I *vaguely* recall that this issue
may have been raised by Marvin. Hm... yes, it seems so:
[edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.
http://mid.mail-archive.com/VI1PR0801MB17908DB1F03A3C5F84545E1080690@VI1PR0801MB1790.eurprd08.prod.outlook.com
https://lists.01.org/pipermail/edk2-devel/2018-May/025163.html
I have three questions here:
(1) Do we have a TianoCore BZ about this?
(2) If not, does the currently proposed commit message capture the issue
in enough detail? Should we reference Marvin's initial report and/or a
TianoCore BZ (if we have one)?
(3) The implementation seems to follow Jeff's idea. Marvin and Jeff, can
you please help with the review?
Either way, I propose the following two tags to be appended:
Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
And one technical question follows below:
On 08/03/18 04:10, Eric Dong wrote:
> Current code logic can't confirm CpuS3DataDxe driver start before
> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not
> valid. Add implementation for AllocateAcpiCpuData function to remove
> this assumption.
>
> Pass OS boot and resume from S3 test.
>
> 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>
> ---
> .../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++--
> 1 file changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
> index 902a339529..0722db6c64 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
> @@ -207,11 +207,62 @@ AllocateAcpiCpuData (
> VOID
> )
> {
> + EFI_STATUS Status;
> + UINTN NumberOfCpus;
> + UINTN NumberOfEnabledProcessors;
> + ACPI_CPU_DATA *AcpiCpuData;
> + EFI_PHYSICAL_ADDRESS Address;
> + UINTN TableSize;
> + CPU_REGISTER_TABLE *RegisterTable;
> + UINTN Index;
> + EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
> +
> + Address = BASE_4GB - 1;
> + Status = gBS->AllocatePages (
> + AllocateMaxAddress,
> + EfiACPIMemoryNVS,
> + EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)),
> + &Address
> + );
> + ASSERT_EFI_ERROR (Status);
I understand that this code is for IA32 and X64 only, but still, Ard has
recently introduced a DxeServicesLib API for this kind of allocation:
it's called AllocatePeiAccessiblePages(). See commit a40e0b7aa918.
(4) Is it feasible to use that function here (and in the second instance
below)?
From a library dependency standpoint, I think it should be fine: we are
modifying the DXE instance of the RegisterCpuFeaturesLib class, so a
dependency on DxeServicesLib should be in order.
Now, if this allocation *must* be satisfied from below 4GB, even if the
PEI phase has access to >=4GB RAM (as determined by
PhitHob->EfiFreeMemoryTop > MAX_UINT32
), then my suggestion is wrong (because in that case,
AllocatePeiAccessiblePages() wouldn't restrict the allocation under
4GB).
CC'ing Ard too.
Thanks!
Laszlo
> + AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address;
> + ASSERT (AcpiCpuData != NULL);
> +
> + GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
> + AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus;
> +
> //
> - // CpuS3DataDxe will do it.
> + // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> //
> - ASSERT (FALSE);
> - return NULL;
> + TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> + Address = BASE_4GB - 1;
> + Status = gBS->AllocatePages (
> + AllocateMaxAddress,
> + EfiACPIMemoryNVS,
> + EFI_SIZE_TO_PAGES (TableSize),
> + &Address
> + );
> + ASSERT_EFI_ERROR (Status);
> + RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address;
> +
> + for (Index = 0; Index < NumberOfCpus; Index++) {
> + Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
> + ASSERT_EFI_ERROR (Status);
> +
> + RegisterTable[Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
> + RegisterTable[Index].TableLength = 0;
> + RegisterTable[Index].AllocatedSize = 0;
> + RegisterTable[Index].RegisterTableEntry = 0;
> +
> + RegisterTable[NumberOfCpus + Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
> + RegisterTable[NumberOfCpus + Index].TableLength = 0;
> + RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
> + RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
> + }
> + AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> + AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> +
> + return AcpiCpuData;
> }
>
> /**
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, August 4, 2018 12:46 AM
> To: Dong, Eric <eric.dong@intel.com>
> Cc: edk2-devel@lists.01.org; Ni, Ruiyu <ruiyu.ni@intel.com>; Marvin Häuser
> <Marvin.Haeuser@outlook.com>; Jeff Fan <vanjeff_919@hotmail.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement
> AllocateAcpiCpuData function.
>
> Hello Eric,
>
> OVMF does not include CpuFeaturesPei / CpuFeaturesDxe, and so it doesn't
> consume this library. I can't provide test results, but I have some superficial
> comments.
>
> First, I'm adding Marvin and Jeff -- I *vaguely* recall that this issue may have
> been raised by Marvin. Hm... yes, it seems so:
>
> [edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.
>
> http://mid.mail-
> archive.com/VI1PR0801MB17908DB1F03A3C5F84545E1080690@VI1PR0801
> MB1790.eurprd08.prod.outlook.com
> https://lists.01.org/pipermail/edk2-devel/2018-May/025163.html
>
> I have three questions here:
>
> (1) Do we have a TianoCore BZ about this?
>
> (2) If not, does the currently proposed commit message capture the issue in
> enough detail? Should we reference Marvin's initial report and/or a
> TianoCore BZ (if we have one)?
>
Yes, it has an BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=959
I will include this info in my next version patch.
> (3) The implementation seems to follow Jeff's idea. Marvin and Jeff, can you
> please help with the review?
>
> Either way, I propose the following two tags to be appended:
>
> Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
>
Sure, will include them in my next version patch.
>
> And one technical question follows below:
>
> On 08/03/18 04:10, Eric Dong wrote:
> > Current code logic can't confirm CpuS3DataDxe driver start before
> > CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
> > Add implementation for AllocateAcpiCpuData function to remove this
> > assumption.
> >
> > Pass OS boot and resume from S3 test.
> >
> > 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>
> > ---
> > .../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++--
> > 1 file changed, 54 insertions(+), 3 deletions(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.
> > c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.
> > c
> > index 902a339529..0722db6c64 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.
> > c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeatures
> > +++ Lib.c
> > @@ -207,11 +207,62 @@ AllocateAcpiCpuData (
> > VOID
> > )
> > {
> > + EFI_STATUS Status;
> > + UINTN NumberOfCpus;
> > + UINTN NumberOfEnabledProcessors;
> > + ACPI_CPU_DATA *AcpiCpuData;
> > + EFI_PHYSICAL_ADDRESS Address;
> > + UINTN TableSize;
> > + CPU_REGISTER_TABLE *RegisterTable;
> > + UINTN Index;
> > + EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
> > +
> > + Address = BASE_4GB - 1;
> > + Status = gBS->AllocatePages (
> > + AllocateMaxAddress,
> > + EfiACPIMemoryNVS,
> > + EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)),
> > + &Address
> > + );
> > + ASSERT_EFI_ERROR (Status);
>
> I understand that this code is for IA32 and X64 only, but still, Ard has recently
> introduced a DxeServicesLib API for this kind of allocation:
> it's called AllocatePeiAccessiblePages(). See commit a40e0b7aa918.
>
> (4) Is it feasible to use that function here (and in the second instance below)?
>
After check the code, I found current usage modal not required it below 4G, also it doesn't need ACPI NVS type memory.
I will update it in my next version change.
> From a library dependency standpoint, I think it should be fine: we are
> modifying the DXE instance of the RegisterCpuFeaturesLib class, so a
> dependency on DxeServicesLib should be in order.
>
> Now, if this allocation *must* be satisfied from below 4GB, even if the PEI
> phase has access to >=4GB RAM (as determined by
>
> PhitHob->EfiFreeMemoryTop > MAX_UINT32
>
> ), then my suggestion is wrong (because in that case,
> AllocatePeiAccessiblePages() wouldn't restrict the allocation under 4GB).
>
> CC'ing Ard too.
>
> Thanks!
> Laszlo
>
> > + AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address; ASSERT
> > + (AcpiCpuData != NULL);
> > +
> > + GetNumberOfProcessor (&NumberOfCpus,
> &NumberOfEnabledProcessors);
> > + AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus;
> > +
> > //
> > - // CpuS3DataDxe will do it.
> > + // Allocate buffer for empty RegisterTable and
> > + PreSmmInitRegisterTable for all CPUs
> > //
> > - ASSERT (FALSE);
> > - return NULL;
> > + TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > + Address = BASE_4GB - 1; Status = gBS->AllocatePages (
> > + AllocateMaxAddress,
> > + EfiACPIMemoryNVS,
> > + EFI_SIZE_TO_PAGES (TableSize),
> > + &Address
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > + RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address;
> > +
> > + for (Index = 0; Index < NumberOfCpus; Index++) {
> > + Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
> > + ASSERT_EFI_ERROR (Status);
> > +
> > + RegisterTable[Index].InitialApicId =
> (UINT32)ProcessorInfoBuffer.ProcessorId;
> > + RegisterTable[Index].TableLength = 0;
> > + RegisterTable[Index].AllocatedSize = 0;
> > + RegisterTable[Index].RegisterTableEntry = 0;
> > +
> > + RegisterTable[NumberOfCpus + Index].InitialApicId =
> (UINT32)ProcessorInfoBuffer.ProcessorId;
> > + RegisterTable[NumberOfCpus + Index].TableLength = 0;
> > + RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
> > + RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0; }
> > + AcpiCpuData->RegisterTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> > + AcpiCpuData->PreSmmInitRegisterTable =
> > + (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> > +
> > + return AcpiCpuData;
> > }
> >
> > /**
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.