public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Wu, Jiaxin" <jiaxin.wu@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"kraxel@redhat.com" <kraxel@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>,
	"Yao, Jiewen" <jiewen.yao@intel.com>,
	"Ni, Ray" <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid
Date: Thu, 25 Apr 2024 01:54:37 +0000	[thread overview]
Message-ID: <MN0PR11MB6158903437051832748FC177FE172@MN0PR11MB6158.namprd11.prod.outlook.com> (raw)
In-Reply-To: <74uoxthjxoztfpmnt552eysn2u2blko6tkllnk3a76ax46yf5d@y34m4b4h6t57>

> 
> SmramInternal.c handles that.  It creates two regions, one is a page at
> the start of SMRAM where S3 state is stored (and marked as allocated),
> one is all the rest.
> 

Yes, the same logic is moved to the OvmfPkg/Library/PlatformInitLib/MemDetect.c:

    //
    // Create first SMRAM descriptor, which contains data structures used in S3 resume.
    // One page is enough for the data structure
    //
    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;

    //
    // Create second SMRAM descriptor, which is free and will be used by SMM foundation.
    //
    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;


> So, if you need some smram to initialize SMM in PEI I'd suggest to
> either add a third region, or make the first region larger.
> 
> It's not clear to me why you put the logic upside down and introduce
> that HOB in the first place.
> 

Let me explain more why need this change:

1. The EFI_SMM_SMRAM_MEMORY_GUID HOB, as defined in the PI specification, is used to describe the SMRAM memory regions supported by the platform. This HOB should be produced during the memory detection phase to align with the PI spec.

2. In addition to the memory reserved for ACPI S3 resume, an increasing number of features require reserving SMRAM for specific purposes, such as SmmRelocation. Other advanced features in Intel platforms also necessitate this. The implementation of these features varies and is entirely dependent on the platform. This is why an increasing number of platforms are adopting the EFI_SMM_SMRAM_MEMORY_GUID HOB for SMRAM description.

3. It is crucial that the SMRAM information remains consistent when retrieved from the platform, whether through the SMM ACCESS PPI/Protocol or the EFI_SMM_SMRAM_MEMORY_GUID HOB. Inconsistencies can lead to unexpected issues, most commonly memory region conflicts.

4. The SMM ACCESS PPI/Protocol can be naturally implemented for general use. The common approach is to utilize the EFI_SMM_SMRAM_MEMORY_GUID HOB. For reference, see the existing implementation in the EDK2 repository at edk2/UefiPayloadPkg/SmmAccessDxe/SmmAccessDxe.inf and edk2-platforms/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.inf. 

For the reasons mentioned, we are moving the SMRAM memory regions to HOBs and allowing SMM access to consume these HOBs.

I will add the above info into commit message.

> 
> Storing anything SMM related outside SMRAM makes me nervous.
> I'd strongly suggest to avoid that.
> 
> It might be that in this specific case it is not a problem.  But it
> needs very careful review of the implications (which I have not done)
> and you have to hope you don't miss a possible attack vector, such as
> someone modifying the HOB and the firmware then storing SMM data + code
> outside SMRAM.
> 

Understand, but here is the case we can record the info in non-smram since PI spec exposes that, there is no difference the info retrieved from PPI/ non-smm Protocol or the non-smram.

Thanks,
Jiaxin






-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118253): https://edk2.groups.io/g/devel/message/118253
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]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-04-25  1:54 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-18  6:55 [edk2-devel] [PATCH v3 00/13] Add SmmRelocationLib Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 01/13] UefiCpuPkg: Add SmmRelocationLib class Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 02/13] UefiCpuPkg/SmmRelocationLib: Add SmmRelocationLib library instance Wu, Jiaxin
2024-04-18  7:32   ` Ni, Ray
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 03/13] UefiCpuPkg/SmmRelocationLib: Rename global variables Wu, Jiaxin
2024-04-18  7:33   ` Ni, Ray
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 04/13] UefiCpuPkg/SmmRelocationLib: Avoid unnecessary memory allocation Wu, Jiaxin
2024-04-18  7:40   ` Ni, Ray
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 05/13] UefiCpuPkg/SmmRelocationLib: Remove unnecessary global variable Wu, Jiaxin
2024-04-18  7:48   ` Ni, Ray
2024-04-18  7:57     ` Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 06/13] UefiCpuPkg/SmmRelocationLib: Add library instance for AMD Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 07/13] OvmfPkg/SmmRelocationLib: Add library instance for OVMF Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid Wu, Jiaxin
2024-04-23  9:25   ` Gerd Hoffmann
2024-04-23 12:18     ` Wu, Jiaxin
2024-04-24 12:35       ` Gerd Hoffmann
2024-04-25  1:54         ` Wu, Jiaxin [this message]
2024-04-25  7:20           ` Gerd Hoffmann
2024-04-25  7:41             ` Wu, Jiaxin
2024-04-23 13:20     ` Wu, Jiaxin
2024-04-23 15:59     ` Wu, Jiaxin
2024-04-24 12:38       ` Gerd Hoffmann
2024-04-25  0:54         ` Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 09/13] OvmfPkg: Refine SmmAccess implementation Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 10/13] OvmfPkg/SmmCpuFeaturesLib: Check Smbase Relocation is done or not Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 11/13] OvmfPkg/PlatformPei: Relocate SmBases in PEI phase Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 12/13] UefiPayloadPkg/UefiPayloadPkg.dsc: Include SmmRelocationLib Wu, Jiaxin
2024-04-18  6:55 ` [edk2-devel] [PATCH v3 13/13] UefiCpuPkg/PiSmmCpuDxeSmm: Remove SmBases relocation logic Wu, Jiaxin
2024-04-18  8:15   ` Ni, Ray
2024-04-19  2:06     ` Wu, Jiaxin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MN0PR11MB6158903437051832748FC177FE172@MN0PR11MB6158.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox