Re: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Laszlo Ersek <lersek@redhat.com>]

Hello Eric,

On 08/08/18 09:40, Eric Dong wrote:
> Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
> so 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.
>
> Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959
>
> Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>

(1) I believe these tags don't apply to this patch -- this patch seems
to be a new idea / addition.


> 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      | 60 +++++++-------------------------
>  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 +
>  2 files changed, 14 insertions(+), 47 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..d8eb8c976f 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 <Library/UefiBootServicesTableLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MtrrLib.h>
> +#include <Library/MemoryAllocationLib.h>
>
>  #include <Protocol/MpService.h>
>  #include <Guid/EventGroup.h>
> @@ -45,42 +46,6 @@ typedef struct {
>    IA32_DESCRIPTOR           IdtrProfile;
>  } ACPI_CPU_DATA_EX;
>
> -/**
> -  Allocate EfiACPIMemoryNVS below 4G memory address.
> -
> -  This function allocates EfiACPIMemoryNVS below 4G memory address.
> -
> -  @param[in] Size   Size of memory to allocate.
> -
> -  @return       Allocated address for output.
> -
> -**/
> -VOID *
> -AllocateAcpiNvsMemoryBelow4G (
> -  IN UINTN  Size
> -  )
> -{
> -  EFI_PHYSICAL_ADDRESS  Address;
> -  EFI_STATUS            Status;
> -  VOID                  *Buffer;
> -
> -  Address = BASE_4GB - 1;
> -  Status  = gBS->AllocatePages (
> -                   AllocateMaxAddress,
> -                   EfiACPIMemoryNVS,
> -                   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.
>
> @@ -150,7 +115,6 @@ CpuS3DataInitialize (
>    EFI_MP_SERVICES_PROTOCOL   *MpServices;
>    UINTN                      NumberOfCpus;
>    UINTN                      NumberOfEnabledProcessors;
> -  VOID                       *Stack;
>    UINTN                      TableSize;
>    CPU_REGISTER_TABLE         *RegisterTable;
>    UINTN                      Index;
> @@ -171,10 +135,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 = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA_EX)));

(2) In the original AllocateAcpiNvsMemoryBelow4G() function, we have a
ZeroMem() call. For compatibility, I think we should call
AllocateZeroPages() here.

(Previously, the trailing portion of the last page may not have been
zeroed, but it's not a problem to zero more than before.)


(3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that
ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from
memory of type EfiACPIMemoryNVS".

If we have determined that this is no longer necessary, then:
- we should first update the documentation in "AcpiCpuData.h" first,
- the commit message should carefully explain why the change is valid.

In particular, my concern is not about the normal boot path. (The normal
boot path is explained for example in the message of commit
92b87f1c8c0b, "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE",
2015-11-30.) I agree that copying from BootServicesData type memory into
SMRAM is fine, during normal boot.

Instead, my concern is with the S3 resume path, when the data from the
SMRAM buffer is *restored*. I seem to recall a case when the data was
first restored from SMRAM to the original AcpiNVS allocation. If we make
that area BootServicesData now, then the OS will reuse it, and then at
S3 resume, we overwrite OS data.

... After reviewing the GetAcpiCpuData() function in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change,
because GetAcpiCpuData() never remembers the address of the
ACPI_CPU_DATA structure, or the addresses of its fields.

However, I would like Mike to review this as well (CC'ing him).


(4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and
IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of
those fields into BootServicesData type memory. In turn, the
EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same names
will point into BootServicesData type memory.

This conflicts with the requirements that are documented in
"UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable,
GdtrProfile and IdtrProfile.

If we want to change the allocation of these objects (of type
MTRR_SETTINGS, IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively), then
we should audit their use during S3 resume in the PiSmmCpuDxeSmm driver,
and document them one-by-one.

... Based on GetAcpiCpuData() again, I *think* this is valid change,
functionally speaking, but I'd like Mike to review as well.


>    ASSERT (AcpiCpuDataEx != NULL);
>    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
>
> @@ -210,11 +171,16 @@ CpuS3DataInitialize (
>    AcpiCpuData->MtrrTable    = (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
>
>    //
> -  // Allocate stack space for all CPUs
> +  // Allocate stack space for all CPUs, use ACPI NVS memory type because it will
> +  // not copy to smram at Smm ready to lock point.
>    //
> -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus * AcpiCpuData->StackSize);
> -  ASSERT (Stack != NULL);
> -  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> +  Status  = gBS->AllocatePages (
> +                   AllocateAnyPages,
> +                   EfiACPIMemoryNVS,
> +                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData->StackSize),
> +                   &AcpiCpuData->StackAddress
> +                   );
> +  ASSERT_EFI_ERROR (Status);

(5) The leading comment should be clarified. We should state that during
S3 resume, the stack will only be used as scratch space, i.e. we won't
read anything from it before we write to it, in PiSmmCpuDxeSemm.
Otherwise, it would be a security bug to keep the area in AcpiNVS and
not in SMRAM.


(6) This change requires a separate investigation from the rest of the
patch, so it should be in a separate patch.

That implies we should first change this call site, and then remove
AllocateAcpiNvsMemoryBelow4G() in a final patch.


(7) Why is it OK to lift the 4GB limitation? ... I think it may be valid
indeed, because PrepareApStartupVector() stores StackAddress to
"mExchangeInfo->StackStart" (which has type (VOID*)), and because
"UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:

        add  edi, StackStartAddressLocation
        add  rax, qword [edi]
        mov  rsp, rax
        mov  qword [edi], rax

in long-mode code. But, again, this should be documented in the
(separate) patch's commit message.


(8) The change breaks the documentation on "ACPI_CPU_DATA.StackAddress",
in "UefiCpuPkg/Include/AcpiCpuData.h". The documentation should be
updated.


>
>    //
>    // Get the boot processor's GDT and IDT
> @@ -227,7 +193,7 @@ CpuS3DataInitialize (
>    //
>    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
>    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> +  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
>    ASSERT (Gdt != NULL);
>    Idt = (VOID *)((UINTN)Gdt + GdtSize);
>    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);

(9) This change breaks the requirements in
"UefiCpuPkg/Include/AcpiCpuData.h" that both
"ACPI_CPU_DATA.GdtrProfile->Base" and "ACPI_CPU_DATA.IdtrProfile->Base"
point into AcpiNVS data.

("The buffer for GDT must also be allocated below 4GB from memory of
type EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated
below 4GB from memory of type EfiACPIMemoryNVS".)


(10) And, indeed, this is the bug that I suspected in point (3), with
regard to the S3 resume path. Namely:


Please consider the GetAcpiCpuData() function in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function:

(10a) copies the IDT and GDT *descriptors* into dynamically allocated
SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and
"mAcpiCpuData.IdtrProfile" fields;

(10b) copies the IDT and GDT themselves into dynamically allocated
SMRAM, pointed-to by the "mGdtForAp" and "mIdtForAp" global variables,

(10c) *does not* update the "Base" members of the IDT and GDT
descriptors that are saved in SMRAM in step (10a). Those continue to
point to memory that was allocated by CpuS3DataDxe in the
CpuS3DataInitialize() function.


Now, consider the PrepareApStartupVector() function in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume. That
function:

(10d) copies the IDT and GDT descriptors that were saved in (10a) into
"mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the Base
members of the descriptors point to the original CpuS3DataDxe
allocation,

(10e) the IDT and the GDT are themselves restored from step (10b), i.e.
from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the
original CpuS3DataDxe allocation address. The comment says:

>   //
>   // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI NVS memory
>   //

In other words, while the IDT and GDT *descriptors* aren't restored
in-place, the IDT and GDT themselves are.

This means that the above code change causes PiSmmCpuDxeSmm to overwrite
OS memory at S3 resume.


Continuing:

On 08/08/18 09:40, Eric Dong wrote:
> @@ -243,7 +209,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 = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
>      ASSERT (RegisterTable != NULL);
>
>      for (Index = 0; Index < NumberOfCpus; Index++) {

(11) This looks valid, beyond breaking the documentation in
"UefiCpuPkg/Include/AcpiCpuData.h", of the members
"PreSmmInitRegisterTable" and "RegisterTable".


> 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
>

I feel uncomfortable about this patch. It is very tricky to review, and
if we make mistakes, we could corrupt OS data, or (worse) perhaps even
compromise SMRAM.

And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS
pages". I haven't measured, but it doesn't look like an attractive
trade-off to me. And, platforms that disable S3 via "PcdAcpiS3Enable"
don't allocate this memory anyway.

Furthermore, the memory footprint optimization does not seem related to
the CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that
Marvin reported (and that is tracked in bug #959).

I suggest that we drop this patch.

Thanks,
Laszlo