From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id B5FED7803D1 for ; Sat, 28 Oct 2023 11:40:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ieQ04ZXXr/zcqWxLbIH1j4f3CJCqQMI3J+NS4uqe7WU=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698493213; v=1; b=aHetmdprqQrAVHiQJTPKEkHZEWwSQxut9jWodRQYRUe4+44OCDfTfUTur416jqwOJaJSkCE7 b7AUawogFA5q3jEyQoQZwPgAVp1fIBiEkqVOi3ooRbvNNcwJHPFCxUK9lKiGtXq5x0ZzQ5r38Pm VwtmxcrBsnLs3Yoz/AUWnVEk= X-Received: by 127.0.0.2 with SMTP id laycYY7687511xCC9JEIeDQV; Sat, 28 Oct 2023 04:40:13 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.16383.1698493212629428702 for ; Sat, 28 Oct 2023 04:40:12 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-547-AZW1wa_mMmm0V72dAJ3ejg-1; Sat, 28 Oct 2023 07:40:04 -0400 X-MC-Unique: AZW1wa_mMmm0V72dAJ3ejg-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1C7B2185A787; Sat, 28 Oct 2023 11:40:04 +0000 (UTC) X-Received: from [10.39.192.70] (unknown [10.39.192.70]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5E7392166B26; Sat, 28 Oct 2023 11:40:02 +0000 (UTC) Message-ID: <3692a402-fede-9534-0517-72298ff2aff8@redhat.com> Date: Sat, 28 Oct 2023 13:40:01 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 0/7] Support Tdx and sev in BaseIoLibIntrinsic and remove BaseIoLibIntrinsicSev To: devel@edk2.groups.io, thomas.lendacky@amd.com, "Tan, Dun" , "leo.duran@amd.com" , "brijesh.singh@amd.com" , "Chang, Abner" , "michael.roth@amd.com" , "Attar, AbdulLateef (Abdul Lateef)" Cc: "Ni, Ray" , "Yao, Jiewen" References: <20231027054300.1382-1-dun.tan@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Dk32SrvSNKQuJpkx8aU1h7Glx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=aHetmdpr; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 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 ; 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 >> Sent: Friday, October 27, 2023 3:07 PM >> To: Tan, Dun ; 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 >>> Sent: Friday, October 27, 2023 2:32 PM >>> To: Yao, Jiewen ; 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 >>> Sent: Friday, October 27, 2023 1:49 PM >>> To: devel@edk2.groups.io; Tan, Dun >>> 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 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] -=-=-=-=-=-=-=-=-=-=-=-