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

["Laszlo Ersek" <lersek@redhat.com>]

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 (#110242): https://edk2.groups.io/g/devel/message/110242
Mute This Topic: https://groups.io/mt/102215661/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-