Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
Hi, Got your points now. I think we can update the code when new library API added. Thanks, Eric > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Marvin Häuser > Sent: Thursday, August 16, 2018 9:00 PM > To: edk2-devel@lists.01.org; Laszlo Ersek ; Dong, Eric > > Cc: Ni, Ruiyu > Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change > Memory Type and address limitation. > > Comments inline. > > Thanks and best regards, > Marvin. > > > -Original Message- > > From: Laszlo Ersek > > Sent: Thursday, August 16, 2018 2:31 PM > > To: Dong, Eric ; Marvin Häuser > > ; edk2-devel@lists.01.org > > Cc: Ni, Ruiyu > > Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change > > Memory Type and address limitation. > > > > On 08/16/18 02:56, Dong, Eric wrote: > > > Hi Marvin & Laszlo, > > > > > > I'm not very clear about the risk to use this function name. I think > > > it is just > > used in a driver as an internal function, other drivers or libraries > > can't use it. I think we don't need to add internal prefix to all > > internal functions in drivers, maybe need for the libraries, right? > > Here we need to add internal prefix just because it has similar name with > other common API? > > > > If I understood correctly, there were two points to Marvin's argument: > > > > - AllocateZeroPages() is the most likely function name that > > "MemoryAllocationLib.h" would add, *if* it ever introduced a function > > for "allocating boot service data pages, plus zeroing them". In that > > case, it would cause a conflict. > > Correct > > > > > - Second, because the function is defined in the same translation unit > > where it is called from (and there are no other callers), we can make > > the function STATIC. > > Pretty much, but it was tied to the first point. There are many functions that > could be static but aren't in edk2, so this isn't significant itself. I > mentioned it > due to my personal naming convention to ensure uniqueness. > > > > > Regarding the first concern, I don't think it's a very practical one. > > I'm neutral on the question. My point is only that, if we really want > > to change the name, I think we should do it separately / incrementally. > > If it's supposed to be done separately, I don't see a point in fixing it > either, it > can still be fixed if such an API is ever introduced. It was meant as a > "preventive" suggestion to be included in this series. "Just in case" > > > > > Regarding the second idea, STATIC is a generally good practice, and we > > should do that everywhere we can. But, because I don't want to re-test > > / re- review this series after all this effort, I suggest we do the > > STATIC thing incrementally as well. > > +1, but that's not worth an own patch to be honest. I should see whether there > is some static analyzer that has checks for "can be static" some day. > > > > > Thanks > > Laszlo > > > > > > > > > > Thanks, > > > Eric > > > > > >> -Original Message- > > >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > > >> Of Laszlo Ersek > > >> Sent: Wednesday, August 15, 2018 11:30 PM > > >> To: Marvin Häuser ; > > >> edk2-devel@lists.01.org; Dong, Eric > > >> Cc: Ni, Ruiyu > > >> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change > > >> Memory Type and address limitation. > > >> > > >> On 08/15/18 15:12, Marvin Häuser wrote: > > >>> Hey Eric and anyone CC'd, > > >>> > > >>> Are you sure you want to name the function "AllocateZeroPages"? > > >>> It's analogous to "AllocateZeroPool", so I could see it becoming > > >>> an API function at some point, which will conflict with this > > >>> definition and might silently break UefiCpuPkg compilation if not > > >>> tested before upstreaming. I usually make any module's private > > >>> functions static and prefix "Internal" if possible, or, if static > > >>> cannot be used, non-static plus prefix something derived from the > > >>> module's name to achieve uniqueness. If I am not mistaken, this > > >>> could be made static, couldn't it? > > >> > > >> I agree that the function's name is not op
Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
Comments inline. Thanks and best regards, Marvin. > -Original Message- > From: Laszlo Ersek > Sent: Thursday, August 16, 2018 2:31 PM > To: Dong, Eric ; Marvin Häuser > ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change > Memory Type and address limitation. > > On 08/16/18 02:56, Dong, Eric wrote: > > Hi Marvin & Laszlo, > > > > I'm not very clear about the risk to use this function name. I think it is > > just > used in a driver as an internal function, other drivers or libraries can't > use it. I > think we don't need to add internal prefix to all internal functions in > drivers, > maybe need for the libraries, right? Here we need to add internal prefix just > because it has similar name with other common API? > > If I understood correctly, there were two points to Marvin's argument: > > - AllocateZeroPages() is the most likely function name that > "MemoryAllocationLib.h" would add, *if* it ever introduced a function for > "allocating boot service data pages, plus zeroing them". In that case, it > would > cause a conflict. Correct > > - Second, because the function is defined in the same translation unit where > it is called from (and there are no other callers), we can make the function > STATIC. Pretty much, but it was tied to the first point. There are many functions that could be static but aren't in edk2, so this isn't significant itself. I mentioned it due to my personal naming convention to ensure uniqueness. > > Regarding the first concern, I don't think it's a very practical one. > I'm neutral on the question. My point is only that, if we really want to > change > the name, I think we should do it separately / incrementally. If it's supposed to be done separately, I don't see a point in fixing it either, it can still be fixed if such an API is ever introduced. It was meant as a "preventive" suggestion to be included in this series. "Just in case" > > Regarding the second idea, STATIC is a generally good practice, and we > should do that everywhere we can. But, because I don't want to re-test / re- > review this series after all this effort, I suggest we do the STATIC thing > incrementally as well. +1, but that's not worth an own patch to be honest. I should see whether there is some static analyzer that has checks for "can be static" some day. > > Thanks > Laszlo > > > > > > Thanks, > > Eric > > > >> -Original Message- > >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > >> Of Laszlo Ersek > >> Sent: Wednesday, August 15, 2018 11:30 PM > >> To: Marvin Häuser ; > >> edk2-devel@lists.01.org; Dong, Eric > >> Cc: Ni, Ruiyu > >> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change > >> Memory Type and address limitation. > >> > >> On 08/15/18 15:12, Marvin Häuser wrote: > >>> Hey Eric and anyone CC'd, > >>> > >>> Are you sure you want to name the function "AllocateZeroPages"? It's > >>> analogous to "AllocateZeroPool", so I could see it becoming an API > >>> function at some point, which will conflict with this definition and > >>> might silently break UefiCpuPkg compilation if not tested before > >>> upstreaming. I usually make any module's private functions static > >>> and prefix "Internal" if possible, or, if static cannot be used, > >>> non-static plus prefix something derived from the module's name to > >>> achieve uniqueness. If I am not mistaken, this could be made static, > >>> couldn't it? > >> > >> I agree that the function's name is not optimal, primarily because > >> the > >> Allocate*Pages() functions tend to take a page count, not a byte count. > >> However, I didn't want to ask for another version just because of > >> this; a lot of review (and now testing) has gone into this set, and this is > just a wart. > >> > >> I suggest that -- after the stable tag -- we push v4 as-is; however, > >> Marvin, please go ahead and file a TianoCore BZ that depends on 959 > >> (i.e. on the BZ currently referenced in patch #5), about fixing up > >> the function name (and about making it static). > >> > >> Note that an "AllocateZeroPages" function exists in > >> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. > >> I guess both functions should be renamed (and likely not to the same > >> new name, because they have different parameter lists). And, only the > >> UefiCpuPkg one can be made static however. Either way, both packages > >> could be covered by the same BZ. > > > > > >> > >> 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
Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
On 08/16/18 02:56, Dong, Eric wrote: > Hi Marvin & Laszlo, > > I'm not very clear about the risk to use this function name. I think it is > just used in a driver as an internal function, other drivers or libraries > can't use it. I think we don't need to add internal prefix to all internal > functions in drivers, maybe need for the libraries, right? Here we need to > add internal prefix just because it has similar name with other common API? If I understood correctly, there were two points to Marvin's argument: - AllocateZeroPages() is the most likely function name that "MemoryAllocationLib.h" would add, *if* it ever introduced a function for "allocating boot service data pages, plus zeroing them". In that case, it would cause a conflict. - Second, because the function is defined in the same translation unit where it is called from (and there are no other callers), we can make the function STATIC. Regarding the first concern, I don't think it's a very practical one. I'm neutral on the question. My point is only that, if we really want to change the name, I think we should do it separately / incrementally. Regarding the second idea, STATIC is a generally good practice, and we should do that everywhere we can. But, because I don't want to re-test / re-review this series after all this effort, I suggest we do the STATIC thing incrementally as well. Thanks Laszlo > > Thanks, > Eric > >> -Original Message- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, August 15, 2018 11:30 PM >> To: Marvin Häuser ; edk2-devel@lists.01.org; >> Dong, Eric >> Cc: Ni, Ruiyu >> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change >> Memory Type and address limitation. >> >> On 08/15/18 15:12, Marvin Häuser wrote: >>> Hey Eric and anyone CC'd, >>> >>> Are you sure you want to name the function "AllocateZeroPages"? It's >>> analogous to "AllocateZeroPool", so I could see it becoming an API >>> function at some point, which will conflict with this definition and >>> might silently break UefiCpuPkg compilation if not tested before >>> upstreaming. I usually make any module's private functions static and >>> prefix "Internal" if possible, or, if static cannot be used, >>> non-static plus prefix something derived from the module's name to >>> achieve uniqueness. If I am not mistaken, this could be made static, >>> couldn't it? >> >> I agree that the function's name is not optimal, primarily because the >> Allocate*Pages() functions tend to take a page count, not a byte count. >> However, I didn't want to ask for another version just because of this; a >> lot of >> review (and now testing) has gone into this set, and this is just a wart. >> >> I suggest that -- after the stable tag -- we push v4 as-is; however, Marvin, >> please go ahead and file a TianoCore BZ that depends on 959 (i.e. on the BZ >> currently referenced in patch #5), about fixing up the function name (and >> about making it static). >> >> Note that an "AllocateZeroPages" function exists in >> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. I guess >> both functions should be renamed (and likely not to the same new name, >> because they have different parameter lists). And, only the UefiCpuPkg one >> can be made static however. Either way, both packages could be covered by >> the same BZ. > > >> >> 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
Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
Hi Marvin & Laszlo, I'm not very clear about the risk to use this function name. I think it is just used in a driver as an internal function, other drivers or libraries can't use it. I think we don't need to add internal prefix to all internal functions in drivers, maybe need for the libraries, right? Here we need to add internal prefix just because it has similar name with other common API? Thanks, Eric > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Wednesday, August 15, 2018 11:30 PM > To: Marvin Häuser ; edk2-devel@lists.01.org; > Dong, Eric > Cc: Ni, Ruiyu > Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change > Memory Type and address limitation. > > On 08/15/18 15:12, Marvin Häuser wrote: > > Hey Eric and anyone CC'd, > > > > Are you sure you want to name the function "AllocateZeroPages"? It's > > analogous to "AllocateZeroPool", so I could see it becoming an API > > function at some point, which will conflict with this definition and > > might silently break UefiCpuPkg compilation if not tested before > > upstreaming. I usually make any module's private functions static and > > prefix "Internal" if possible, or, if static cannot be used, > > non-static plus prefix something derived from the module's name to > > achieve uniqueness. If I am not mistaken, this could be made static, > > couldn't it? > > I agree that the function's name is not optimal, primarily because the > Allocate*Pages() functions tend to take a page count, not a byte count. > However, I didn't want to ask for another version just because of this; a lot > of > review (and now testing) has gone into this set, and this is just a wart. > > I suggest that -- after the stable tag -- we push v4 as-is; however, Marvin, > please go ahead and file a TianoCore BZ that depends on 959 (i.e. on the BZ > currently referenced in patch #5), about fixing up the function name (and > about making it static). > > Note that an "AllocateZeroPages" function exists in > "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. I guess > both functions should be renamed (and likely not to the same new name, > because they have different parameter lists). And, only the UefiCpuPkg one > can be made static however. Either way, both packages could be covered by > the same BZ. > > 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
Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
On 08/15/18 15:12, Marvin Häuser wrote: > Hey Eric and anyone CC'd, > > Are you sure you want to name the function "AllocateZeroPages"? It's > analogous to "AllocateZeroPool", so I could see it becoming an API > function at some point, which will conflict with this definition and > might silently break UefiCpuPkg compilation if not tested before > upstreaming. I usually make any module's private functions static and > prefix "Internal" if possible, or, if static cannot be used, > non-static plus prefix something derived from the module's name to > achieve uniqueness. If I am not mistaken, this could be made static, > couldn't it? I agree that the function's name is not optimal, primarily because the Allocate*Pages() functions tend to take a page count, not a byte count. However, I didn't want to ask for another version just because of this; a lot of review (and now testing) has gone into this set, and this is just a wart. I suggest that -- after the stable tag -- we push v4 as-is; however, Marvin, please go ahead and file a TianoCore BZ that depends on 959 (i.e. on the BZ currently referenced in patch #5), about fixing up the function name (and about making it static). Note that an "AllocateZeroPages" function exists in "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. I guess both functions should be renamed (and likely not to the same new name, because they have different parameter lists). And, only the UefiCpuPkg one can be made static however. Either way, both packages could be covered by the same BZ. Thanks Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
Hey Eric and anyone CC'd,
Are you sure you want to name the function "AllocateZeroPages"? It's analogous
to "AllocateZeroPool", so I could see it becoming an API function at some
point, which will conflict with this definition and might silently break
UefiCpuPkg compilation if not tested before upstreaming. I usually make any
module's private functions static and prefix "Internal" if possible, or, if
static cannot be used, non-static plus prefix something derived from the
module's name to achieve uniqueness. If I am not mistaken, this could be made
static, couldn't it?
Thanks,
Marvin
> -Original Message-
> From: Eric Dong
> Sent: Wednesday, August 15, 2018 4:15 AM
> To: edk2-devel@lists.01.org
> Cc: Marvin Häuser ; Fan Jeff
> ; Laszlo Ersek ; Ruiyu Ni
>
> Subject: [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type
> and address limitation.
>
> 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.
>
> V4 Changes:
> 1. Create AllocateZeroPages and use it. It's easy to use than gBS-
> >AllocatePages.
>
> Pass OS boot and resume from S3 test.
>
> Cc: Marvin Häuser
> Cc: Fan Jeff
> Cc: Laszlo Ersek
> Cc: Ruiyu Ni
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong
> ---
> UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 34
> +---
> UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 +
> 2 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..3e8c8b383c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -81,6 +82,28 @@ AllocateAcpiNvsMemoryBelow4G (
>return Buffer;
> }
>
> +/**
> + Allocate memory and clean it with zero.
> +
> + @param[in] Size Size of memory to allocate.
> +
> + @return Allocated address for output.
> +
> +**/
> +VOID *
> +AllocateZeroPages (
> + IN UINTN Size
> + )
> +{
> + VOID *Buffer;
> +
> + Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size)); if (Buffer !=
> + NULL) {
> +ZeroMem (Buffer, Size);
> + }
> +
> + return Buffer;
> +}
> /**
>Callback function executed when the EndOfDxe event group is signaled.
>
> @@ -171,10 +194,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 = AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX));
>ASSERT (AcpiCpuDataEx != NULL);
>AcpiCpuData = >AcpiCpuData;
>
> @@ -223,11 +243,11 @@ CpuS3DataInitialize (
>AsmReadIdtr (>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 = AllocateZeroPages (GdtSize + IdtSize);
>ASSERT (Gdt != NULL);
>Idt = (VOID *)((UINTN)Gdt + GdtSize);
>CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize); @@ -
> 243,7 +263,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 *)AllocateZeroPages
> + (TableSize);
> ASSERT (RegisterTable != NULL);
>
> for (Index = 0; Index < NumberOfCpus; Index++) { diff --git
> a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index 480c98ebcd..c16731529c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -51,6 +51,7 @@
>DebugLib
>BaseLib
>MtrrLib
> + MemoryAllocationLib
>
> [Guids]
>gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event
> --
> 2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
On 08/15/18 04:14, 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.
>
> V4 Changes:
> 1. Create AllocateZeroPages and use it. It's easy to use than
> gBS->AllocatePages.
>
> Pass OS boot and resume from S3 test.
>
> Cc: Marvin Häuser
> Cc: Fan Jeff
> Cc: Laszlo Ersek
> Cc: Ruiyu Ni
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong
> ---
> UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 34
> +---
> UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 +
> 2 files changed, 28 insertions(+), 7 deletions(-)
Reviewed-by: Laszlo Ersek
Thanks,
Laszlo
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..3e8c8b383c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -81,6 +82,28 @@ AllocateAcpiNvsMemoryBelow4G (
>return Buffer;
> }
>
> +/**
> + Allocate memory and clean it with zero.
> +
> + @param[in] Size Size of memory to allocate.
> +
> + @return Allocated address for output.
> +
> +**/
> +VOID *
> +AllocateZeroPages (
> + IN UINTN Size
> + )
> +{
> + VOID *Buffer;
> +
> + Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
> + if (Buffer != NULL) {
> +ZeroMem (Buffer, Size);
> + }
> +
> + return Buffer;
> +}
> /**
>Callback function executed when the EndOfDxe event group is signaled.
>
> @@ -171,10 +194,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 = AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX));
>ASSERT (AcpiCpuDataEx != NULL);
>AcpiCpuData = >AcpiCpuData;
>
> @@ -223,11 +243,11 @@ CpuS3DataInitialize (
>AsmReadIdtr (>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 = AllocateZeroPages (GdtSize + IdtSize);
>ASSERT (Gdt != NULL);
>Idt = (VOID *)((UINTN)Gdt + GdtSize);
>CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
> @@ -243,7 +263,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 *)AllocateZeroPages (TableSize);
> ASSERT (RegisterTable != NULL);
>
> for (Index = 0; Index < NumberOfCpus; Index++) {
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index 480c98ebcd..c16731529c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -51,6 +51,7 @@
>DebugLib
>BaseLib
>MtrrLib
> + MemoryAllocationLib
>
> [Guids]
>gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
Reviewed-by: Ruiyu Ni
Thanks/Ray
> -Original Message-
> From: edk2-devel On Behalf Of Eric Dong
> Sent: Wednesday, August 15, 2018 10:15 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek ; Ni, Ruiyu
> Subject: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory
> Type and address limitation.
>
> 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.
>
> V4 Changes:
> 1. Create AllocateZeroPages and use it. It's easy to use than
> gBS->AllocatePages.
>
> Pass OS boot and resume from S3 test.
>
> Cc: Marvin Häuser
> Cc: Fan Jeff
> Cc: Laszlo Ersek
> Cc: Ruiyu Ni
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong
> ---
> UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 34
> +---
> UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 +
> 2 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..3e8c8b383c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -81,6 +82,28 @@ AllocateAcpiNvsMemoryBelow4G (
>return Buffer;
> }
>
> +/**
> + Allocate memory and clean it with zero.
> +
> + @param[in] Size Size of memory to allocate.
> +
> + @return Allocated address for output.
> +
> +**/
> +VOID *
> +AllocateZeroPages (
> + IN UINTN Size
> + )
> +{
> + VOID *Buffer;
> +
> + Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size)); if (Buffer !=
> + NULL) {
> +ZeroMem (Buffer, Size);
> + }
> +
> + return Buffer;
> +}
> /**
>Callback function executed when the EndOfDxe event group is signaled.
>
> @@ -171,10 +194,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 = AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX));
>ASSERT (AcpiCpuDataEx != NULL);
>AcpiCpuData = >AcpiCpuData;
>
> @@ -223,11 +243,11 @@ CpuS3DataInitialize (
>AsmReadIdtr (>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 = AllocateZeroPages (GdtSize + IdtSize);
>ASSERT (Gdt != NULL);
>Idt = (VOID *)((UINTN)Gdt + GdtSize);
>CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize); @@ -
> 243,7 +263,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 *)AllocateZeroPages
> + (TableSize);
> ASSERT (RegisterTable != NULL);
>
> for (Index = 0; Index < NumberOfCpus; Index++) { diff --git
> a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index 480c98ebcd..c16731529c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -51,6 +51,7 @@
>DebugLib
>BaseLib
>MtrrLib
> + MemoryAllocationLib
>
> [Guids]
>gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event
> --
> 2.15.0.windows.1
>
> ___
> 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
[edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
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.
V4 Changes:
1. Create AllocateZeroPages and use it. It's easy to use than
gBS->AllocatePages.
Pass OS boot and resume from S3 test.
Cc: Marvin Häuser
Cc: Fan Jeff
Cc: Laszlo Ersek
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 34 +---
UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 +
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
index dccb406b8d..3e8c8b383c 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
#include
#include
#include
+#include
#include
#include
@@ -81,6 +82,28 @@ AllocateAcpiNvsMemoryBelow4G (
return Buffer;
}
+/**
+ Allocate memory and clean it with zero.
+
+ @param[in] Size Size of memory to allocate.
+
+ @return Allocated address for output.
+
+**/
+VOID *
+AllocateZeroPages (
+ IN UINTN Size
+ )
+{
+ VOID *Buffer;
+
+ Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
+ if (Buffer != NULL) {
+ZeroMem (Buffer, Size);
+ }
+
+ return Buffer;
+}
/**
Callback function executed when the EndOfDxe event group is signaled.
@@ -171,10 +194,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 = AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX));
ASSERT (AcpiCpuDataEx != NULL);
AcpiCpuData = >AcpiCpuData;
@@ -223,11 +243,11 @@ CpuS3DataInitialize (
AsmReadIdtr (>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 = AllocateZeroPages (GdtSize + IdtSize);
ASSERT (Gdt != NULL);
Idt = (VOID *)((UINTN)Gdt + GdtSize);
CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
@@ -243,7 +263,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 *)AllocateZeroPages (TableSize);
ASSERT (RegisterTable != NULL);
for (Index = 0; Index < NumberOfCpus; Index++) {
diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index 480c98ebcd..c16731529c 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -51,6 +51,7 @@
DebugLib
BaseLib
MtrrLib
+ MemoryAllocationLib
[Guids]
gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event
--
2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
