From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web08.241.1632754762128135508 for ; Mon, 27 Sep 2021 07:59:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FE8rucy2; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: kraxel@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1632754761; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=BogKx3H57GPHzEj3gUtDsaHAlEf7ysKYodCJpgcDhMI=; b=FE8rucy25tUBJERclR04BXbGJHsqDm4/KDQVo5mp2A6vlLp/5aZpu2o+xoAVZh9mhw+9/P S6H8Rcy/sD9iaeT+W1M91iqknKYOEYomUL8HdxVgFL4NiIirAAXDlG7On/+OWnYgYuCskM 9M42DBMVNuiZy/lQtL4YSuR8sCie1io= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-570-OqpSpAXPOICo3ktGD4e16A-1; Mon, 27 Sep 2021 10:59:18 -0400 X-MC-Unique: OqpSpAXPOICo3ktGD4e16A-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2CE901023F4D; Mon, 27 Sep 2021 14:59:17 +0000 (UTC) Received: from sirius.home.kraxel.org (unknown [10.39.193.134]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8734D60C13; Mon, 27 Sep 2021 14:59:16 +0000 (UTC) Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id C98EE180038E; Mon, 27 Sep 2021 16:59:14 +0200 (CEST) Date: Mon, 27 Sep 2021 16:59:14 +0200 From: "Gerd Hoffmann" To: "Yao, Jiewen" Cc: "devel@edk2.groups.io" , "Xu, Min M" , Brijesh Singh , Ard Biesheuvel , "Justen, Jordan L" , Erdem Aktas , James Bottomley , Tom Lendacky Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector Message-ID: <20210927145914.7r6ezdzno3zt2flq@sirius.home.kraxel.org> References: <20210924052825.2qljhtvweonbov5q@sirius.home.kraxel.org> <20210924100726.m33otbjod4fo3vur@sirius.home.kraxel.org> <20210924140221.6b3nk32eofwbwpgb@sirius.home.kraxel.org> <20210927080516.a64levdyyg6b4hne@sirius.home.kraxel.org> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=kraxel@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi, > > struct { > > uint64_t load_address; > > uint32_t file_offset; > > uint32_t section_size; > > uint32_t section_type; > > uint32_t section_flags; > > }; > > [Jiewen] This data structure does not work in a special use case - A > TD may want to have a fixed memory size. It is TD that tells the VMM > how many DRAM should be allocated by using metadata table. Not the > case that a VMM tells the TD how many DRAM is allocated by using HOB. > > In that special case, the TD_HOB is NOT required. The VMM parses the > metadata to allocate the DRAM (AUG page). Hmm. Not covered in tdx-virtual-firmware-design-guide-rev-1.pdf > The runtime section size must be UINT64, otherwise we cannot support > 4G memory section. > The build time section size can be UINT32. We don't expect to create a >4G binary in near future. > And we need both. That still doesn't explain why you need two sizes. Instead of depending on zero-fill in case MemoryDataSize > RawDataSize you can just use two entries, simliar to ELF binaries which have separate '.data' and '.bss' sections too. > I can understand why you think there is no needed fields, based upon > what you see in EDKII/TDVF project. However, the usage in current > TDVF is just a subset, but not all usages. So you are doing stuff behind closed doors ... > The TDX metadata structure is carefully designed to support those > variants. Also, leaving some room for future is a common practice. > Besides EDKII/TDVF, we are doing other TDX related projects reusing > the same metadata structure. (But sorry, I cannot tell more at this > time.) ... and don't want tell details. Even the fact that you are doing that is disclosed only after poking for a while because the patches submitted leave a bunch of questions open. This is NOT how Open Source Development works. If the patches can't speak for themself in cases like this the very minimum requirement is proper documentation. It is not acceptable that I have to ask five times to figure that the format is supposed to cover use cases beyond TDVF. > The benefit is that the KVM or cloud hypervisor can have a common > logic to handle "TDX boot", instead of using different table in > different use cases. The benefit of a unified table for tdx and sev is that the VMM can have common logic to find page ranges which need special initialization. But I suspect at that point we are going to trade code sharing at one place for code sharing at another place. > I think it is OK, if SEV wants to reuse the existing TDX metadata > table. (We need SEV people agree.) Then we can have one metadata > table. So, when submitting the next revision of this series, please ... (1) Move the tdx metadata changes to a separate patch. (2) Add *complete* documentation for the TDX metadata format ... so the SEV people can make up their mind whenever they want use that or not. Please do also clarify what the process to allocate section type numbers (or reserve a number range) for SEV would be. > [Jiewen] We don't fork OVMF in config-B. > > Instead of we will create new Tdvf/Tdvf.dsc and Tdvf/Tdvf.fdf in > config-B, similar to > https://github.com/tianocore/edk2/tree/master/OvmfPkg/AmdSev or > https://github.com/tianocore/edk2/tree/master/OvmfPkg/Bhyve That is > why I treat it as different platform. The differences between Ovmf and AmdSev are very small. Bhyve has more differences, but it's a different hypervisor so it isn't surprising it has its own PlatformPei. How does Tdvf handle the platform setup? It must be done in SEC somehow, so I suspect you have a (possibly stripped-down) version of the PlatformPei adapted for SEC? That is exactly the kind of code duplication I want avoid. > I reluctant to merge it back to Ovmf.dsc/fdf. I don't worry that much about Ovmf.dsc/fdf files. Whenever we add a compile-time option (-D ENABLE_TDX) to Ovmf.dsc/fdf or whenever we add a separate Tdvf.dsc/fdf doesn't make that much of a difference. I'm more worried about the code duplication and the completely different (PEI-less) initialization workflow. When touching the platform setup code both cases (with/without PEI) must be considered, which increases development and testing and maintenance effort long-term. I want less variants, not more. Ideally I'd like to also get rid of the OvmfPkgIa32X64.dsc/fdf for example. It seems some features have a dependency on PEI running in 32-bit mode though. > The reason is the main Ovmf supports some features (such as S3, TPM) > which may depend on PEI modules, but it is NOT needed in TDVF. So using PEI in OVMF isn't that over-engineered, isn't it? And I suspect SMM support can be added to the list of features which depend on the PEI phase (at least when we want reuse the existing common code in UefiCpuPkg, MdePkg and elsewhere). > We need re-evaluate the effort to enable those features in non-PEI > configuration in OVMF. - That is totally unnecessary in TDVF enabling > task. Well, it's surely additional upfront work. But I expect it will pay off long-term. Less maintenance work, less testing work, lower risk of adding regressions due to SEC and PEI init code paths variants running out of sync. So "totally unnecessary" only when you ignore the work needed after the initial merge. take care, Gerd