On 10/27/23 03:05, Tan, Dun wrote:
Hi all,
Could you please help to review this patch set? In this patch set, the IoLib
instance BaseIoLibIntrinsic is modified to support AMD SEV feature and the
BaseIoLibIntrinsicSev is removed.
Also could you help to do a test on AMD processor to make sure that the SEV
feature still works good with this patch set?
I was able to test SEV, SEV-ES and SEV-SNP guests successfully at each
step of the patchset.
However, you are unrolling the string I/O for everyone, now, not just SEV
guests. Is that acceptable to the community? I think there need to be
comments in IoLibFifo.c around the new code about why the access is
unrolled/looping so that someone down the road doesn't come along and try
to use string I/O again.
From a commit message standpoint, you have up to 74 characters per line
to use and I see most of your messages do not make use of that. Also, you
use sev when it should be SEV. Using SEV will make grep'ing commit
messages simpler.
Thanks,
Tom
Thanks,
Dun
-----Original Message-----
From: Tan, Dun
Sent: Friday, October 27, 2023 3:35 PM
To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH 0/7] Support Tdx and sev in BaseIoLibIntrinsic
and remove BaseIoLibIntrinsicSev
Thanks for the suggestion.
I'll update the test result once I finished the test. Also the abstract message
in this patch has been modified to mention that this patch should not be merged
now.
Thanks,
Dun
-----Original Message-----
From: Yao, Jiewen <jiewen....@intel.com>
Sent: Friday, October 27, 2023 3:07 PM
To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH 0/7] Support Tdx and sev in BaseIoLibIntrinsic
and remove BaseIoLibIntrinsicSev
Here is my suggestion:
1) Please perform the test to ensure the functional part is correct.
Without that, how can people know you are doing things right?
2) If you do not run any test, before you send out patch, please call out that
clearly.
That is important to reminder the maintainer: Don't merge, even if it pass
review.
Otherwise, once the review passed, the maintainer may merge it.
I don't think that is the intention.
Thank you
Yao, Jiewen
-----Original Message-----
From: Tan, Dun <dun....@intel.com>
Sent: Friday, October 27, 2023 2:32 PM
To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH 0/7] Support Tdx and sev in
BaseIoLibIntrinsic and remove BaseIoLibIntrinsicSev
Hi Jiewen,
Currently I'm working on the Tdx test. Since the patch set doesn't
change the code logic when Tdx or SEV is enabled, so I want to send
out the patch as soon as possible to see if there is any comments from
community.
I will include AMD SEV reviewer in this patch series. Thanks for reminding.
Thanks,
Dun
-----Original Message-----
From: Yao, Jiewen <jiewen....@intel.com>
Sent: Friday, October 27, 2023 1:49 PM
To: devel@edk2.groups.io; Tan, Dun <dun....@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/7] Support Tdx and sev in
BaseIoLibIntrinsic and remove BaseIoLibIntrinsicSev
HI
Since this impact TDX and SEV, would you please let me know what kind
of test you have done?
Have you validated TDX and SEV before you submit the patch? Please
describe that clearly in your patch description.
Also please include AMD SEV reviewer in this patch series.
Thank you
Yao, Jiewen
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
duntan
Sent: Friday, October 27, 2023 1:43 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] [PATCH 0/7] Support Tdx and sev in
BaseIoLibIntrinsic and remove BaseIoLibIntrinsicSev (Don't merge
because the test hasn't been completed yet.)
The goal is to have single BaseIoLibIntrinsic instance that can also
used for sev and Tdx.
In this patch set, string I/O instructions are deleted in IoRead/WriteFifo API.
Then change the source file of BaseIoLibIntrinsic to also support
Tdx and sev feature. So BaseIoLibIntrinsicSev and related assembly
code can be
removed.
Dun Tan (7):
MdePkg: Create TdxLibNull.inf instance
MdePkg: Add CcProbeLibNull and TdxLibNull implement
MdePkg: simplify IoRead/WriteFifo in IoLibFifo.c
MdePkg:support Tdx and sev in BaseIoLibIntrinsic
OvmfPkg: Add CcProbeLib in PlatformInitLib.inf
OvmfPkg: use BaseIoLibIntrinsic.inf in dsc files
MdePkg:remove BaseIoLibIntrinsicSev related code
MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf | 14
++++++++++---
-
MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf | 61
------------------
-------------------------------------------
MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm | 131
-----------------
---
--------------------------------------------------------------------
--
----------------------------
-------------
MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm | 293
---------------
---
--------------------------------------------------------------------
--
----------------------------
--------------------------------------------------------------------
--
----------------------------
-------------------------------------------------------------------------------
MdePkg/Library/BaseIoLibIntrinsic/IoLibFifo.c | 45
+++++++++++++++++++++++++++++++++++++--------
MdePkg/Library/BaseIoLibIntrinsic/IoLibSev.h | 166
----------------------
--
--------------------------------------------------------------------
--
----------------------------
--------------------------------------------
MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm | 120
------------------
--
--------------------------------------------------------------------
--
----------------------------
--
MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm | 282
----------------
--
--------------------------------------------------------------------
--
----------------------------
--------------------------------------------------------------------
--
----------------------------
--------------------------------------------------------------------
MdePkg/Library/TdxLib/TdxLibNull.inf | 21
+++++++++++++++++++++
MdePkg/MdeLibs.dsc.inc | 4 +++-
MdePkg/MdePkg.dsc | 2 +-
OvmfPkg/AmdSev/AmdSevX64.dsc | 2 +-
OvmfPkg/Bhyve/BhyveX64.dsc | 2 +-
OvmfPkg/CloudHv/CloudHvX64.dsc | 2 +-
OvmfPkg/IntelTdx/IntelTdxX64.dsc | 2 +-
OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 3 ++-
OvmfPkg/Microvm/MicrovmX64.dsc | 2 +-
OvmfPkg/OvmfPkgIa32.dsc | 2 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
OvmfPkg/OvmfPkgX64.dsc | 2 +-
OvmfPkg/OvmfXen.dsc | 2 +-
21 files changed, 83 insertions(+), 1077 deletions(-) delete mode
100644 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
delete mode 100644
MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifo.nasm
delete mode 100644
MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
delete mode 100644 MdePkg/Library/BaseIoLibIntrinsic/IoLibSev.h
delete mode 100644
MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifo.nasm
delete mode 100644
MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
create mode 100644 MdePkg/Library/TdxLib/TdxLibNull.inf
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110227): https://edk2.groups.io/g/devel/message/110227
Mute This Topic: https://groups.io/mt/102215661/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
