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

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,



> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, August 9, 2018 12:55 AM
> To: Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
> 
> 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.
> 

Yes, will remove it in my next version patch.

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

Agree, will use AllocateZeroPages.

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

Agree, will do it.

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

I searched the code base and not found any code needs to restore the original AcpiNVS memory.

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

I will add a separate patch to remove the memory type and below 4G limitation in the header file.

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

Agree, will update the comments.

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

Agree, will create a separate patch for this issue.

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

Agree this is a bug, will create a separate patch to fix this issue.

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

We have reach the agreement that this is a "Not a correctly used" code bug, and it is caused by not do the related code clean when enable new features.  So I prefer to do this clean up to make code more clean.

> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel