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

["duntan" <dun.tan@intel.com>]

Hi Tom,
Thanks for your testing and comments. I'll modify the patch commit message and code according to your comments. 
Yes the patch changes the behavior for IoReadFifo/ IoWriteFifo API for non-Tdx and non-SEV case. Looking forward to more comments from the community.

Hi Laszlo,
May I know the actually performance gap data in the case that you mentioned? 
Also, if a DEBUG binary is used, I think the boot time impact might be acceptable?

Thanks,
Dun

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Saturday, October 28, 2023 7:40 PM
To: devel@edk2.groups.io; thomas.lendacky@amd.com; Tan, Dun <dun.tan@intel.com>; leo.duran@amd.com; brijesh.singh@amd.com; Chang, Abner <abner.chang@amd.com>; michael.roth@amd.com; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>
Cc: Ni, Ray <ray.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/7] Support Tdx and sev in BaseIoLibIntrinsic and remove BaseIoLibIntrinsicSev

On 10/27/23 23:31, Lendacky, Thomas via groups.io wrote:
> 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?

Thank you for making this comment, Tom, because this is exactly what I meant to raise immediately, upon reading the cover letter.

No, it is not acceptable.

The FIFO variants exist for a reason. When the guest performs multiple individual IO Port accesses, those translate to individual traps to the hypervisor, with significant performance impact. When IO Port string operations are used instead, with the REP prefix, then there is just
*one* trap, and the hypervisor can perform the whole "string" transfer in one go (up to a page size, anyways, IIRC). This has very visible impact on OVMF debug output (via the isa-debugcon QEMU device), and/or in case fw_cfg is used without DMA support.

(If you search OvmfPkg for IoReadFifo8 and IoWriteFifo8, you'll find the QemuFwCfgLib and PlatformDebugLibIoPort libraries using them.)

In fact, during initial SEV enablement, the SEV enlightenment was introduced because SEV does not handle the REP prefix with these instructions, and so a fallback had to be added.

See commits b6d11d7c4678 ("MdePkg: BaseIoLibIntrinsic (IoLib class) library", 2017-04-13) and 97353a9c914d ("OvmfPkg: Update dsc to use IoLib from BaseIoLibIntrinsicSev.inf", 2017-07-10).

Accordingly, there's a *huge* performance (boot time) impact when you boot OVMF in a SEV guest with DEBUG_VERBOSE messages enabled (and captured; i.e., when the isa-debugcon device is active).

> 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.

String IO must be preserved for such guests that don't run in Confidential Virtual Machines ("CVM"s).

In particular patches #6 and #7 would damage OVMF.

Nacked-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

> 
> 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 (#110393): https://edk2.groups.io/g/devel/message/110393
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]
-=-=-=-=-=-=-=-=-=-=-=-