Re: [edk2-devel] [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Hi Gerd,

There was the issue in my patch to change the smm access driver: SmmAccessPeiOpen(), I removed below code due to the comment in original code that indicate the DescriptorIndex is not considered at all:

  ...
  if (DescriptorIndex >= DescIdxCount) {
    return EFI_INVALID_PARAMETER;
  }
  //
  // According to current practice, *DescriptorIndex is not considered at all*,
  // beyond validating it.
  //
  ...

But it's important for smmlockboxpeilib to check the return status of SmmAccessPeiOpen (EFI_INVALID_PARAMETER) to continue the RestoreLockBox():
    for (Index = 0; !EFI_ERROR (Status); Index++) {
      Status = SmmAccess->Open ((EFI_PEI_SERVICES **)GetPeiServicesTablePointer (), SmmAccess, Index);
    }

So, it hangs at for() loop once I removed above code in the SmmAccessPeiOpen!!!

After that fix, I still found S3 doesn't work, I checked the master code without my patch. It also can't work for S3, which means S3 broken on latest master code. You can also double confirm the log that stop as below:

...
S3_BOOT_SCRIPT_LIB_TERMINATE_OPCODE
S3BootScriptDone - Success
Call AsmDisablePaging64() to return to S3 Resume in PEI Phase
Install PPI: 88C9D306-0900-4EB5-8260-3E2DBEDA1F89
Install PPI: 605EA650-C65C-42E1-BA80-91A52AB618C6
Notify: PPI Guid: 605EA650-C65C-42E1-BA80-91A52AB618C6, Peim notify entry point: 82B5B0
Signal EndOfS3Resume
Signal 96F5296D-05F7-4F3C-8467-E456890E0CB5 to SMM - Enter
Locate Smm Communicate Ppi failed (Not Found)!
Transfer to 16bit OS waking vector - 991F0 ----> hang here!!!

Thanks,
Jiaxin 

> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Tuesday, April 23, 2024 9:20 PM
> To: Gerd Hoffmann <kraxel@redhat.com>
> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create
> gEfiSmmSmramMemoryGuid
> 
> More info:
> I quick dump the SMRAM info with original SmmAccess implementation, it's
> same as I produced in the gEfiSmmSmramMemoryGuid HOB.
> 
> SmmAccess:
> SmmAccessPeiEntryPoint: SMRAM map follows, 2 entries
> SmmAccessPeiEntryPoint:             7F000000                 1000             7F000000
> 1A    ---> for the S3 Resume in gEfiAcpiVariableGuid
> SmmAccessPeiEntryPoint:             7F001000               FFF000             7F001000
> A
> 
> Smram map in the gEfiSmmSmramMemoryGuid:
> PlatformQemuInitializeRam:             7F000000                 1000             7F000000
> 1A    --> ---> for the S3 Resume in gEfiAcpiVariableGuid
> PlatformQemuInitializeRam:             7F001000               FFF000             7F001000
> A
> 
> 
> Thanks,
> Jiaxin
> 
> > -----Original Message-----
> > From: Wu, Jiaxin
> > Sent: Tuesday, April 23, 2024 8:19 PM
> > To: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Yao,
> > Jiewen <jiewen.yao@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: RE: [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create
> > gEfiSmmSmramMemoryGuid
> >
> > >
> > > > +    SmramHobDescriptorBlock                             =
> > > (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)(Hob.Raw);
> > >
> > > > +    SmramHobDescriptorBlock->Descriptor[0].PhysicalStart =
> > > PlatformInfoHob->LowMemory - TsegSize;
> > > > +    SmramHobDescriptorBlock->Descriptor[0].CpuStart      =
> > > PlatformInfoHob->LowMemory - TsegSize;
> > > > +    SmramHobDescriptorBlock->Descriptor[0].PhysicalSize  =
> > EFI_PAGE_SIZE;
> > > > +    SmramHobDescriptorBlock->Descriptor[0].RegionState   =
> > > EFI_SMRAM_CLOSED | EFI_CACHEABLE | EFI_ALLOCATED;
> > >
> > > > +    SmramHobDescriptorBlock->Descriptor[1].PhysicalStart =
> > > SmramHobDescriptorBlock->Descriptor[0].PhysicalStart + EFI_PAGE_SIZE;
> > > > +    SmramHobDescriptorBlock->Descriptor[1].CpuStart      =
> > > SmramHobDescriptorBlock->Descriptor[0].CpuStart + EFI_PAGE_SIZE;
> > > > +    SmramHobDescriptorBlock->Descriptor[1].PhysicalSize  = TsegSize -
> > > EFI_PAGE_SIZE;
> > > > +    SmramHobDescriptorBlock->Descriptor[1].RegionState   =
> > > EFI_SMRAM_CLOSED | EFI_CACHEABLE;
> > >
> > > This is not going to fly.
> > >
> > > First, smram allocation doesn't work that way.  Have a look at
> > > OvmfPkg/SmmAccess.  I guess that easily explains why this series
> > > breaks S3 suspend.
> > >
> >
> > Oh? Could you explain a bit more for 1) how smram allocation works? 2)
> > what's the possible reason break the S3? I haven't check yet.
> >
> > > Second, storing these descriptors in a HOB (which is PEI memory)
> > > is questionable from a security point of view.
> > >
> >
> > HOB is only to expose the SMRAM address and size, not the contents in
> > smram, what's the security concern?
> >
> >
> > Thanks,
> > Jiaxin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118174): https://edk2.groups.io/g/devel/message/118174
Mute This Topic: https://groups.io/mt/105593577/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-