Date   

Re: [PATCH 3/3] BaseTools: Drop check for distutils.utils

Yuwei Chen
 

This patch looks good to me.

Reviewed-by: Yuwei Chen<yuwei.chen@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Cole
Sent: Saturday, July 24, 2021 4:02 AM
To: devel@edk2.groups.io
Cc: Cole Robinson <crobinso@redhat.com>
Subject: [edk2-devel] [PATCH 3/3] BaseTools: Drop check for distutils.utils

distutils.utils is no longer used anywhere, so this check can be dropped.

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
BaseTools/Tests/RunTests.py | 7 -------
1 file changed, 7 deletions(-)

diff --git a/BaseTools/Tests/RunTests.py b/BaseTools/Tests/RunTests.py
index 81af736cd8..934683a446 100644
--- a/BaseTools/Tests/RunTests.py
+++ b/BaseTools/Tests/RunTests.py
@@ -13,13 +13,6 @@ import os
import sys
import unittest

-try:
- import distutils.util
-except ModuleNotFoundError:
- sys.exit('''
-Python reported: "No module named 'distutils.util"
-''')
-
import TestTools

def GetCTestSuite():
--
2.31.1





Re: [PATCH 0/3] BaseTools: fix some python DeprecationWarnings

Bob Feng
 

This patch set is good to me.

Reviewed-by: Bob Feng <bob.c.feng@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Cole
Sent: Saturday, July 24, 2021 4:02 AM
To: devel@edk2.groups.io
Cc: Cole Robinson <crobinso@redhat.com>
Subject: [edk2-devel] [PATCH 0/3] BaseTools: fix some python DeprecationWarnings

This addresses some python DeprecationWarnings that are popping up with python 3.10

Cole Robinson (3):
build: Fix python3.10 threading DeprecationWarnings
python: Replace distutils.utils.split_quotes with shlex.split
BaseTools: Drop check for distutils.utils

.../Source/Python/AutoGen/UniClassObject.py | 4 +-
.../Python/UPT/Library/UniClassObject.py | 4 +-
BaseTools/Source/Python/build/build.py | 48 +++++++++----------
BaseTools/Tests/RunTests.py | 7 ---
4 files changed, 28 insertions(+), 35 deletions(-)

--
2.31.1


Re: Adding HTTP boot IO timeout programmability from PcdHttpIoTimeout

Heng Luo
 

+ NetworkPkg Maitainer/Reviewer

Hi Zachary,

I notice you pushed the patch to https://github.com/tianocore/edk2/pull/1834

But according to “The developer process for the EDK II project" In  https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Normally we send the patch by mail, I recommend you send the patch by mail too(use git send-email *.patch):

The developer process for the EDK II project

  1. Setup the EDK II tree if you do not have one
  2. Create and checkout a topic branch for new feature or bug fix

$ git checkout -b <new-dev-branch> origin/master

  1. Make changes in the working tree
  2. Break up working tree changes into independent commits that do not break git bisect
    • Commit-Partitioning
    • To stage all modifications: $ git add -u
    • To add new files: $ git add <path-to-new-file>
    • To have git prompt you to selectively stage changes: $ git add -p
  3. Follow the commit message template given below when writing commit messages
    • Commit-Message-Format
    • To commit staged changes: $ git commit
      • Add the -s parameter to automatically append your Signed-off-by tag to the commit message.
  4. Use the ‘PatchCheck.py’ script under ‘edk2\BaseTools\Scripts’ directory to verify the commits are correctly formatted
    • To check the latest changes: $ python BaseTools/Scripts/PatchCheck.py -<N>
      • For example, 2 changes would be: $ python BaseTools/Scripts/PatchCheck.py -2
    • It is strongly recommended that you run PatchCheck.py after each commit. You can then easily amend the commit to correct any issues.
  5. Get the latest changes from origin

$ git fetch origin

Note: This updates origin/master, but not your local master branch. (origin/master may have newer commits than master.)

  1. Rebase the topic branch onto master branch

$ git rebase origin/master

  1. Create patch (serial) to the edk2-devel mailing list
    • Clean out any old patches: $ rm *.patch
    • Generate new patch files: $ git format-patch -M --thread origin/master
      • Add the --cover-letter parameter for long patch series. (Be sure to edit the cover-letter.)
      • Add the --subject-prefix="PATCH v2" if you are sending out a second version of the patch series.
    • $ git send-email *.patch
  2. Modify local commits based on the review feedbacks and repeat steps 3 to 9
    • For the latest commit, you can use $ git commit --amend
    • For multiple commits use $ git rebase -i origin/master
    • Consult your git gurus on edk2-devel or irc channel if you have questions.

 

Thanks,

Heng

 

From: Clark-williams, Zachary <zachary.clark-williams@...>
Sent: Friday, July 23, 2021 11:15 AM
To: devel@edk2.groups.io
Cc: Goetz, Philippe C <philippe.c.goetz@...>; Nagar, Rupa <rupa.nagar@...>; Luo, Heng <heng.luo@...>; Zhuang, Qihua <qihua.zhuang@...>; Lu, James <james.lu@...>
Subject: Adding HTTP boot IO timeout programmability from PcdHttpIoTimeout

 

Hello,

 

Please review the attached filed EDK2 tracker for feature enablement of programmable timeout of the HTTP boot IO timer.

NetworkPkg-HttpBoot: Making the HTTP IO timeout value programmable with PCD.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3507

HTTP boot has a default set forced timeout value of 5 seconds for getting the recovery image from a remote source. This change allows the HTTP boot flow to get the IO timeout value from the PcdHttpIoTimeout. PcdHttpIoTimeout value is set in the OneClickRecovery driver from the value provided by CSME.

PcdHttpIoTimeout minimum value 0.5 seconds
PcdHttpIoTimeout maximum value 120 seconds
PcdHttpIoTimeout default value 5 seconds

 

Thank you,

Zack


Re: [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

Yao, Jiewen
 

Hi James
"However, this ran into problems when it was decided AmdSev shouldn't have it's own Library."

I am not clear on the history. Would you please clarify why AmdSev should not have its own library?

It looks not reasonable to me. AmdSev is just a feature. A feature may have its own library. We have enough examples.

Also, the instance name "Grub" is very confusing. I compared PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a customized PlatformBootManagerLib.

For example, XEN feature removing and PIIX4 difference has nothing to do with Grub...
=================
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D
=================

It is a big misleading. Can we move the PlatformBootManagerLibGrub To AmdSev now?

-----Original Message-----
From: James Bottomley <jejb@linux.ibm.com>
Sent: Monday, July 26, 2021 5:10 AM
To: devel@edk2.groups.io; dovmurik@linux.ibm.com; Yao, Jiewen
<jiewen.yao@intel.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; Hubertus Franke
<frankeh@us.ibm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
Jordan L <jordan.l.justen@intel.com>; Ashish Kalra <ashish.kalra@amd.com>;
Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
<erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
Mujawar <sami.mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

On Sun, 2021-07-25 at 10:52 +0300, Dov Murik wrote:
And I do have one question:
May I know what is criteria to put a SEV module to OvmfPkg\AmdSev
or OvmfPkg directly?

My original understanding is:
If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf},
then it should be OvmfPkg.
If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf},
Then it should be in OvmfPkg\AmdSev.

Am I right?
I actually don't know the criteria. What you say sounds reasonable.
I'll also let James (who introduced the AmdSevX64 target) say what he
thinks.
The original reason for the AmdSev package was actually for
attestation: The only way to get attested boot using a standard VM
image for SEV and SEV-ES was to pull grub inside the measurement
envelope and have a stripped down hard failing boot path, so if the key
didn't decode the encrypted boot volume for some reason, the whole
thing would fail without revealing the injected secret. This stripped
down hard failing boot path is much easier to construct as a separate
target.

Essentially that means that lots of SEV exists outside the AmdSev
directory and things should only be in it if they're either modified to
support the encrypted volume boot path or are only required by it.
However, this ran into problems when it was decided AmdSev shouldn't
have it's own Library, so the modified boot path now lives in
OvmfPkg/Library/PlatformBootManagerLibGrub, so now it's unclear even to
me what the criteria are.

James


Re: [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

James Bottomley
 

On Sun, 2021-07-25 at 10:52 +0300, Dov Murik wrote:
And I do have one question:
May I know what is criteria to put a SEV module to OvmfPkg\AmdSev
or OvmfPkg directly?

My original understanding is:
If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf},
then it should be OvmfPkg.
If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf},
Then it should be in OvmfPkg\AmdSev.

Am I right?
I actually don't know the criteria. What you say sounds reasonable.
I'll also let James (who introduced the AmdSevX64 target) say what he
thinks.
The original reason for the AmdSev package was actually for
attestation: The only way to get attested boot using a standard VM
image for SEV and SEV-ES was to pull grub inside the measurement
envelope and have a stripped down hard failing boot path, so if the key
didn't decode the encrypted boot volume for some reason, the whole
thing would fail without revealing the injected secret. This stripped
down hard failing boot path is much easier to construct as a separate
target.

Essentially that means that lots of SEV exists outside the AmdSev
directory and things should only be in it if they're either modified to
support the encrypted volume boot path or are only required by it.
However, this ran into problems when it was decided AmdSev shouldn't
have it's own Library, so the modified boot path now lives in
OvmfPkg/Library/PlatformBootManagerLibGrub, so now it's unclear even to
me what the criteria are.

James


Re: [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

Dov Murik
 

Thanks for the explanations. Comments below:


On 25/07/2021 11:17, Yao, Jiewen wrote:
Thank you Dov.
Comment below:

-----Original Message-----
From: Dov Murik <dovmurik@linux.ibm.com>
Sent: Sunday, July 25, 2021 3:53 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; James Bottomley
<jejb@linux.ibm.com>; Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>;
Tom Lendacky <thomas.lendacky@amd.com>; Leif Lindholm
<leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>; Dov Murik
<dovmurik@linux.ibm.com>
Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

Hi Jiewen,

On 25/07/2021 5:43, Yao, Jiewen wrote:
Hi
I am reviewing this patch series. I am OK with most parts.
Thank you.


And I do have one question:
May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or
OvmfPkg directly?

My original understanding is:
If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should
be OvmfPkg.
If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it
should be in OvmfPkg\AmdSev.

Am I right?
I actually don't know the criteria. What you say sounds reasonable.
I'll also let James (who introduced the AmdSevX64 target) say what he
thinks.
[Jiewen] OK.




If that is indeed the case, then I need to:

1. Modify patch 4: put the code of the null implementation in
OvmfPkf/BlobVerifierLibNull/
[Jiewen] Since this is a library, I expect to be OvmfPkf/Library/BlobVerifierLibNull/
Yes, you're right, I missed that "/Library/" in my description.



2. Modify patches 5+6: use the new path of the BlobVerifierLibNull inf file

3. Modify patches 10+11: put the code of the SevHashes implementation in
OvmfPkg/AmdSev/BlobVerifierLibSevHashes/

Jiewen, is that what you had in mind?
[Jiewen] Let's comply with the exiting rule.
1) A library in a packet should be in XXXPkg/Library/AAALib in general. (For example, OvmfPkg/Library)
2) If a library in a packet belongs to a feature, then it should be XXXPkg/FeatureYYY/AAALib. (For example, OvmfPkg/Csm/CsmSupportLib)
OK. I'll send a new version with the paths corrected.






Two other things to consider:

A. The blob verification by hashes just uses initially-measured memory,
and no other features of SEV. It might be useful for other Confidential
Computing implementations. But maybe if that need arises then we'll
extract it from the AmdSev folder.
[Jiewen] I think so. Because it is generic, that is why I agree that the *library class* name does not include SEV keyword.
The *library instance* should add *Sev* keyword, because the implementation does include SEV specific, such as SEV_HASH_TABLE_GUID, SEV_KERNEL_HASH_GUID.

If you want to make it for generic confidential computing, then more work should be done. For example:
1) The instance name should be BlobVerificationLibTeeLinuxModuleHash. (TEE to replace SEV. We need Linux keyword, because I don’t think it is applicable for Windows)
2) We need consider crypto agile. The current instance only consider SHA256, which is not allowed in TDX.
3) The GUID definition should be in OvmfPkg.dec, as the interface.

Since we don’t have a TEE folder yet, I prefer we defer that work.
Later, when we finish all Sev/Tdx work, we can consider create a common Tee dir or event a package. But that may happen in next year.
OK I'll keep that in mind for later. Also note that these require
corresponding changes in QEMU.





B. There were talks about reducing the number of targets, and maybe
unifying AmdSevX64 into OvmfPkgX64. If this is indeed the direction
we're going to, then there's no need to separate the code.
[Jiewen] I think we are following what Laslo suggested.
1) The OvmfPkg includes the *basic* feature at first.
2) The advanced feature is checked into SEV folder or TDX folder, at first.
3) We can revisit those advanced feature once we think they are mature.

We agree that direction, and we should follow that.
Let's keep #1 and #2 at first to finish the feature at first (in this year). Then we can see how to enhance in #3 (maybe next year).
The more we know each other, the better we can create an architecture to support TEE.
Sounds good.

Thanks,
-Dov



Thank you
Yao Jiewen



Thanks,
-Dov


Thank you
Yao Jiewen


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov
Murik
Sent: Thursday, July 22, 2021 4:43 PM
To: devel@edk2.groups.io
Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh
<brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen
<jiewen.yao@intel.com>;
Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
Mujawar <sami.mujawar@arm.com>
Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

Booting with SEV prevented the loading of kernel, initrd, and kernel
command-line via QEMU fw_cfg interface because they arrive from the VMM
which is untrusted in SEV.

However, in some cases the kernel, initrd, and cmdline are not secret
but should not be modified by the host. In such a case, we want to
verify inside the trusted VM that the kernel, initrd, and cmdline are
indeed the ones expected by the Guest Owner, and only if that is the
case go on and boot them up (removing the need for grub inside OVMF in
that mode).

This patch series reserves an area in MEMFD (previously the last 1KB of
the launch secret page) which will contain the hashes of these three
blobs (kernel, initrd, cmdline), each under its own GUID entry. This
tables of hashes is populated by QEMU before launch, and encrypted as
part of the initial VM memory; this makes sure these hashes are part of
the SEV measurement (which has to be approved by the Guest Owner for
secret injection, for example). Note that populating the hashes table
requires QEMU support [1].

OVMF parses the table of hashes populated by QEMU (patch 10), and as it
reads the fw_cfg blobs from QEMU, it will verify each one against the
expected hash. This is all done inside the trusted VM context. If all
the hashes are correct, boot of the kernel is allowed to continue.

Any attempt by QEMU to modify the kernel, initrd, cmdline (including
dropping one of them), or to modify the OVMF code that verifies those
hashes, will cause the initial SEV measurement to change and therefore
will be detectable by the Guest Owner during launch before secret
injection.

Relevant part of OVMF serial log during boot with AmdSevX86 build and
QEMU with -kernel/-initrd/-append:

...
BlobVerifierLibSevHashesConstructor: Found injected hashes table in secure
location
Select Item: 0x17
Select Item: 0x8
FetchBlob: loading 7379328 bytes for "kernel"
Select Item: 0x18
Select Item: 0x11
VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in
table
VerifyBlob: Hash comparison succeeded for "kernel"
Select Item: 0xB
FetchBlob: loading 12483878 bytes for "initrd"
Select Item: 0x12
VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
VerifyBlob: Hash comparison succeeded for "initrd"
Select Item: 0x14
FetchBlob: loading 86 bytes for "cmdline"
Select Item: 0x15
VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in
table
VerifyBlob: Hash comparison succeeded for "cmdline"
...

The patch series is organized as follows:

1: Simple comment fix in adjacent area in the code.
2: Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
fetching.
3: Allow the (previously blocked) usage of -kernel in AmdSevX64.
4-7: Add BlobVerifierLib with null implementation and use it in the correct
location in QemuKernelLoaderFsDxe.
8-9: Reserve memory for hashes table, declare this area in the reset vector.
10-11: Add the secure implementation BlobVerifierLibSevHashes and use it in
AmdSevX64 builds.

[1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-
dovmurik@linux.ibm.com/

Code is at
https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v4

v4 changes:
- BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
when parsing the SEV hashes table structure

v3: https://edk2.groups.io/g/devel/message/77955
v3 changes:
- Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
DebugLib reference, fix doxygen comments, add missing IN attribute
- Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
doxygen comments, add missing IN attribute,
calculate buffer hash only when the guid is found in hashes table
- SecretPei: use ALIGN_VALUE to round the hob size
- Coding style fixes
- Add missing 'Ref:' in patch 1 commit message
- Fix phrasing and typos in commit messages
- Remove Cc: Laszlo from series

v2: https://edk2.groups.io/g/devel/message/77505
v2 changes:
- Use the last 1KB of the existing SEV launch secret page for hashes table
(instead of reserving a whole new MEMFD page).
- Build on top of commit cf203024745f
("OvmfPkg/GenericQemuLoadImageLib:
Read
cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location
in
which all of kernel/initrd/cmdline are fetched from QEMU.
- Use static linking of the two BlobVerifierLib implemenatations.
- Reorganize series.

v1: https://edk2.groups.io/g/devel/message/75567

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>

---

Ard: please review patch 6 (ArmVirtPkg). Thanks.

Tom, Brijesh: I'll also send the diff for patch 10. Thanks.

---

Dov Murik (8):
OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
OvmfPkg: add library class BlobVerifierLib with null implementation
OvmfPkg: add BlobVerifierLibNull to DSC
ArmVirtPkg: add BlobVerifierLibNull to DSC
OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
OvmfPkg/AmdSev/SecretPei: build hob for full page
OvmfPkg: add BlobVerifierLibSevHashes
OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

James Bottomley (3):
OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

OvmfPkg/OvmfPkg.dec | 9 +
ArmVirtPkg/ArmVirtQemu.dsc | 5 +-
ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.dsc | 9 +-
OvmfPkg/OvmfPkgIa32.dsc | 5 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 5 +-
OvmfPkg/OvmfPkgX64.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 5 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf | 24
+++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf |
37
++++

OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.
inf | 2 +
OvmfPkg/ResetVector/ResetVector.inf | 2 +
OvmfPkg/Include/Library/BlobVerifierLib.h | 38 ++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
|
11 ++
OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 2 +-
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 3 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33
++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c |
199
++++++++++++++++++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
|
5 +
OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c | 0
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
| 9 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20
++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 +
23 files changed, 425 insertions(+), 10 deletions(-)
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
create mode 100644
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
create mode 100644
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
copy OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c (100%)

--
2.25.1





Re: [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

Yao, Jiewen
 

Thank you Dov.
Comment below:

-----Original Message-----
From: Dov Murik <dovmurik@linux.ibm.com>
Sent: Sunday, July 25, 2021 3:53 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; James Bottomley
<jejb@linux.ibm.com>; Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>;
Tom Lendacky <thomas.lendacky@amd.com>; Leif Lindholm
<leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>; Dov Murik
<dovmurik@linux.ibm.com>
Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

Hi Jiewen,

On 25/07/2021 5:43, Yao, Jiewen wrote:
Hi
I am reviewing this patch series. I am OK with most parts.
Thank you.


And I do have one question:
May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or
OvmfPkg directly?

My original understanding is:
If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should
be OvmfPkg.
If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it
should be in OvmfPkg\AmdSev.

Am I right?
I actually don't know the criteria. What you say sounds reasonable.
I'll also let James (who introduced the AmdSevX64 target) say what he
thinks.
[Jiewen] OK.




If that is indeed the case, then I need to:

1. Modify patch 4: put the code of the null implementation in
OvmfPkf/BlobVerifierLibNull/
[Jiewen] Since this is a library, I expect to be OvmfPkf/Library/BlobVerifierLibNull/


2. Modify patches 5+6: use the new path of the BlobVerifierLibNull inf file

3. Modify patches 10+11: put the code of the SevHashes implementation in
OvmfPkg/AmdSev/BlobVerifierLibSevHashes/

Jiewen, is that what you had in mind?
[Jiewen] Let's comply with the exiting rule.
1) A library in a packet should be in XXXPkg/Library/AAALib in general. (For example, OvmfPkg/Library)
2) If a library in a packet belongs to a feature, then it should be XXXPkg/FeatureYYY/AAALib. (For example, OvmfPkg/Csm/CsmSupportLib)





Two other things to consider:

A. The blob verification by hashes just uses initially-measured memory,
and no other features of SEV. It might be useful for other Confidential
Computing implementations. But maybe if that need arises then we'll
extract it from the AmdSev folder.
[Jiewen] I think so. Because it is generic, that is why I agree that the *library class* name does not include SEV keyword.
The *library instance* should add *Sev* keyword, because the implementation does include SEV specific, such as SEV_HASH_TABLE_GUID, SEV_KERNEL_HASH_GUID.

If you want to make it for generic confidential computing, then more work should be done. For example:
1) The instance name should be BlobVerificationLibTeeLinuxModuleHash. (TEE to replace SEV. We need Linux keyword, because I don’t think it is applicable for Windows)
2) We need consider crypto agile. The current instance only consider SHA256, which is not allowed in TDX.
3) The GUID definition should be in OvmfPkg.dec, as the interface.

Since we don’t have a TEE folder yet, I prefer we defer that work.
Later, when we finish all Sev/Tdx work, we can consider create a common Tee dir or event a package. But that may happen in next year.




B. There were talks about reducing the number of targets, and maybe
unifying AmdSevX64 into OvmfPkgX64. If this is indeed the direction
we're going to, then there's no need to separate the code.
[Jiewen] I think we are following what Laslo suggested.
1) The OvmfPkg includes the *basic* feature at first.
2) The advanced feature is checked into SEV folder or TDX folder, at first.
3) We can revisit those advanced feature once we think they are mature.

We agree that direction, and we should follow that.
Let's keep #1 and #2 at first to finish the feature at first (in this year). Then we can see how to enhance in #3 (maybe next year).
The more we know each other, the better we can create an architecture to support TEE.

Thank you
Yao Jiewen



Thanks,
-Dov


Thank you
Yao Jiewen


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov
Murik
Sent: Thursday, July 22, 2021 4:43 PM
To: devel@edk2.groups.io
Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh
<brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen
<jiewen.yao@intel.com>;
Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
Mujawar <sami.mujawar@arm.com>
Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

Booting with SEV prevented the loading of kernel, initrd, and kernel
command-line via QEMU fw_cfg interface because they arrive from the VMM
which is untrusted in SEV.

However, in some cases the kernel, initrd, and cmdline are not secret
but should not be modified by the host. In such a case, we want to
verify inside the trusted VM that the kernel, initrd, and cmdline are
indeed the ones expected by the Guest Owner, and only if that is the
case go on and boot them up (removing the need for grub inside OVMF in
that mode).

This patch series reserves an area in MEMFD (previously the last 1KB of
the launch secret page) which will contain the hashes of these three
blobs (kernel, initrd, cmdline), each under its own GUID entry. This
tables of hashes is populated by QEMU before launch, and encrypted as
part of the initial VM memory; this makes sure these hashes are part of
the SEV measurement (which has to be approved by the Guest Owner for
secret injection, for example). Note that populating the hashes table
requires QEMU support [1].

OVMF parses the table of hashes populated by QEMU (patch 10), and as it
reads the fw_cfg blobs from QEMU, it will verify each one against the
expected hash. This is all done inside the trusted VM context. If all
the hashes are correct, boot of the kernel is allowed to continue.

Any attempt by QEMU to modify the kernel, initrd, cmdline (including
dropping one of them), or to modify the OVMF code that verifies those
hashes, will cause the initial SEV measurement to change and therefore
will be detectable by the Guest Owner during launch before secret
injection.

Relevant part of OVMF serial log during boot with AmdSevX86 build and
QEMU with -kernel/-initrd/-append:

...
BlobVerifierLibSevHashesConstructor: Found injected hashes table in secure
location
Select Item: 0x17
Select Item: 0x8
FetchBlob: loading 7379328 bytes for "kernel"
Select Item: 0x18
Select Item: 0x11
VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in
table
VerifyBlob: Hash comparison succeeded for "kernel"
Select Item: 0xB
FetchBlob: loading 12483878 bytes for "initrd"
Select Item: 0x12
VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
VerifyBlob: Hash comparison succeeded for "initrd"
Select Item: 0x14
FetchBlob: loading 86 bytes for "cmdline"
Select Item: 0x15
VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in
table
VerifyBlob: Hash comparison succeeded for "cmdline"
...

The patch series is organized as follows:

1: Simple comment fix in adjacent area in the code.
2: Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
fetching.
3: Allow the (previously blocked) usage of -kernel in AmdSevX64.
4-7: Add BlobVerifierLib with null implementation and use it in the correct
location in QemuKernelLoaderFsDxe.
8-9: Reserve memory for hashes table, declare this area in the reset vector.
10-11: Add the secure implementation BlobVerifierLibSevHashes and use it in
AmdSevX64 builds.

[1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-
dovmurik@linux.ibm.com/

Code is at
https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v4

v4 changes:
- BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
when parsing the SEV hashes table structure

v3: https://edk2.groups.io/g/devel/message/77955
v3 changes:
- Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
DebugLib reference, fix doxygen comments, add missing IN attribute
- Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
doxygen comments, add missing IN attribute,
calculate buffer hash only when the guid is found in hashes table
- SecretPei: use ALIGN_VALUE to round the hob size
- Coding style fixes
- Add missing 'Ref:' in patch 1 commit message
- Fix phrasing and typos in commit messages
- Remove Cc: Laszlo from series

v2: https://edk2.groups.io/g/devel/message/77505
v2 changes:
- Use the last 1KB of the existing SEV launch secret page for hashes table
(instead of reserving a whole new MEMFD page).
- Build on top of commit cf203024745f
("OvmfPkg/GenericQemuLoadImageLib:
Read
cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location
in
which all of kernel/initrd/cmdline are fetched from QEMU.
- Use static linking of the two BlobVerifierLib implemenatations.
- Reorganize series.

v1: https://edk2.groups.io/g/devel/message/75567

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>

---

Ard: please review patch 6 (ArmVirtPkg). Thanks.

Tom, Brijesh: I'll also send the diff for patch 10. Thanks.

---

Dov Murik (8):
OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
OvmfPkg: add library class BlobVerifierLib with null implementation
OvmfPkg: add BlobVerifierLibNull to DSC
ArmVirtPkg: add BlobVerifierLibNull to DSC
OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
OvmfPkg/AmdSev/SecretPei: build hob for full page
OvmfPkg: add BlobVerifierLibSevHashes
OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

James Bottomley (3):
OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

OvmfPkg/OvmfPkg.dec | 9 +
ArmVirtPkg/ArmVirtQemu.dsc | 5 +-
ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.dsc | 9 +-
OvmfPkg/OvmfPkgIa32.dsc | 5 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 5 +-
OvmfPkg/OvmfPkgX64.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 5 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf | 24
+++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf |
37
++++

OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.
inf | 2 +
OvmfPkg/ResetVector/ResetVector.inf | 2 +
OvmfPkg/Include/Library/BlobVerifierLib.h | 38 ++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
|
11 ++
OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 2 +-
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 3 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33
++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c |
199
++++++++++++++++++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
|
5 +
OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c | 0
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
| 9 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20
++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 +
23 files changed, 425 insertions(+), 10 deletions(-)
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
create mode 100644
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
create mode 100644
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
copy OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c (100%)

--
2.25.1





Re: [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

Dov Murik
 

Hi Jiewen,

On 25/07/2021 5:43, Yao, Jiewen wrote:
Hi
I am reviewing this patch series. I am OK with most parts.
Thank you.


And I do have one question:
May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or OvmfPkg directly?

My original understanding is:
If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should be OvmfPkg.
If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it should be in OvmfPkg\AmdSev.

Am I right?
I actually don't know the criteria. What you say sounds reasonable.
I'll also let James (who introduced the AmdSevX64 target) say what he
thinks.


If that is indeed the case, then I need to:

1. Modify patch 4: put the code of the null implementation in
OvmfPkf/BlobVerifierLibNull/

2. Modify patches 5+6: use the new path of the BlobVerifierLibNull inf file

3. Modify patches 10+11: put the code of the SevHashes implementation in
OvmfPkg/AmdSev/BlobVerifierLibSevHashes/

Jiewen, is that what you had in mind?



Two other things to consider:

A. The blob verification by hashes just uses initially-measured memory,
and no other features of SEV. It might be useful for other Confidential
Computing implementations. But maybe if that need arises then we'll
extract it from the AmdSev folder.

B. There were talks about reducing the number of targets, and maybe
unifying AmdSevX64 into OvmfPkgX64. If this is indeed the direction
we're going to, then there's no need to separate the code.


Thanks,
-Dov


Thank you
Yao Jiewen


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov Murik
Sent: Thursday, July 22, 2021 4:43 PM
To: devel@edk2.groups.io
Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>;
Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
Mujawar <sami.mujawar@arm.com>
Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

Booting with SEV prevented the loading of kernel, initrd, and kernel
command-line via QEMU fw_cfg interface because they arrive from the VMM
which is untrusted in SEV.

However, in some cases the kernel, initrd, and cmdline are not secret
but should not be modified by the host. In such a case, we want to
verify inside the trusted VM that the kernel, initrd, and cmdline are
indeed the ones expected by the Guest Owner, and only if that is the
case go on and boot them up (removing the need for grub inside OVMF in
that mode).

This patch series reserves an area in MEMFD (previously the last 1KB of
the launch secret page) which will contain the hashes of these three
blobs (kernel, initrd, cmdline), each under its own GUID entry. This
tables of hashes is populated by QEMU before launch, and encrypted as
part of the initial VM memory; this makes sure these hashes are part of
the SEV measurement (which has to be approved by the Guest Owner for
secret injection, for example). Note that populating the hashes table
requires QEMU support [1].

OVMF parses the table of hashes populated by QEMU (patch 10), and as it
reads the fw_cfg blobs from QEMU, it will verify each one against the
expected hash. This is all done inside the trusted VM context. If all
the hashes are correct, boot of the kernel is allowed to continue.

Any attempt by QEMU to modify the kernel, initrd, cmdline (including
dropping one of them), or to modify the OVMF code that verifies those
hashes, will cause the initial SEV measurement to change and therefore
will be detectable by the Guest Owner during launch before secret
injection.

Relevant part of OVMF serial log during boot with AmdSevX86 build and
QEMU with -kernel/-initrd/-append:

...
BlobVerifierLibSevHashesConstructor: Found injected hashes table in secure
location
Select Item: 0x17
Select Item: 0x8
FetchBlob: loading 7379328 bytes for "kernel"
Select Item: 0x18
Select Item: 0x11
VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
VerifyBlob: Hash comparison succeeded for "kernel"
Select Item: 0xB
FetchBlob: loading 12483878 bytes for "initrd"
Select Item: 0x12
VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
VerifyBlob: Hash comparison succeeded for "initrd"
Select Item: 0x14
FetchBlob: loading 86 bytes for "cmdline"
Select Item: 0x15
VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
VerifyBlob: Hash comparison succeeded for "cmdline"
...

The patch series is organized as follows:

1: Simple comment fix in adjacent area in the code.
2: Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
fetching.
3: Allow the (previously blocked) usage of -kernel in AmdSevX64.
4-7: Add BlobVerifierLib with null implementation and use it in the correct
location in QemuKernelLoaderFsDxe.
8-9: Reserve memory for hashes table, declare this area in the reset vector.
10-11: Add the secure implementation BlobVerifierLibSevHashes and use it in
AmdSevX64 builds.

[1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-
dovmurik@linux.ibm.com/

Code is at
https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v4

v4 changes:
- BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
when parsing the SEV hashes table structure

v3: https://edk2.groups.io/g/devel/message/77955
v3 changes:
- Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
DebugLib reference, fix doxygen comments, add missing IN attribute
- Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
doxygen comments, add missing IN attribute,
calculate buffer hash only when the guid is found in hashes table
- SecretPei: use ALIGN_VALUE to round the hob size
- Coding style fixes
- Add missing 'Ref:' in patch 1 commit message
- Fix phrasing and typos in commit messages
- Remove Cc: Laszlo from series

v2: https://edk2.groups.io/g/devel/message/77505
v2 changes:
- Use the last 1KB of the existing SEV launch secret page for hashes table
(instead of reserving a whole new MEMFD page).
- Build on top of commit cf203024745f ("OvmfPkg/GenericQemuLoadImageLib:
Read
cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location in
which all of kernel/initrd/cmdline are fetched from QEMU.
- Use static linking of the two BlobVerifierLib implemenatations.
- Reorganize series.

v1: https://edk2.groups.io/g/devel/message/75567

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>

---

Ard: please review patch 6 (ArmVirtPkg). Thanks.

Tom, Brijesh: I'll also send the diff for patch 10. Thanks.

---

Dov Murik (8):
OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
OvmfPkg: add library class BlobVerifierLib with null implementation
OvmfPkg: add BlobVerifierLibNull to DSC
ArmVirtPkg: add BlobVerifierLibNull to DSC
OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
OvmfPkg/AmdSev/SecretPei: build hob for full page
OvmfPkg: add BlobVerifierLibSevHashes
OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

James Bottomley (3):
OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

OvmfPkg/OvmfPkg.dec | 9 +
ArmVirtPkg/ArmVirtQemu.dsc | 5 +-
ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.dsc | 9 +-
OvmfPkg/OvmfPkgIa32.dsc | 5 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 5 +-
OvmfPkg/OvmfPkgX64.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 5 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf | 24
+++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37
++++

OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.
inf | 2 +
OvmfPkg/ResetVector/ResetVector.inf | 2 +
OvmfPkg/Include/Library/BlobVerifierLib.h | 38 ++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h |
11 ++
OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 2 +-
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 3 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33 ++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 199
++++++++++++++++++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c |
5 +
OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c | 0
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
| 9 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 +
23 files changed, 425 insertions(+), 10 deletions(-)
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
create mode 100644
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
copy OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c (100%)

--
2.25.1





Re: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to support Tdx

Min Xu
 

Agree. I will update the patch set based upon your suggestions.

Thanks!
Xu, Min

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Sunday, July 25, 2021 2:01 PM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Brijesh Singh
<brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Tom Lendacky
<thomas.lendacky@amd.com>
Subject: RE: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to
support Tdx

Hi Min, Brijesh, James
I feel very frustrated when I review the existing OVMF reset vector.

A big problem is that this code mixed too many SEV stuff, and we are trying
to add more TDX stuff in *one* file, without clear isolation.

Take PageTables64.asm as example, here are the symbols. (* means it is
newly added.) =================
CheckSevFeatures:
GetSevEncBit:
SevEncBitLowHlt:
SevSaveMask:
NoSev:
NoSevEsVcHlt:
NoSevPass:
SevExit:
IsSevEsEnabled:
SevEsDisabled:
SetCr3ForPageTables64:
CheckSev: (*)
SevNotActive:
clearPageTablesMemoryLoop:
pageTableEntriesLoop:
tdClearTdxPageTablesMemoryLoop: (*)
IsSevEs: (*)
pageTableEntries4kLoop:
clearGhcbMemoryLoop:
SetCr3:
SevEsIdtNotCpuid:
SevEsIdtNoCpuidResponse:
SevEsIdtTerminate:
SevEsIdtHlt:
SevEsIdtVmmComm:
NextReg:
VmmDone:
=================

In order to better maintain the ResetVector, may I propose some refinement:
1) The main function only contains the non-TEE function, where TEE == SEV +
TDX.
2) The TEE related code is moved to TEE specific standalone file, such
*Sev.asm and *Tdx.Asm.

3) We need handle different cases with different pattern.
I hope the patter can be used consistently. As such, the reviewer can easily
understand what it is for.

3.1) If TEE function is a hook, then the main function calls TEE function
directly. The TEE function need implement a TEE check function (such as
IsSev, or IsTdx). For example:
====================
OneTimeCall PreMainFunctionHookSev
OneTimeCall PreMainFunctionHookTdx
MainFunction:
XXXXXX
OneTimeCall PostMainFunctionHookSev
OneTimeCall PostMainFunctionHookTdx
====================
3.2) If TEE function is a replacement for non-TEE function. The main function
can call TEE replacement function, then check the return status to decide
next step. For example:
====================
OneTimeCall MainFunctionSev
Jz MainFunctionEnd
OneTimeCall MainFunctionTdx
Jz MainFunctionEnd
MainFunction:
XXXXXX
MainFunctionEnd:
====================

4) If we found it is too hard to write code in above patter, we can discuss
case by case.




-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, July 22, 2021 1:52 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Brijesh Singh <brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; James Bottomley
<jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom
Lendacky
<thomas.lendacky@amd.com>
Subject: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to
support Tdx

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). But in Non-Td guest the initial state of
CPUs is 16-bit real mode. To resolve this conflict, BITS 16/32 is used
in the very beginning of ResetVector. It will check the 32-bit
protected mode or 16-bit real mode, then jump to the corresponding entry
point.
This is done in OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm.

ReloadFlat32.asm load the GDT and set the CR0, then jump to Flat-32
mode.

InitTdx.asm is called to record the Tdx signature ('TDXG') and other
tdx information in a TDX_WORK_AREA which can be used by the other
routines in ResetVector.

Init32.asm is 32-bit initialization code in OvmfPkg. It puts above
ReloadFlat32 and InitTdx together to do the initializaiton for Tdx.

After that Tdx jumps to 64-bit long mode by doing following tasks:
1. SetCr3ForPageTables64
For OVMF, some initial page tables is built at:
PcdOvmfSecPageTablesBase - (PcdOvmfSecPageTablesBase + 0x6000)
This page table supports the 4-level page table.
But Tdx support 4-level and 5-level page table based on the CPU GPA
width.
48bit is 4-level paging, 52-bit is 5-level paging.
If 5-level page table is supported (GPAW is 52), then a top level
page directory pointers (1 * 256TB entry) is generated in the
TdxPageTable.
2. Set Cr4
Enable PAE.
3. Adjust Cr3
If GPAW is 48, then Cr3 is PT_ADDR (0). If GPAW is 52, then Cr3 is
TDX_PT_ADDR (0).

Tdx MailBox [0x10, 0x800] is reserved for OS. So we initialize piece
of this area ([0x10, 0x20]) to record the Tdx flag ('TDXG') and other
Tdx info so that they can be used in the following flow.

After all above is successfully done, Tdx jump to SecEntry.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 21 ++++++++
OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm | 47 ++++++++++++++++
OvmfPkg/ResetVector/Ia32/Init32.asm | 34 ++++++++++++
OvmfPkg/ResetVector/Ia32/InitTdx.asm | 57 ++++++++++++++++++++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 41 ++++++++++++++
OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm | 44 +++++++++++++++
OvmfPkg/ResetVector/ResetVector.nasmb | 18 +++++++
7 files changed, 262 insertions(+)
create mode 100644 OvmfPkg/ResetVector/Ia32/Init32.asm
create mode 100644 OvmfPkg/ResetVector/Ia32/InitTdx.asm
create mode 100644 OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index ac86ce69ebe8..a390ed81d021 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -155,10 +155,31 @@ resetVector:
;
; This is where the processor will begin execution ;
+; In IA32 we follow the standard reset vector flow. While in X64, Td
+guest ; may be supported. Td guest requires the startup mode to be
+32-bit ; protected mode but the legacy VM startup mode is 16-bit real
mode.
+; To make NASM generate such shared entry code that behaves correctly
+in ; both 16-bit and 32-bit mode, more BITS directives are added.
+;
+%ifdef ARCH_IA32
+
nop
nop
jmp EarlyBspInitReal16

+%else
+
+ smsw ax
+ test al, 1
+ jz .Real
+BITS 32
+ jmp Main32
+BITS 16
+.Real:
+ jmp EarlyBspInitReal16
+
+%endif
+
ALIGN 16

fourGigabytes:
diff --git a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
index c6d0d898bcd1..2206ca719593 100644
--- a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
+++ b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
@@ -17,6 +17,9 @@ Transition32FlatTo64Flat:

OneTimeCall SetCr3ForPageTables64

+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz TdxTransition32FlatTo64Flat
+
mov eax, cr4
bts eax, 5 ; enable PAE
mov cr4, eax
@@ -65,10 +68,54 @@ EnablePaging:
bts eax, 31 ; set PG
mov cr0, eax ; enable paging

+ jmp _jumpTo64Bit
+
+;
+; Tdx Transition from 32Flat to 64Flat ;
+TdxTransition32FlatTo64Flat:
+
+ mov eax, cr4
+ bts eax, 5 ; enable PAE
+
+ ;
+ ; byte[TDX_WORK_AREA_PAGELEVEL5] holds the indicator whether
+ 52bit is
supported.
+ ; if it is the case, need to set LA57 and use 5-level paging
+ ;
+ cmp byte[TDX_WORK_AREA_PAGELEVEL5], 0
+ jz .set_cr4
+ bts eax, 12
+.set_cr4:
+ mov cr4, eax
+ mov ebx, cr3
+
+ ;
+ ; if la57 is not set, we are ok
+ ; if using 5-level paging, adjust top-level page directory
+ ;
+ bt eax, 12
+ jnc .set_cr3
+ mov ebx, TDX_PT_ADDR (0)
+.set_cr3:
+ mov cr3, ebx
+
+ mov eax, cr0
+ bts eax, 31 ; set PG
+ mov cr0, eax ; enable paging
+
+_jumpTo64Bit:
jmp LINEAR_CODE64_SEL:ADDR_OF(jumpTo64BitAndLandHere)
+
BITS 64
jumpTo64BitAndLandHere:

+ ;
+ ; For Td guest we are done and jump to the end
+ ;
+ mov eax, TDX_WORK_AREA
+ cmp dword [eax], 0x47584454 ; 'TDXG'
+ jz GoodCompare
+
;
; Check if the second step of the SEV-ES mitigation is to be performed.
;
diff --git a/OvmfPkg/ResetVector/Ia32/Init32.asm
b/OvmfPkg/ResetVector/Ia32/Init32.asm
new file mode 100644
index 000000000000..772adc51e531
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/Init32.asm
@@ -0,0 +1,34 @@
+;--------------------------------------------------------------------
+----------
+; @file
+; 32-bit initialization for Tdx
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> ;
+SPDX-License-Identifier: BSD-2-Clause-Patent ;
+;--------------------------------------------------------------------
+----------
+
+BITS 32
+
+;
+; Modified: EBP
+;
+; @param[in] EBX [6:0] CPU supported GPA width
+; [7:7] 5 level page table support
+; @param[in] ECX [31:0] TDINITVP - Untrusted Configuration
+; @param[in] EDX [31:0] VCPUID
+; @param[in] ESI [31:0] VCPU_Index
+;
+Init32:
+ ;
+ ; Save EBX in EBP because EBX will be changed in ReloadFlat32
+ ;
+ mov ebp, ebx
+
+ OneTimeCall ReloadFlat32
+
+ ;
+ ; Init Tdx
+ ;
+ OneTimeCall InitTdx
+
+ OneTimeCallRet Init32
diff --git a/OvmfPkg/ResetVector/Ia32/InitTdx.asm
b/OvmfPkg/ResetVector/Ia32/InitTdx.asm
new file mode 100644
index 000000000000..de8273da6a0c
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/InitTdx.asm
@@ -0,0 +1,57 @@
+;--------------------------------------------------------------------
+----------
+; @file
+; Initialize TDX_WORK_AREA to record the Tdx flag ('TDXG') and other
Tdx info
+; so that the following codes can use these information.
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> ;
+SPDX-License-Identifier: BSD-2-Clause-Patent ;
+;--------------------------------------------------------------------
+----------
+
+BITS 32
+
+;
+; Modified: EBP
+;
+InitTdx:
+ ;
+ ; In Td guest, BSP/AP shares the same entry point
+ ; BSP builds up the page table, while APs shouldn't do the same task.
+ ; Instead, APs just leverage the page table which is built by BSP.
+ ; APs will wait until the page table is ready.
+ ; In Td guest, vCPU 0 is treated as the BSP, the others are APs.
+ ; ESI indicates the vCPU ID.
+ ;
+ cmp esi, 0
+ je tdBspEntry
+
+apWait:
+ cmp byte[TDX_WORK_AREA_PGTBL_READY], 0
+ je apWait
+ jmp doneTdxInit
+
+tdBspEntry:
+ ;
+ ; It is of Tdx Guest
+ ; Save the Tdx info in TDX_WORK_AREA so that the following code can
use
+ ; these information.
+ ;
+ mov dword [TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+
+ ;
+ ; EBP[6:0] CPU supported GPA width
+ ;
+ and ebp, 0x3f
+ cmp ebp, 52
+ jl NotPageLevel5
+ mov byte[TDX_WORK_AREA_PAGELEVEL5], 1
+
+NotPageLevel5:
+ ;
+ ; ECX[31:0] TDINITVP - Untrusted Configuration
+ ;
+ mov DWORD[TDX_WORK_AREA_INITVP], ecx
+ mov DWORD[TDX_WORK_AREA_INFO], ebp
+
+doneTdxInit:
+ OneTimeCallRet InitTdx
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 5fae8986d9da..508df6cf5967 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -218,6 +218,24 @@ SevEsDisabled:
;
SetCr3ForPageTables64:

+ ;
+ ; Check Td guest
+ ;
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jnz CheckSev
+
+ xor edx, edx
+
+ ;
+ ; In Td guest, BSP builds the page table and set the flag of
+ ; TDX_WORK_AREA_PGTBL_READY. APs check this flag and then set
+ ; cr3 directly.
+ ;
+ cmp byte[TDX_WORK_AREA_PGTBL_READY], 1
+ jz SetCr3
+ jmp SevNotActive
+
+CheckSev:
OneTimeCall CheckSevFeatures
xor edx, edx
test eax, eax
@@ -277,6 +295,29 @@ pageTableEntriesLoop:
mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
loop pageTableEntriesLoop

+ ;
+ ; If it is Td guest, TdxExtraPageTable should be initialized as well
+ ;
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jnz IsSevEs
+
+ xor eax, eax
+ mov ecx, 0x400
+tdClearTdxPageTablesMemoryLoop:
+ mov dword [ecx * 4 + TDX_PT_ADDR (0) - 4], eax
+ loop tdClearTdxPageTablesMemoryLoop
+
+ xor edx, edx
+ ;
+ ; Top level Page Directory Pointers (1 * 256TB entry)
+ ;
+ mov dword[TDX_PT_ADDR (0)], PT_ADDR (0) + PAGE_PDP_ATTR
+ mov dword[TDX_PT_ADDR (4)], edx
+
+ mov byte[TDX_WORK_AREA_PGTBL_READY], 1
+ jmp SetCr3
+
+IsSevEs:
OneTimeCall IsSevEsEnabled
test eax, eax
jz SetCr3
diff --git a/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
b/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
new file mode 100644
index 000000000000..06d44142625a
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
@@ -0,0 +1,44 @@
+;--------------------------------------------------------------------
+----------
+; @file
+; Load the GDT and set the CR0/CR4, then jump to Flat 32 protected
mode.
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> ;
+SPDX-License-Identifier: BSD-2-Clause-Patent ;
+;--------------------------------------------------------------------
+----------
+
+%define SEC_DEFAULT_CR0 0x00000023
+%define SEC_DEFAULT_CR4 0x640
+
+BITS 32
+
+;
+; Modified: EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS ;
+ReloadFlat32:
+
+ cli
+ mov ebx, ADDR_OF(gdtr)
+ lgdt [ebx]
+
+ mov eax, SEC_DEFAULT_CR0
+ mov cr0, eax
+
+ jmp LINEAR_CODE_SEL:dword
ADDR_OF(jumpToFlat32BitAndLandHere)
+BITS 32
+jumpToFlat32BitAndLandHere:
+
+ mov eax, SEC_DEFAULT_CR4
+ mov cr4, eax
+
+ debugShowPostCode POSTCODE_32BIT_MODE
+
+ mov ax, LINEAR_SEL
+ mov ds, ax
+ mov es, ax
+ mov fs, ax
+ mov gs, ax
+ mov ss, ax
+
+ OneTimeCallRet ReloadFlat32
+
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
b/OvmfPkg/ResetVector/ResetVector.nasmb
index b653fe87abd6..3ec163613477 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -106,6 +106,21 @@
%define TDX_EXTRA_PAGE_TABLE_BASE FixedPcdGet32
(PcdOvmfSecGhcbPageTableBase)
%define TDX_EXTRA_PAGE_TABLE_SIZE FixedPcdGet32
(PcdOvmfSecGhcbPageTableSize)

+ ;
+ ; TdMailboxBase [0x10, 0x800] is reserved for OS.
+ ; Td guest initialize piece of this area (TdMailboxBase
+ [0x10,0x20]) to ; record the Td guest info so that this information
+ can be used in the ; following ResetVector flow.
+ ;
+ %define TD_MAILBOX_WORKAREA_OFFSET 0x10
+ %define TDX_WORK_AREA (TDX_MAILBOX_MEMORY_BASE +
TD_MAILBOX_WORKAREA_OFFSET)
+ %define TDX_WORK_AREA_PAGELEVEL5 (TDX_WORK_AREA + 4)
+ %define TDX_WORK_AREA_PGTBL_READY (TDX_WORK_AREA + 5)
+ %define TDX_WORK_AREA_INITVP (TDX_WORK_AREA + 8)
+ %define TDX_WORK_AREA_INFO (TDX_WORK_AREA + 8 + 4)
+
+ %define TDX_PT_ADDR(Offset) (TDX_EXTRA_PAGE_TABLE_BASE +
(Offset))
+
%define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase)
+
(Offset))

%define GHCB_PT_ADDR (FixedPcdGet32
(PcdOvmfSecGhcbPageTableBase))
@@ -117,6 +132,9 @@
%define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32
(PcdOvmfSecPeiTempRamBase) + FixedPcdGet32
(PcdOvmfSecPeiTempRamSize))

%include "X64/TdxMetadata.asm"
+ %include "Ia32/Init32.asm"
+ %include "Ia32/InitTdx.asm"
+ %include "Ia32/ReloadFlat32.asm"

%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/PageTables64.asm"
--
2.29.2.windows.2


Re: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point in Main.asm

Min Xu
 

I see. I will update it in the next version.

Thanks!

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Sunday, July 25, 2021 3:44 PM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point
in Main.asm

Yes, if that can avoid the UefiCpuPkg change.

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Sunday, July 25, 2021 3:42 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry
point in Main.asm

On July 25, 2021 2:08 PM, Yao, Jiewen wrote:
Current OvmfPkg Reser vector is a mess (see my previous email).
I also compared the ResetVector code in UefiCpuPkg and OvmfPkg.
There are already duplication/override.

I suggest we just drop UefiCpuPkg change, and focus on improving OvmfPkg.
If we need add something in UefiCpuPkg, let's copy the file to
OvmfPkg and update there.
Do you mean we create the Main.asm in OvmfPkg/ResetVector/ and update
the changes in this Main.asm?

I really don't want to mess up UefiCpuPkg at this moment.
We can make a better architecture to share reset vector in virtual
BIOS and physical BIOS later. But at this moment, let's finish the
virtual BIOS architecture at first.

Thank you
Yao Jiewen

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, July 22, 2021 1:52 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Dong, Eric
<eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
<jiewen.yao@intel.com>
Subject: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry
point in Main.asm

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). Main32 entry point is added in
UefiCpuPkg/ResetVector/Vtf0/Main.asm so that Main.asm can support
the 32-bit protected mode.

Init32.asm is the 32-bit initialization code. It is a null stub in
UefiCpuPkg. The actual initialization can be implemented in the
platform (OvmfPkg/ResetVector/Ia32/Init32.asm is the example.)

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm | 13 +++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Main.asm | 14 ++++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb | 2 +-
3 files changed, 28 insertions(+), 1 deletion(-) create mode
100644 UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm

diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
new file mode 100644
index 000000000000..0cdae4a4a84a
--- /dev/null
+++ b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
@@ -0,0 +1,13 @@
+;----------------------------------------------------------------
+----
+----------
+; @file
+; 32-bit initialization code.
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+;
+SPDX-License-Identifier: BSD-2-Clause-Patent ;
+;----------------------------------------------------------------
+----
+----------
+
+BITS 32
+
+Init32:
+ nop
+ OneTimeCallRet Init32
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
index 19d08482f831..4920c6937e1b 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
@@ -36,6 +36,20 @@ Main16:

BITS 32

+%ifdef ARCH_X64
+
+ jmp SearchBfv
+
+;
+; Entry point of Main32
+;
+Main32:
+
+ OneTimeCall Init32
+
+%endif
+
+SearchBfv:
;
; Search for the Boot Firmware Volume (BFV)
;
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
index 493738c79c1c..6493b9863c48 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
+++ b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
@@ -51,7 +51,7 @@
%include "Ia32/SearchForSecEntry.asm"

%ifdef ARCH_X64
-%include "Ia32/Flat32ToFlat64.asm"
+%include "Ia32/Init32.asm"
%include "Ia32/PageTables64.asm"
%endif

--
2.29.2.windows.2


Re: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point in Main.asm

Yao, Jiewen
 

Yes, if that can avoid the UefiCpuPkg change.

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Sunday, July 25, 2021 3:42 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point in
Main.asm

On July 25, 2021 2:08 PM, Yao, Jiewen wrote:
Current OvmfPkg Reser vector is a mess (see my previous email).
I also compared the ResetVector code in UefiCpuPkg and OvmfPkg. There
are already duplication/override.

I suggest we just drop UefiCpuPkg change, and focus on improving OvmfPkg.
If we need add something in UefiCpuPkg, let's copy the file to OvmfPkg and
update there.
Do you mean we create the Main.asm in OvmfPkg/ResetVector/ and update
the changes in this Main.asm?

I really don't want to mess up UefiCpuPkg at this moment.
We can make a better architecture to share reset vector in virtual BIOS and
physical BIOS later. But at this moment, let's finish the virtual BIOS
architecture at first.

Thank you
Yao Jiewen

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, July 22, 2021 1:52 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Dong, Eric <eric.dong@intel.com>;
Ni, Ray <ray.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point
in Main.asm

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). Main32 entry point is added in
UefiCpuPkg/ResetVector/Vtf0/Main.asm so that Main.asm can support the
32-bit protected mode.

Init32.asm is the 32-bit initialization code. It is a null stub in
UefiCpuPkg. The actual initialization can be implemented in the
platform (OvmfPkg/ResetVector/Ia32/Init32.asm is the example.)

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm | 13 +++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Main.asm | 14 ++++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb | 2 +-
3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644
UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm

diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
new file mode 100644
index 000000000000..0cdae4a4a84a
--- /dev/null
+++ b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
@@ -0,0 +1,13 @@
+;--------------------------------------------------------------------
+----------
+; @file
+; 32-bit initialization code.
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> ;
+SPDX-License-Identifier: BSD-2-Clause-Patent ;
+;--------------------------------------------------------------------
+----------
+
+BITS 32
+
+Init32:
+ nop
+ OneTimeCallRet Init32
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
index 19d08482f831..4920c6937e1b 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
@@ -36,6 +36,20 @@ Main16:

BITS 32

+%ifdef ARCH_X64
+
+ jmp SearchBfv
+
+;
+; Entry point of Main32
+;
+Main32:
+
+ OneTimeCall Init32
+
+%endif
+
+SearchBfv:
;
; Search for the Boot Firmware Volume (BFV)
;
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
index 493738c79c1c..6493b9863c48 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
+++ b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
@@ -51,7 +51,7 @@
%include "Ia32/SearchForSecEntry.asm"

%ifdef ARCH_X64
-%include "Ia32/Flat32ToFlat64.asm"
+%include "Ia32/Init32.asm"
%include "Ia32/PageTables64.asm"
%endif

--
2.29.2.windows.2


Re: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point in Main.asm

Min Xu
 

On July 25, 2021 2:08 PM, Yao, Jiewen wrote:
Current OvmfPkg Reser vector is a mess (see my previous email).
I also compared the ResetVector code in UefiCpuPkg and OvmfPkg. There
are already duplication/override.

I suggest we just drop UefiCpuPkg change, and focus on improving OvmfPkg.
If we need add something in UefiCpuPkg, let's copy the file to OvmfPkg and
update there.
Do you mean we create the Main.asm in OvmfPkg/ResetVector/ and update
the changes in this Main.asm?

I really don't want to mess up UefiCpuPkg at this moment.
We can make a better architecture to share reset vector in virtual BIOS and
physical BIOS later. But at this moment, let's finish the virtual BIOS
architecture at first.

Thank you
Yao Jiewen

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, July 22, 2021 1:52 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Dong, Eric <eric.dong@intel.com>;
Ni, Ray <ray.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point
in Main.asm

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). Main32 entry point is added in
UefiCpuPkg/ResetVector/Vtf0/Main.asm so that Main.asm can support the
32-bit protected mode.

Init32.asm is the 32-bit initialization code. It is a null stub in
UefiCpuPkg. The actual initialization can be implemented in the
platform (OvmfPkg/ResetVector/Ia32/Init32.asm is the example.)

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm | 13 +++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Main.asm | 14 ++++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb | 2 +-
3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644
UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm

diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
new file mode 100644
index 000000000000..0cdae4a4a84a
--- /dev/null
+++ b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
@@ -0,0 +1,13 @@
+;--------------------------------------------------------------------
+----------
+; @file
+; 32-bit initialization code.
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> ;
+SPDX-License-Identifier: BSD-2-Clause-Patent ;
+;--------------------------------------------------------------------
+----------
+
+BITS 32
+
+Init32:
+ nop
+ OneTimeCallRet Init32
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
index 19d08482f831..4920c6937e1b 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
@@ -36,6 +36,20 @@ Main16:

BITS 32

+%ifdef ARCH_X64
+
+ jmp SearchBfv
+
+;
+; Entry point of Main32
+;
+Main32:
+
+ OneTimeCall Init32
+
+%endif
+
+SearchBfv:
;
; Search for the Boot Firmware Volume (BFV)
;
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
index 493738c79c1c..6493b9863c48 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
+++ b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
@@ -51,7 +51,7 @@
%include "Ia32/SearchForSecEntry.asm"

%ifdef ARCH_X64
-%include "Ia32/Flat32ToFlat64.asm"
+%include "Ia32/Init32.asm"
%include "Ia32/PageTables64.asm"
%endif

--
2.29.2.windows.2


Re: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point in Main.asm

Yao, Jiewen
 

Current OvmfPkg Reser vector is a mess (see my previous email).
I also compared the ResetVector code in UefiCpuPkg and OvmfPkg. There are already duplication/override.

I suggest we just drop UefiCpuPkg change, and focus on improving OvmfPkg.
If we need add something in UefiCpuPkg, let's copy the file to OvmfPkg and update there.

I really don't want to mess up UefiCpuPkg at this moment.
We can make a better architecture to share reset vector in virtual BIOS and physical BIOS later. But at this moment, let's finish the virtual BIOS architecture at first.

Thank you
Yao Jiewen

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, July 22, 2021 1:52 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
Ray <ray.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: [PATCH V2 3/4] UefiCpuPkg/ResetVector: Add Main32 entry point in
Main.asm

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). Main32 entry point is added in
UefiCpuPkg/ResetVector/Vtf0/Main.asm so that Main.asm can support
the 32-bit protected mode.

Init32.asm is the 32-bit initialization code. It is a null stub in
UefiCpuPkg. The actual initialization can be implemented in the platform
(OvmfPkg/ResetVector/Ia32/Init32.asm is the example.)

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm | 13 +++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Main.asm | 14 ++++++++++++++
UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb | 2 +-
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm

diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
new file mode 100644
index 000000000000..0cdae4a4a84a
--- /dev/null
+++ b/UefiCpuPkg/ResetVector/Vtf0/Ia32/Init32.asm
@@ -0,0 +1,13 @@
+;------------------------------------------------------------------------------
+; @file
+; 32-bit initialization code.
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS 32
+
+Init32:
+ nop
+ OneTimeCallRet Init32
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
index 19d08482f831..4920c6937e1b 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Main.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Main.asm
@@ -36,6 +36,20 @@ Main16:

BITS 32

+%ifdef ARCH_X64
+
+ jmp SearchBfv
+
+;
+; Entry point of Main32
+;
+Main32:
+
+ OneTimeCall Init32
+
+%endif
+
+SearchBfv:
;
; Search for the Boot Firmware Volume (BFV)
;
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
index 493738c79c1c..6493b9863c48 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
+++ b/UefiCpuPkg/ResetVector/Vtf0/Vtf0.nasmb
@@ -51,7 +51,7 @@
%include "Ia32/SearchForSecEntry.asm"

%ifdef ARCH_X64
-%include "Ia32/Flat32ToFlat64.asm"
+%include "Ia32/Init32.asm"
%include "Ia32/PageTables64.asm"
%endif

--
2.29.2.windows.2


Re: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to support Tdx

Yao, Jiewen
 

Hi Min, Brijesh, James
I feel very frustrated when I review the existing OVMF reset vector.

A big problem is that this code mixed too many SEV stuff, and we are trying to add more TDX stuff in *one* file, without clear isolation.

Take PageTables64.asm as example, here are the symbols. (* means it is newly added.)
=================
CheckSevFeatures:
GetSevEncBit:
SevEncBitLowHlt:
SevSaveMask:
NoSev:
NoSevEsVcHlt:
NoSevPass:
SevExit:
IsSevEsEnabled:
SevEsDisabled:
SetCr3ForPageTables64:
CheckSev: (*)
SevNotActive:
clearPageTablesMemoryLoop:
pageTableEntriesLoop:
tdClearTdxPageTablesMemoryLoop: (*)
IsSevEs: (*)
pageTableEntries4kLoop:
clearGhcbMemoryLoop:
SetCr3:
SevEsIdtNotCpuid:
SevEsIdtNoCpuidResponse:
SevEsIdtTerminate:
SevEsIdtHlt:
SevEsIdtVmmComm:
NextReg:
VmmDone:
=================

In order to better maintain the ResetVector, may I propose some refinement:
1) The main function only contains the non-TEE function, where TEE == SEV + TDX.
2) The TEE related code is moved to TEE specific standalone file, such *Sev.asm and *Tdx.Asm.

3) We need handle different cases with different pattern.
I hope the patter can be used consistently. As such, the reviewer can easily understand what it is for.

3.1) If TEE function is a hook, then the main function calls TEE function directly. The TEE function need implement a TEE check function (such as IsSev, or IsTdx). For example:
====================
OneTimeCall PreMainFunctionHookSev
OneTimeCall PreMainFunctionHookTdx
MainFunction:
XXXXXX
OneTimeCall PostMainFunctionHookSev
OneTimeCall PostMainFunctionHookTdx
====================
3.2) If TEE function is a replacement for non-TEE function. The main function can call TEE replacement function, then check the return status to decide next step. For example:
====================
OneTimeCall MainFunctionSev
Jz MainFunctionEnd
OneTimeCall MainFunctionTdx
Jz MainFunctionEnd
MainFunction:
XXXXXX
MainFunctionEnd:
====================

4) If we found it is too hard to write code in above patter, we can discuss case by case.

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, July 22, 2021 1:52 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Brijesh Singh <brijesh.singh@amd.com>; Erdem
Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>
Subject: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to support
Tdx

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). But in Non-Td guest the initial state of
CPUs is 16-bit real mode. To resolve this conflict, BITS 16/32 is used
in the very beginning of ResetVector. It will check the 32-bit protected
mode or 16-bit real mode, then jump to the corresponding entry point.
This is done in OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm.

ReloadFlat32.asm load the GDT and set the CR0, then jump to Flat-32 mode.

InitTdx.asm is called to record the Tdx signature ('TDXG') and other tdx
information in a TDX_WORK_AREA which can be used by the other routines in
ResetVector.

Init32.asm is 32-bit initialization code in OvmfPkg. It puts above
ReloadFlat32 and InitTdx together to do the initializaiton for Tdx.

After that Tdx jumps to 64-bit long mode by doing following tasks:
1. SetCr3ForPageTables64
For OVMF, some initial page tables is built at:
PcdOvmfSecPageTablesBase - (PcdOvmfSecPageTablesBase + 0x6000)
This page table supports the 4-level page table.
But Tdx support 4-level and 5-level page table based on the CPU GPA width.
48bit is 4-level paging, 52-bit is 5-level paging.
If 5-level page table is supported (GPAW is 52), then a top level
page directory pointers (1 * 256TB entry) is generated in the
TdxPageTable.
2. Set Cr4
Enable PAE.
3. Adjust Cr3
If GPAW is 48, then Cr3 is PT_ADDR (0). If GPAW is 52, then Cr3 is
TDX_PT_ADDR (0).

Tdx MailBox [0x10, 0x800] is reserved for OS. So we initialize piece of this
area ([0x10, 0x20]) to record the Tdx flag ('TDXG') and other Tdx info so that
they can be used in the following flow.

After all above is successfully done, Tdx jump to SecEntry.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 21 ++++++++
OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm | 47 ++++++++++++++++
OvmfPkg/ResetVector/Ia32/Init32.asm | 34 ++++++++++++
OvmfPkg/ResetVector/Ia32/InitTdx.asm | 57 ++++++++++++++++++++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 41 ++++++++++++++
OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm | 44 +++++++++++++++
OvmfPkg/ResetVector/ResetVector.nasmb | 18 +++++++
7 files changed, 262 insertions(+)
create mode 100644 OvmfPkg/ResetVector/Ia32/Init32.asm
create mode 100644 OvmfPkg/ResetVector/Ia32/InitTdx.asm
create mode 100644 OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index ac86ce69ebe8..a390ed81d021 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -155,10 +155,31 @@ resetVector:
;
; This is where the processor will begin execution
;
+; In IA32 we follow the standard reset vector flow. While in X64, Td guest
+; may be supported. Td guest requires the startup mode to be 32-bit
+; protected mode but the legacy VM startup mode is 16-bit real mode.
+; To make NASM generate such shared entry code that behaves correctly in
+; both 16-bit and 32-bit mode, more BITS directives are added.
+;
+%ifdef ARCH_IA32
+
nop
nop
jmp EarlyBspInitReal16

+%else
+
+ smsw ax
+ test al, 1
+ jz .Real
+BITS 32
+ jmp Main32
+BITS 16
+.Real:
+ jmp EarlyBspInitReal16
+
+%endif
+
ALIGN 16

fourGigabytes:
diff --git a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
index c6d0d898bcd1..2206ca719593 100644
--- a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
+++ b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
@@ -17,6 +17,9 @@ Transition32FlatTo64Flat:

OneTimeCall SetCr3ForPageTables64

+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz TdxTransition32FlatTo64Flat
+
mov eax, cr4
bts eax, 5 ; enable PAE
mov cr4, eax
@@ -65,10 +68,54 @@ EnablePaging:
bts eax, 31 ; set PG
mov cr0, eax ; enable paging

+ jmp _jumpTo64Bit
+
+;
+; Tdx Transition from 32Flat to 64Flat
+;
+TdxTransition32FlatTo64Flat:
+
+ mov eax, cr4
+ bts eax, 5 ; enable PAE
+
+ ;
+ ; byte[TDX_WORK_AREA_PAGELEVEL5] holds the indicator whether 52bit is
supported.
+ ; if it is the case, need to set LA57 and use 5-level paging
+ ;
+ cmp byte[TDX_WORK_AREA_PAGELEVEL5], 0
+ jz .set_cr4
+ bts eax, 12
+.set_cr4:
+ mov cr4, eax
+ mov ebx, cr3
+
+ ;
+ ; if la57 is not set, we are ok
+ ; if using 5-level paging, adjust top-level page directory
+ ;
+ bt eax, 12
+ jnc .set_cr3
+ mov ebx, TDX_PT_ADDR (0)
+.set_cr3:
+ mov cr3, ebx
+
+ mov eax, cr0
+ bts eax, 31 ; set PG
+ mov cr0, eax ; enable paging
+
+_jumpTo64Bit:
jmp LINEAR_CODE64_SEL:ADDR_OF(jumpTo64BitAndLandHere)
+
BITS 64
jumpTo64BitAndLandHere:

+ ;
+ ; For Td guest we are done and jump to the end
+ ;
+ mov eax, TDX_WORK_AREA
+ cmp dword [eax], 0x47584454 ; 'TDXG'
+ jz GoodCompare
+
;
; Check if the second step of the SEV-ES mitigation is to be performed.
;
diff --git a/OvmfPkg/ResetVector/Ia32/Init32.asm
b/OvmfPkg/ResetVector/Ia32/Init32.asm
new file mode 100644
index 000000000000..772adc51e531
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/Init32.asm
@@ -0,0 +1,34 @@
+;------------------------------------------------------------------------------
+; @file
+; 32-bit initialization for Tdx
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS 32
+
+;
+; Modified: EBP
+;
+; @param[in] EBX [6:0] CPU supported GPA width
+; [7:7] 5 level page table support
+; @param[in] ECX [31:0] TDINITVP - Untrusted Configuration
+; @param[in] EDX [31:0] VCPUID
+; @param[in] ESI [31:0] VCPU_Index
+;
+Init32:
+ ;
+ ; Save EBX in EBP because EBX will be changed in ReloadFlat32
+ ;
+ mov ebp, ebx
+
+ OneTimeCall ReloadFlat32
+
+ ;
+ ; Init Tdx
+ ;
+ OneTimeCall InitTdx
+
+ OneTimeCallRet Init32
diff --git a/OvmfPkg/ResetVector/Ia32/InitTdx.asm
b/OvmfPkg/ResetVector/Ia32/InitTdx.asm
new file mode 100644
index 000000000000..de8273da6a0c
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/InitTdx.asm
@@ -0,0 +1,57 @@
+;------------------------------------------------------------------------------
+; @file
+; Initialize TDX_WORK_AREA to record the Tdx flag ('TDXG') and other Tdx info
+; so that the following codes can use these information.
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS 32
+
+;
+; Modified: EBP
+;
+InitTdx:
+ ;
+ ; In Td guest, BSP/AP shares the same entry point
+ ; BSP builds up the page table, while APs shouldn't do the same task.
+ ; Instead, APs just leverage the page table which is built by BSP.
+ ; APs will wait until the page table is ready.
+ ; In Td guest, vCPU 0 is treated as the BSP, the others are APs.
+ ; ESI indicates the vCPU ID.
+ ;
+ cmp esi, 0
+ je tdBspEntry
+
+apWait:
+ cmp byte[TDX_WORK_AREA_PGTBL_READY], 0
+ je apWait
+ jmp doneTdxInit
+
+tdBspEntry:
+ ;
+ ; It is of Tdx Guest
+ ; Save the Tdx info in TDX_WORK_AREA so that the following code can use
+ ; these information.
+ ;
+ mov dword [TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+
+ ;
+ ; EBP[6:0] CPU supported GPA width
+ ;
+ and ebp, 0x3f
+ cmp ebp, 52
+ jl NotPageLevel5
+ mov byte[TDX_WORK_AREA_PAGELEVEL5], 1
+
+NotPageLevel5:
+ ;
+ ; ECX[31:0] TDINITVP - Untrusted Configuration
+ ;
+ mov DWORD[TDX_WORK_AREA_INITVP], ecx
+ mov DWORD[TDX_WORK_AREA_INFO], ebp
+
+doneTdxInit:
+ OneTimeCallRet InitTdx
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 5fae8986d9da..508df6cf5967 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -218,6 +218,24 @@ SevEsDisabled:
;
SetCr3ForPageTables64:

+ ;
+ ; Check Td guest
+ ;
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jnz CheckSev
+
+ xor edx, edx
+
+ ;
+ ; In Td guest, BSP builds the page table and set the flag of
+ ; TDX_WORK_AREA_PGTBL_READY. APs check this flag and then set
+ ; cr3 directly.
+ ;
+ cmp byte[TDX_WORK_AREA_PGTBL_READY], 1
+ jz SetCr3
+ jmp SevNotActive
+
+CheckSev:
OneTimeCall CheckSevFeatures
xor edx, edx
test eax, eax
@@ -277,6 +295,29 @@ pageTableEntriesLoop:
mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
loop pageTableEntriesLoop

+ ;
+ ; If it is Td guest, TdxExtraPageTable should be initialized as well
+ ;
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jnz IsSevEs
+
+ xor eax, eax
+ mov ecx, 0x400
+tdClearTdxPageTablesMemoryLoop:
+ mov dword [ecx * 4 + TDX_PT_ADDR (0) - 4], eax
+ loop tdClearTdxPageTablesMemoryLoop
+
+ xor edx, edx
+ ;
+ ; Top level Page Directory Pointers (1 * 256TB entry)
+ ;
+ mov dword[TDX_PT_ADDR (0)], PT_ADDR (0) + PAGE_PDP_ATTR
+ mov dword[TDX_PT_ADDR (4)], edx
+
+ mov byte[TDX_WORK_AREA_PGTBL_READY], 1
+ jmp SetCr3
+
+IsSevEs:
OneTimeCall IsSevEsEnabled
test eax, eax
jz SetCr3
diff --git a/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
b/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
new file mode 100644
index 000000000000..06d44142625a
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
@@ -0,0 +1,44 @@
+;------------------------------------------------------------------------------
+; @file
+; Load the GDT and set the CR0/CR4, then jump to Flat 32 protected mode.
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+%define SEC_DEFAULT_CR0 0x00000023
+%define SEC_DEFAULT_CR4 0x640
+
+BITS 32
+
+;
+; Modified: EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS
+;
+ReloadFlat32:
+
+ cli
+ mov ebx, ADDR_OF(gdtr)
+ lgdt [ebx]
+
+ mov eax, SEC_DEFAULT_CR0
+ mov cr0, eax
+
+ jmp LINEAR_CODE_SEL:dword ADDR_OF(jumpToFlat32BitAndLandHere)
+BITS 32
+jumpToFlat32BitAndLandHere:
+
+ mov eax, SEC_DEFAULT_CR4
+ mov cr4, eax
+
+ debugShowPostCode POSTCODE_32BIT_MODE
+
+ mov ax, LINEAR_SEL
+ mov ds, ax
+ mov es, ax
+ mov fs, ax
+ mov gs, ax
+ mov ss, ax
+
+ OneTimeCallRet ReloadFlat32
+
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
b/OvmfPkg/ResetVector/ResetVector.nasmb
index b653fe87abd6..3ec163613477 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -106,6 +106,21 @@
%define TDX_EXTRA_PAGE_TABLE_BASE FixedPcdGet32
(PcdOvmfSecGhcbPageTableBase)
%define TDX_EXTRA_PAGE_TABLE_SIZE FixedPcdGet32
(PcdOvmfSecGhcbPageTableSize)

+ ;
+ ; TdMailboxBase [0x10, 0x800] is reserved for OS.
+ ; Td guest initialize piece of this area (TdMailboxBase [0x10,0x20]) to
+ ; record the Td guest info so that this information can be used in the
+ ; following ResetVector flow.
+ ;
+ %define TD_MAILBOX_WORKAREA_OFFSET 0x10
+ %define TDX_WORK_AREA (TDX_MAILBOX_MEMORY_BASE +
TD_MAILBOX_WORKAREA_OFFSET)
+ %define TDX_WORK_AREA_PAGELEVEL5 (TDX_WORK_AREA + 4)
+ %define TDX_WORK_AREA_PGTBL_READY (TDX_WORK_AREA + 5)
+ %define TDX_WORK_AREA_INITVP (TDX_WORK_AREA + 8)
+ %define TDX_WORK_AREA_INFO (TDX_WORK_AREA + 8 + 4)
+
+ %define TDX_PT_ADDR(Offset) (TDX_EXTRA_PAGE_TABLE_BASE + (Offset))
+
%define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) +
(Offset))

%define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
@@ -117,6 +132,9 @@
%define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32
(PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))

%include "X64/TdxMetadata.asm"
+ %include "Ia32/Init32.asm"
+ %include "Ia32/InitTdx.asm"
+ %include "Ia32/ReloadFlat32.asm"

%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/PageTables64.asm"
--
2.29.2.windows.2


Re: [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes

Yao, Jiewen
 

Hi Dov
If this library is only needed by AmdSev build, I feel it should be in AmdSev dir, instead of Ovmf dir. (See my question in previous email).


Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov Murik
Sent: Thursday, July 22, 2021 4:45 PM
To: devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; James Bottomley
<jejb@linux.ibm.com>; Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>;
Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Dov Murik <dovmurik@linux.ibm.com>
Subject: Re: [edk2-devel] [PATCH v4 10/11] OvmfPkg: add
BlobVerifierLibSevHashes


Here's the diff from the v3 of this patch. It's supposed to catch
more cases of bad length fields overflowing the reserved MEMFD space or
the declared length of the table.



diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
index 797d63d18067..372ae6f469e7 100644
--- a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
@@ -94,7 +94,7 @@ VerifyBlob (
)
{
CONST GUID *Guid;
- INT32 Len;
+ INT32 Remaining;
HASH_TABLE *Entry;

if (mHashesTable == NULL || mHashesTableSize == 0) {
@@ -111,9 +111,13 @@ VerifyBlob (
return EFI_ACCESS_DENIED;
}

- for (Entry = mHashesTable, Len = 0;
- Len < (INT32)mHashesTableSize;
- Len += Entry->Len,
+ //
+ // Remaining is INT32 to catch underflow in case Entry->Len has a
+ // very high UINT16 value
+ //
+ for (Entry = mHashesTable, Remaining = mHashesTableSize;
+ Remaining >= sizeof *Entry && Remaining >= Entry->Len;
+ Remaining -= Entry->Len,
Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
UINTN EntrySize;
EFI_STATUS Status;
@@ -125,7 +129,7 @@ VerifyBlob (

DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
Guid));

- EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);
+ EntrySize = Entry->Len - sizeof Entry->Guid - sizeof Entry->Len;
if (EntrySize != SHA256_DIGEST_SIZE) {
DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d != %d\n",
__FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));
@@ -161,7 +165,8 @@ VerifyBlob (
This function always returns success, even if the table can't be
found. The
subsequent VerifyBlob calls will fail if no table was found.

- @retval RETURN_SUCCESS The verifier tables were set up correctly
+ @retval RETURN_SUCCESS The hashes table is set up correctly, or
there is no
+ hashes table
**/
RETURN_STATUS
EFIAPI
@@ -175,15 +180,9 @@ BlobVerifierLibSevHashesConstructor (
mHashesTable = NULL;
mHashesTableSize = 0;

- if (Ptr == NULL || Size == 0) {
- return RETURN_SUCCESS;
- }
-
- if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
- return RETURN_SUCCESS;
- }
-
- if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
+ if (Ptr == NULL || Size < sizeof *Ptr ||
+ !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
+ Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
return RETURN_SUCCESS;
}





On 22/07/2021 11:43, Dov Murik wrote:
Add an implementation for BlobVerifierLib that locates the SEV hashes
table and verifies that the calculated hashes of the kernel, initrd, and
cmdline blobs indeed match the expected hashes stated in the hashes
table.

If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
returned which will cause a failure to load a kernel image.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37 ++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 199
++++++++++++++++++++
2 files changed, 236 insertions(+)

diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
new file mode 100644
index 000000000000..76ca0b8154ce
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
@@ -0,0 +1,37 @@
+## @file

+#

+# Blob verifier library that uses SEV hashes table. The hashes table holds the

+# allowed hashes of the kernel, initrd, and cmdline blobs.

+#

+# Copyright (C) 2021, IBM Corp

+#

+# SPDX-License-Identifier: BSD-2-Clause-Patent

+#

+##

+

+[Defines]

+ INF_VERSION = 1.29

+ BASE_NAME = BlobVerifierLibSevHashes

+ FILE_GUID = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b

+ MODULE_TYPE = BASE

+ VERSION_STRING = 1.0

+ LIBRARY_CLASS = BlobVerifierLib

+ CONSTRUCTOR = BlobVerifierLibSevHashesConstructor

+

+[Sources]

+ BlobVerifierSevHashes.c

+

+[Packages]

+ CryptoPkg/CryptoPkg.dec

+ MdePkg/MdePkg.dec

+ OvmfPkg/OvmfPkg.dec

+

+[LibraryClasses]

+ BaseCryptLib

+ BaseMemoryLib

+ DebugLib

+ PcdLib

+

+[FixedPcd]

+ gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase

+ gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize

diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
new file mode 100644
index 000000000000..372ae6f469e7
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
@@ -0,0 +1,199 @@
+/** @file

+

+ Blob verifier library that uses SEV hashes table. The hashes table holds the

+ allowed hashes of the kernel, initrd, and cmdline blobs.

+

+ Copyright (C) 2021, IBM Corporation

+

+ SPDX-License-Identifier: BSD-2-Clause-Patent

+**/

+

+#include <Library/BaseCryptLib.h>

+#include <Library/BaseLib.h>

+#include <Library/BaseMemoryLib.h>

+#include <Library/DebugLib.h>

+#include <Library/BlobVerifierLib.h>

+

+/**

+ The SEV Hashes table must be in encrypted memory and has the table

+ and its entries described by

+

+ <GUID>|UINT16 <len>|<data>

+

+ With the whole table GUID being 9438d606-4f22-4cc9-b479-a793d411fd21

+

+ The current possible table entries are for the kernel, the initrd

+ and the cmdline:

+

+ 4de79437-abd2-427f-b835-d5b172d2045b kernel

+ 44baf731-3a2f-4bd7-9af1-41e29169781d initrd

+ 97d02dd8-bd20-4c94-aa78-e7714d36ab2a cmdline

+

+ The size of the entry is used to identify the hash, but the

+ expectation is that it will be 32 bytes of SHA-256.

+**/

+

+#define SEV_HASH_TABLE_GUID \

+ (GUID) { 0x9438d606, 0x4f22, 0x4cc9, { 0xb4, 0x79, 0xa7, 0x93, 0xd4, 0x11,
0xfd, 0x21 } }

+#define SEV_KERNEL_HASH_GUID \

+ (GUID) { 0x4de79437, 0xabd2, 0x427f, { 0xb8, 0x35, 0xd5, 0xb1, 0x72, 0xd2,
0x04, 0x5b } }

+#define SEV_INITRD_HASH_GUID \

+ (GUID) { 0x44baf731, 0x3a2f, 0x4bd7, { 0x9a, 0xf1, 0x41, 0xe2, 0x91, 0x69,
0x78, 0x1d } }

+#define SEV_CMDLINE_HASH_GUID \

+ (GUID) { 0x97d02dd8, 0xbd20, 0x4c94, { 0xaa, 0x78, 0xe7, 0x71, 0x4d, 0x36,
0xab, 0x2a } }

+

+STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;

+STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;

+STATIC CONST EFI_GUID mSevCmdlineHashGuid =
SEV_CMDLINE_HASH_GUID;

+

+#pragma pack (1)

+typedef struct {

+ GUID Guid;

+ UINT16 Len;

+ UINT8 Data[];

+} HASH_TABLE;

+#pragma pack ()

+

+STATIC HASH_TABLE *mHashesTable;

+STATIC UINT16 mHashesTableSize;

+

+STATIC

+CONST GUID*

+FindBlobEntryGuid (

+ IN CONST CHAR16 *BlobName

+ )

+{

+ if (StrCmp (BlobName, L"kernel") == 0) {

+ return &mSevKernelHashGuid;

+ } else if (StrCmp (BlobName, L"initrd") == 0) {

+ return &mSevInitrdHashGuid;

+ } else if (StrCmp (BlobName, L"cmdline") == 0) {

+ return &mSevCmdlineHashGuid;

+ } else {

+ return NULL;

+ }

+}

+

+/**

+ Verify blob from an external source.

+

+ @param[in] BlobName The name of the blob

+ @param[in] Buf The data of the blob

+ @param[in] BufSize The size of the blob in bytes

+

+ @retval EFI_SUCCESS The blob was verified successfully.

+ @retval EFI_ACCESS_DENIED The blob could not be verified, and
therefore

+ should be considered non-secure.

+**/

+EFI_STATUS

+EFIAPI

+VerifyBlob (

+ IN CONST CHAR16 *BlobName,

+ IN CONST VOID *Buf,

+ IN UINT32 BufSize

+ )

+{

+ CONST GUID *Guid;

+ INT32 Remaining;

+ HASH_TABLE *Entry;

+

+ if (mHashesTable == NULL || mHashesTableSize == 0) {

+ DEBUG ((DEBUG_ERROR,

+ "%a: Verifier called but no hashes table discoverd in MEMFD\n",

+ __FUNCTION__));

+ return EFI_ACCESS_DENIED;

+ }

+

+ Guid = FindBlobEntryGuid (BlobName);

+ if (Guid == NULL) {

+ DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n",
__FUNCTION__,

+ BlobName));

+ return EFI_ACCESS_DENIED;

+ }

+

+ //

+ // Remaining is INT32 to catch underflow in case Entry->Len has a

+ // very high UINT16 value

+ //

+ for (Entry = mHashesTable, Remaining = mHashesTableSize;

+ Remaining >= sizeof *Entry && Remaining >= Entry->Len;

+ Remaining -= Entry->Len,

+ Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {

+ UINTN EntrySize;

+ EFI_STATUS Status;

+ UINT8 Hash[SHA256_DIGEST_SIZE];

+

+ if (!CompareGuid (&Entry->Guid, Guid)) {

+ continue;

+ }

+

+ DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
Guid));

+

+ EntrySize = Entry->Len - sizeof Entry->Guid - sizeof Entry->Len;

+ if (EntrySize != SHA256_DIGEST_SIZE) {

+ DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d != %d\n",

+ __FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));

+ return EFI_ACCESS_DENIED;

+ }

+

+ //

+ // Calculate the buffer's hash and verify that it is identical to the

+ // expected hash table entry

+ //

+ Sha256HashAll (Buf, BufSize, Hash);

+

+ if (CompareMem (Entry->Data, Hash, EntrySize) == 0) {

+ Status = EFI_SUCCESS;

+ DEBUG ((DEBUG_INFO, "%a: Hash comparison succeeded for \"%s\"\n",

+ __FUNCTION__, BlobName));

+ } else {

+ Status = EFI_ACCESS_DENIED;

+ DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",

+ __FUNCTION__, BlobName));

+ }

+ return Status;

+ }

+

+ DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n",
__FUNCTION__,

+ Guid));

+ return EFI_ACCESS_DENIED;

+}

+

+/**

+ Locate the SEV hashes table.

+

+ This function always returns success, even if the table can't be found. The

+ subsequent VerifyBlob calls will fail if no table was found.

+

+ @retval RETURN_SUCCESS The hashes table is set up correctly, or there is
no

+ hashes table

+**/

+RETURN_STATUS

+EFIAPI

+BlobVerifierLibSevHashesConstructor (

+ VOID

+ )

+{

+ HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64
(PcdQemuHashTableBase);

+ UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);

+

+ mHashesTable = NULL;

+ mHashesTableSize = 0;

+

+ if (Ptr == NULL || Size < sizeof *Ptr ||

+ !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||

+ Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {

+ return RETURN_SUCCESS;

+ }

+

+ DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure
location\n",

+ __FUNCTION__));

+

+ mHashesTable = (HASH_TABLE *)Ptr->Data;

+ mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;

+

+ DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n",
__FUNCTION__,

+ mHashesTable, mHashesTableSize));

+

+ return RETURN_SUCCESS;

+}



Re: [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline

Yao, Jiewen
 

Hi
I am reviewing this patch series. I am OK with most parts.

And I do have one question:
May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or OvmfPkg directly?

My original understanding is:
If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should be OvmfPkg.
If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it should be in OvmfPkg\AmdSev.

Am I right?

Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov Murik
Sent: Thursday, July 22, 2021 4:43 PM
To: devel@edk2.groups.io
Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
<tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>;
Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
Mujawar <sami.mujawar@arm.com>
Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
kernel/initrd/cmdline

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

Booting with SEV prevented the loading of kernel, initrd, and kernel
command-line via QEMU fw_cfg interface because they arrive from the VMM
which is untrusted in SEV.

However, in some cases the kernel, initrd, and cmdline are not secret
but should not be modified by the host. In such a case, we want to
verify inside the trusted VM that the kernel, initrd, and cmdline are
indeed the ones expected by the Guest Owner, and only if that is the
case go on and boot them up (removing the need for grub inside OVMF in
that mode).

This patch series reserves an area in MEMFD (previously the last 1KB of
the launch secret page) which will contain the hashes of these three
blobs (kernel, initrd, cmdline), each under its own GUID entry. This
tables of hashes is populated by QEMU before launch, and encrypted as
part of the initial VM memory; this makes sure these hashes are part of
the SEV measurement (which has to be approved by the Guest Owner for
secret injection, for example). Note that populating the hashes table
requires QEMU support [1].

OVMF parses the table of hashes populated by QEMU (patch 10), and as it
reads the fw_cfg blobs from QEMU, it will verify each one against the
expected hash. This is all done inside the trusted VM context. If all
the hashes are correct, boot of the kernel is allowed to continue.

Any attempt by QEMU to modify the kernel, initrd, cmdline (including
dropping one of them), or to modify the OVMF code that verifies those
hashes, will cause the initial SEV measurement to change and therefore
will be detectable by the Guest Owner during launch before secret
injection.

Relevant part of OVMF serial log during boot with AmdSevX86 build and
QEMU with -kernel/-initrd/-append:

...
BlobVerifierLibSevHashesConstructor: Found injected hashes table in secure
location
Select Item: 0x17
Select Item: 0x8
FetchBlob: loading 7379328 bytes for "kernel"
Select Item: 0x18
Select Item: 0x11
VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
VerifyBlob: Hash comparison succeeded for "kernel"
Select Item: 0xB
FetchBlob: loading 12483878 bytes for "initrd"
Select Item: 0x12
VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
VerifyBlob: Hash comparison succeeded for "initrd"
Select Item: 0x14
FetchBlob: loading 86 bytes for "cmdline"
Select Item: 0x15
VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
VerifyBlob: Hash comparison succeeded for "cmdline"
...

The patch series is organized as follows:

1: Simple comment fix in adjacent area in the code.
2: Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
fetching.
3: Allow the (previously blocked) usage of -kernel in AmdSevX64.
4-7: Add BlobVerifierLib with null implementation and use it in the correct
location in QemuKernelLoaderFsDxe.
8-9: Reserve memory for hashes table, declare this area in the reset vector.
10-11: Add the secure implementation BlobVerifierLibSevHashes and use it in
AmdSevX64 builds.

[1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-
dovmurik@linux.ibm.com/

Code is at
https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v4

v4 changes:
- BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
when parsing the SEV hashes table structure

v3: https://edk2.groups.io/g/devel/message/77955
v3 changes:
- Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
DebugLib reference, fix doxygen comments, add missing IN attribute
- Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
doxygen comments, add missing IN attribute,
calculate buffer hash only when the guid is found in hashes table
- SecretPei: use ALIGN_VALUE to round the hob size
- Coding style fixes
- Add missing 'Ref:' in patch 1 commit message
- Fix phrasing and typos in commit messages
- Remove Cc: Laszlo from series

v2: https://edk2.groups.io/g/devel/message/77505
v2 changes:
- Use the last 1KB of the existing SEV launch secret page for hashes table
(instead of reserving a whole new MEMFD page).
- Build on top of commit cf203024745f ("OvmfPkg/GenericQemuLoadImageLib:
Read
cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location in
which all of kernel/initrd/cmdline are fetched from QEMU.
- Use static linking of the two BlobVerifierLib implemenatations.
- Reorganize series.

v1: https://edk2.groups.io/g/devel/message/75567

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>

---

Ard: please review patch 6 (ArmVirtPkg). Thanks.

Tom, Brijesh: I'll also send the diff for patch 10. Thanks.

---

Dov Murik (8):
OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
OvmfPkg: add library class BlobVerifierLib with null implementation
OvmfPkg: add BlobVerifierLibNull to DSC
ArmVirtPkg: add BlobVerifierLibNull to DSC
OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
OvmfPkg/AmdSev/SecretPei: build hob for full page
OvmfPkg: add BlobVerifierLibSevHashes
OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

James Bottomley (3):
OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

OvmfPkg/OvmfPkg.dec | 9 +
ArmVirtPkg/ArmVirtQemu.dsc | 5 +-
ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.dsc | 9 +-
OvmfPkg/OvmfPkgIa32.dsc | 5 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 5 +-
OvmfPkg/OvmfPkgX64.dsc | 5 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 5 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf | 24
+++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37
++++

OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.
inf | 2 +
OvmfPkg/ResetVector/ResetVector.inf | 2 +
OvmfPkg/Include/Library/BlobVerifierLib.h | 38 ++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h |
11 ++
OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 2 +-
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 3 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33 ++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 199
++++++++++++++++++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c |
5 +
OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c | 0
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
| 9 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 +
23 files changed, 425 insertions(+), 10 deletions(-)
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
create mode 100644
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
copy OvmfPkg/Library/{PlatformBootManagerLib =>
PlatformBootManagerLibGrub}/QemuKernel.c (100%)

--
2.25.1





Re: [PATCH v3] OvmfPkg: Remove unused print service driver (PrintDxe)

Yao, Jiewen
 

Hi Philippe
I cannot find the Bugzilla for this patch.

Would you please file an Bugzilla and send a new version with the Bugzilla id?

Then I could merge it.

Thank you
Yao Jiewen

-----Original Message-----
From: Yao, Jiewen
Sent: Wednesday, July 21, 2021 7:46 PM
To: Philippe Mathieu-Daudé <philmd@redhat.com>; devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Xu, Min
M <min.m.xu@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>
Subject: RE: [PATCH v3] OvmfPkg: Remove unused print service driver (PrintDxe)

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@redhat.com>
Sent: Wednesday, July 21, 2021 4:21 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Xu, Min
M <min.m.xu@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH v3] OvmfPkg: Remove unused print service driver
(PrintDxe)

Ping?

On 7/7/21 8:02 PM, Philippe Mathieu-Daudé wrote:
From: Philippe Mathieu-Daude <philmd@redhat.com>

PrintDxe produces gEfiPrint2ProtocolGuid and gEfiPrint2SProtocolGuid,
and those are consumed by the following PrintLib instance:

MdeModulePkg/Library/DxePrintLibPrint2Protocol/DxePrintLibPrint2Protocol.inf

However, none of the OVMF DSC files contain such a PrintLib class
resolution, so none of the OVMF platforms need PrintDxe.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 1 -
OvmfPkg/Bhyve/BhyveX64.dsc | 1 -
OvmfPkg/OvmfPkgIa32.dsc | 1 -
OvmfPkg/OvmfPkgIa32X64.dsc | 1 -
OvmfPkg/OvmfPkgX64.dsc | 1 -
OvmfPkg/OvmfXen.dsc | 1 -
OvmfPkg/AmdSev/AmdSevX64.fdf | 1 -
OvmfPkg/Bhyve/BhyveX64.fdf | 1 -
OvmfPkg/OvmfPkgIa32.fdf | 1 -
OvmfPkg/OvmfPkgIa32X64.fdf | 1 -
OvmfPkg/OvmfPkgX64.fdf | 1 -
OvmfPkg/OvmfXen.fdf | 1 -
12 files changed, 12 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 1d487befae08..d1974b4a6873 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -722,7 +722,6 @@ [Components]
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
}
- MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index 006831449518..c08fa9bdbf5b 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -673,7 +673,6 @@ [Components]
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
}
- MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index f53efeae7986..dff4b97b37c0 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -816,7 +816,6 @@ [Components]
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
}
- MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index b3662e17f256..f3df655c990e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -830,7 +830,6 @@ [Components.X64]
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
}
- MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 0a237a905866..dc9a2720f9b2 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -828,7 +828,6 @@ [Components]
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
}
- MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 3c1ca6bfd493..aee91a61e7c3 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -610,7 +610,6 @@ [Components]
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
}
- MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf
b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 9977b0f00a18..42f120d016e1 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -234,7 +234,6 @@ [FV.DXEFV]
INF MdeModulePkg/Application/UiApp/UiApp.inf
INF OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
-INF MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
index b3b4d44cef34..e8227f865f75 100644
--- a/OvmfPkg/Bhyve/BhyveX64.fdf
+++ b/OvmfPkg/Bhyve/BhyveX64.fdf
@@ -233,7 +233,6 @@ [FV.DXEFV]
INF MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
INF MdeModulePkg/Application/UiApp/UiApp.inf
INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
-INF MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 04b41445ca34..031eb4225c53 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -254,7 +254,6 @@ [FV.DXEFV]
INF MdeModulePkg/Application/UiApp/UiApp.inf
INF OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
-INF MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 02fd8f0c413e..7194f08e6024 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -255,7 +255,6 @@ [FV.DXEFV]
INF MdeModulePkg/Application/UiApp/UiApp.inf
INF OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
-INF MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 5fa8c0895808..b304e3149d4f 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -267,7 +267,6 @@ [FV.DXEFV]
INF MdeModulePkg/Application/UiApp/UiApp.inf
INF OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
-INF MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index aeb9336fd5b7..d109341d2890 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -325,7 +325,6 @@ [FV.DXEFV]
INF MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
INF MdeModulePkg/Application/UiApp/UiApp.inf
INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
-INF MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf


Re: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to support Tdx

Min Xu
 

On July 23, 2021 9:35 PM, Lendacky, Thomas wrote:
On 7/22/21 5:58 PM, Xu, Min M wrote:
On July 23, 2021 1:08 AM, Tom Lendacky wrote:
On 7/22/21 12:52 AM, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

diff --git a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
index c6d0d898bcd1..2206ca719593 100644
--- a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
+++ b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
@@ -17,6 +17,9 @@ Transition32FlatTo64Flat:

OneTimeCall SetCr3ForPageTables64

+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz TdxTransition32FlatTo64Flat
+
Is the memory area guaranteed to be zeroed for legacy guests?
Hopefully, this won't trip up a non-TDX guest with a false match (highly
unlikely, though).
TDX_WORK_AREA is piece of TdxMailbox which is located in the MEMFD
started from PcdOvmfSecGhcbBackupBase. In Td guest, this memory region
is initialized to all-0 by host VMM. In legacy guests, I am not sure
what's the initialized value it is. So 'TDXG' is checked to guarantee it is Td-
guest or not.
Since Tdx re-use the memory region (PcdOvmfSecGhcbBackupBase) as the
TDX_WORK_AREA, and @Tom Lendacky you should be the original owner of
PcdOvmfSecGhcbBackupBase, can this area be cleared in the beginning of
ResetVector in legacy guests? Or I should better create a TDX specific
work area in MEMFD to guarantee the Td And Non-Td check?
I believe PcdOvmfSecGhcbBackupBase can be cleared early. For SEV-ES, it isn't
shared with the hypervisor, so clearing it before activating the pagetables can
be done (it will be treated as encrypted before paging is enabled and mapped
as encrypted after paging is enabled) and for a legacy guest the mapping
doesn't matter. It isn't required to be cleared today, so if you do add
something, be sure to put a comment in there about why it's being done. No
need for a new area.

The possibility of random data being there that matches 'TDXG' is extremely
low. But better safe than sorry, I guess.
Thanks Tom. I will update it in the next version.
Thanks!
Xu, Min


[PATCH 3/3] BaseTools: Drop check for distutils.utils

Cole
 

distutils.utils is no longer used anywhere, so this check can
be dropped.

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
BaseTools/Tests/RunTests.py | 7 -------
1 file changed, 7 deletions(-)

diff --git a/BaseTools/Tests/RunTests.py b/BaseTools/Tests/RunTests.py
index 81af736cd8..934683a446 100644
--- a/BaseTools/Tests/RunTests.py
+++ b/BaseTools/Tests/RunTests.py
@@ -13,13 +13,6 @@ import os
import sys
import unittest

-try:
- import distutils.util
-except ModuleNotFoundError:
- sys.exit('''
-Python reported: "No module named 'distutils.util"
-''')
-
import TestTools

def GetCTestSuite():
--
2.31.1


[PATCH 2/3] python: Replace distutils.utils.split_quotes with shlex.split

Cole
 

distutils is deprecated and may be removed in python 3.12.
Use shlex.split which has been around since python 2.3.

shlex.split does not split on all the ASCII control characters that
split_quoted will[1], but for edk2 usage I don't think that matters.

[1] https://stackoverflow.com/questions/54999301/what-is-the-difference-between-distutils-util-split-quoted-and-shlex-split

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
BaseTools/Source/Python/AutoGen/UniClassObject.py | 4 ++--
BaseTools/Source/Python/UPT/Library/UniClassObject.py | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/UniClassObject.py b/BaseTools/Source/Python/AutoGen/UniClassObject.py
index 883c2356e0..b16330e368 100644
--- a/BaseTools/Source/Python/AutoGen/UniClassObject.py
+++ b/BaseTools/Source/Python/AutoGen/UniClassObject.py
@@ -12,7 +12,7 @@
#
from __future__ import print_function
import Common.LongFilePathOs as os, codecs, re
-import distutils.util
+import shlex
import Common.EdkLogger as EdkLogger
from io import BytesIO
from Common.BuildToolError import *
@@ -233,7 +233,7 @@ class UniFileClassObject(object):
# Get Language definition
#
def GetLangDef(self, File, Line):
- Lang = distutils.util.split_quoted((Line.split(u"//")[0]))
+ Lang = shlex.split(Line.split(u"//")[0])
if len(Lang) != 3:
try:
FileIn = UniFileClassObject.OpenUniFile(LongFilePath(File.Path))
diff --git a/BaseTools/Source/Python/UPT/Library/UniClassObject.py b/BaseTools/Source/Python/UPT/Library/UniClassObject.py
index d25f300146..8c44dc2252 100644
--- a/BaseTools/Source/Python/UPT/Library/UniClassObject.py
+++ b/BaseTools/Source/Python/UPT/Library/UniClassObject.py
@@ -14,7 +14,7 @@ from __future__ import print_function
# Import Modules
#
import os, codecs, re
-import distutils.util
+import shlex
from Logger import ToolError
from Logger import Log as EdkLogger
from Logger import StringTable as ST
@@ -320,7 +320,7 @@ class UniFileClassObject(object):
# Get Language definition
#
def GetLangDef(self, File, Line):
- Lang = distutils.util.split_quoted((Line.split(u"//")[0]))
+ Lang = shlex.split(Line.split(u"//")[0])
if len(Lang) != 3:
try:
FileIn = codecs.open(File.Path, mode='rb', encoding='utf_8').readlines()
--
2.31.1

6401 - 6420 of 84489