Re: [edk2-devel] [PATCH 0/7] Support Tdx and sev in BaseIoLibIntrinsic and remove BaseIoLibIntrinsicSev

["Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io>]

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.yao@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.yao@intel.com>
> Sent: Friday, October 27, 2023 3:07 PM
> To: Tan, Dun <dun.tan@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.tan@intel.com>
>> Sent: Friday, October 27, 2023 2:32 PM
>> To: Yao, Jiewen <jiewen.yao@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.yao@intel.com>
>> Sent: Friday, October 27, 2023 1:49 PM
>> To: devel@edk2.groups.io; Tan, Dun <dun.tan@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-