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

[Bob Feng]

This patch set is good to me.

Reviewed-by: Bob Feng 

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Cole
Sent: Saturday, July 24, 2021 4:02 AM
To: devel@edk2.groups.io
Cc: Cole Robinson 
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








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78163): https://edk2.groups.io/g/devel/message/78163
Mute This Topic: https://groups.io/mt/84409128/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] 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
 *   This is document on the SourceForge to Github Quick 
Start
 page
  2.  Create and checkout a topic branch for new feature or bug fix

$ git checkout -b  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 
 *   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 
-
*   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 
Sent: Friday, July 23, 2021 11:15 AM
To: devel@edk2.groups.io
Cc: Goetz, Philippe C ; Nagar, Rupa 
; Luo, Heng ; Zhuang, Qihua 
; Lu, James 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78162): https://edk2.groups.io/g/devel/message/78162
Mute This Topic: https://groups.io/mt/84394505/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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 
> Sent: Monday, July 26, 2021 5:10 AM
> To: devel@edk2.groups.io; dovmu...@linux.ibm.com; Yao, Jiewen
> 
> Cc: Tobin Feldman-Fitzthum ; Tobin Feldman-Fitzthum
> ; Jim Cadden ; Hubertus Franke
> ; Ard Biesheuvel ; Justen,
> Jordan L ; Ashish Kalra ;
> Brijesh Singh ; Erdem Aktas
> ; Xu, Min M ; Tom Lendacky
> ; Leif Lindholm ; Sami
> Mujawar 
> 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
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78161): https://edk2.groups.io/g/devel/message/78161
Mute This Topic: https://groups.io/mt/84375116/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78160): https://edk2.groups.io/g/devel/message/78160
Mute This Topic: https://groups.io/mt/84375116/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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 
>> Sent: Sunday, July 25, 2021 3:53 PM
>> To: Yao, Jiewen ; devel@edk2.groups.io
>> Cc: Tobin Feldman-Fitzthum ; Tobin Feldman-Fitzthum
>> ; Jim Cadden ; James Bottomley
>> ; Hubertus Franke ; Ard Biesheuvel
>> ; Justen, Jordan L ;
>> Ashish Kalra ; Brijesh Singh ;
>> Erdem Aktas ; Xu, Min M ;
>> Tom Lendacky ; Leif Lindholm
>> ; Sami Mujawar ; Dov Murik
>> 
>> 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  On Behalf Of Dov
>> Murik
 Sent: Thursday, July 22, 2021 4:43 PM
 To: devel@edk2.groups.io
 Cc: Dov Murik ; Tobin Feldman-Fitzthum
 ; Tobin Feldman-Fitzthum ; Jim
 Cadden ; James Bottomley ;
 Hubertus Franke ; Ard Biesheuvel
 ; Justen, Jordan L ;
 Ashish Kalra ; Brijesh Singh
>> ;
 Erdem Aktas ; Yao, Jiewen
>> ;
 Xu, Min M ; Tom 

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

[Yao, Jiewen]

Thank you Dov.
Comment below:

> -Original Message-
> From: Dov Murik 
> Sent: Sunday, July 25, 2021 3:53 PM
> To: Yao, Jiewen ; devel@edk2.groups.io
> Cc: Tobin Feldman-Fitzthum ; Tobin Feldman-Fitzthum
> ; Jim Cadden ; James Bottomley
> ; Hubertus Franke ; Ard Biesheuvel
> ; Justen, Jordan L ;
> Ashish Kalra ; Brijesh Singh ;
> Erdem Aktas ; Xu, Min M ;
> Tom Lendacky ; Leif Lindholm
> ; Sami Mujawar ; Dov Murik
> 
> 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  On Behalf Of Dov
> Murik
> >> Sent: Thursday, July 22, 2021 4:43 PM
> >> To: devel@edk2.groups.io
> >> Cc: Dov Murik ; Tobin Feldman-Fitzthum
> >> ; Tobin Feldman-Fitzthum ; Jim
> >> Cadden ; James Bottomley ;
> >> Hubertus Franke ; Ard Biesheuvel
> >> ; Justen, Jordan L ;
> >> Ashish Kalra ; Brijesh Singh
> ;
> >> Erdem Aktas ; Yao, Jiewen
> ;
> >> Xu, Min M ; Tom Lendacky
> >> ; Leif Lindholm ; Sami
> >> Mujawar 
> >> 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 

Re: [edk2-devel] [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  On Behalf Of Dov Murik
>> Sent: Thursday, July 22, 2021 4:43 PM
>> To: devel@edk2.groups.io
>> Cc: Dov Murik ; Tobin Feldman-Fitzthum
>> ; Tobin Feldman-Fitzthum ; Jim
>> Cadden ; James Bottomley ;
>> Hubertus Franke ; Ard Biesheuvel
>> ; Justen, Jordan L ;
>> Ashish Kalra ; Brijesh Singh ;
>> Erdem Aktas ; Yao, Jiewen ;
>> Xu, Min M ; Tom Lendacky
>> ; Leif Lindholm ; Sami
>> Mujawar 
>> 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 

Re: [edk2-devel] [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 
> Sent: Sunday, July 25, 2021 2:01 PM
> To: Xu, Min M ; devel@edk2.groups.io
> Cc: Ard Biesheuvel ; Brijesh Singh
> ; Erdem Aktas ; James
> Bottomley ; Tom Lendacky
> 
> 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:
>   XX
>   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:
>   XX
> 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 
> > Sent: Thursday, July 22, 2021 1:52 PM
> > To: devel@edk2.groups.io
> > Cc: Xu, Min M ; Ard Biesheuvel
> > ; Brijesh Singh ;
> > Erdem Aktas ; James Bottomley
> > ; Yao, Jiewen ; Tom
> Lendacky
> > 
> > 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 
> > Cc: Brijesh Singh 
> > Cc: 

Re: [edk2-devel] [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 
> Sent: Sunday, July 25, 2021 3:44 PM
> To: Xu, Min M ; devel@edk2.groups.io
> Cc: Dong, Eric ; Ni, Ray 
> 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 
> > Sent: Sunday, July 25, 2021 3:42 PM
> > To: Yao, Jiewen ; devel@edk2.groups.io
> > Cc: Dong, Eric ; Ni, Ray 
> > 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 
> > > > Sent: Thursday, July 22, 2021 1:52 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Xu, Min M ; Dong, Eric
> > > > ; Ni, Ray ; Yao, Jiewen
> > > > 
> > > > 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 
> > > > Cc: Ray Ni 
> > > > Cc: Jiewen Yao 
> > > > Signed-off-by: Min Xu 
> > > > ---
> > > >  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 ..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.
> > > > +;
> > > > +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:
> > > >
> > > >  BITS32
> > > >
> > > > +%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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78155): https://edk2.groups.io/g/devel/message/78155
Mute This Topic: 

Re: [edk2-devel] [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 
> Sent: Sunday, July 25, 2021 3:42 PM
> To: Yao, Jiewen ; devel@edk2.groups.io
> Cc: Dong, Eric ; Ni, Ray 
> 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 
> > > Sent: Thursday, July 22, 2021 1:52 PM
> > > To: devel@edk2.groups.io
> > > Cc: Xu, Min M ; Dong, Eric ;
> > > Ni, Ray ; Yao, Jiewen 
> > > 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 
> > > Cc: Ray Ni 
> > > Cc: Jiewen Yao 
> > > Signed-off-by: Min Xu 
> > > ---
> > >  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 ..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. ;
> > > +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:
> > >
> > >  BITS32
> > >
> > > +%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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78154): https://edk2.groups.io/g/devel/message/78154
Mute This Topic: https://groups.io/mt/84373829/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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 
> > Sent: Thursday, July 22, 2021 1:52 PM
> > To: devel@edk2.groups.io
> > Cc: Xu, Min M ; Dong, Eric ;
> > Ni, Ray ; Yao, Jiewen 
> > 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 
> > Cc: Ray Ni 
> > Cc: Jiewen Yao 
> > Signed-off-by: Min Xu 
> > ---
> >  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 ..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. ;
> > +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:
> >
> >  BITS32
> >
> > +%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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78153): https://edk2.groups.io/g/devel/message/78153
Mute This Topic: https://groups.io/mt/84373829/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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 
> Sent: Thursday, July 22, 2021 1:52 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M ; Dong, Eric ; Ni,
> Ray ; Yao, Jiewen 
> 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 
> Cc: Ray Ni 
> Cc: Jiewen Yao 
> Signed-off-by: Min Xu 
> ---
>  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 ..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.
> +; 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:
> 
>  BITS32
> 
> +%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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78152): https://edk2.groups.io/g/devel/message/78152
Mute This Topic: https://groups.io/mt/84373829/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [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:
XX
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:
XX
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 
> Sent: Thursday, July 22, 2021 1:52 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M ; Ard Biesheuvel
> ; Brijesh Singh ; Erdem
> Aktas ; James Bottomley ;
> Yao, Jiewen ; Tom Lendacky
> 
> 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 
> Cc: Brijesh Singh 
> Cc: Erdem Aktas 
> Cc: James Bottomley 
> Cc: Jiewen Yao 
> Cc: Tom Lendacky 
> Signed-off-by: Min Xu 
> ---
>  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