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 <ler...@redhat.com>
Sent: Saturday, October 28, 2023 7:40 PM
To: devel@edk2.groups.io; thomas.lenda...@amd.com; Tan, Dun
<dun....@intel.com>; leo.du...@amd.com; brijesh.si...@amd.com; Chang, Abner
<abner.ch...@amd.com>; michael.r...@amd.com; Attar, AbdulLateef (Abdul Lateef)
<abdullateef.at...@amd.com>
Cc: Ni, Ray <ray...@intel.com>; Yao, Jiewen <jiewen....@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 <ler...@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....@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 (#110393): https://edk2.groups.io/g/devel/message/110393
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]
-=-=-=-=-=-=-=-=-=-=-=-
