Message ID | 1699546489-4606-1-git-send-email-jpiotrowski@linux.microsoft.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b129:0:b0:403:3b70:6f57 with SMTP id q9csp544510vqs; Thu, 9 Nov 2023 08:15:54 -0800 (PST) X-Google-Smtp-Source: AGHT+IHfQQ5Wm5+WBR3PKOOoF0J7LaunwTtQ553AxmmtFYJYNbrAXnBRKlt1f1puI1AjpXXG0JhD X-Received: by 2002:a17:902:e752:b0:1c9:ff46:163d with SMTP id p18-20020a170902e75200b001c9ff46163dmr6011250plf.38.1699546554140; Thu, 09 Nov 2023 08:15:54 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1699546554; cv=none; d=google.com; s=arc-20160816; b=qkosGeK7NAv24UR/sx33K7AUznfzNfCHEpsKG8JgCjx44Mafm2j6j4EC8zGX3dqC5h 68WsQGKT4H3eH9Jy0La8XDgVBUFqXMZINLmXJyDPU/4z1AdxSMXnlzR7hFBmifX9Vb+7 P1l1IFeAGByVp1tUoIo1YegruksqkY8wAzH5LsGnL79unPEONA6k0OR8sBj6NCVuVtGv wf6f53fWXDAZVD2p+NnFIjTkswNB5tsqemVnY/AEjEcoT8vBTk5JqcMoQ8x2Cu+oo4Vs 6hm/TcebPUfvpApcKyMUz/iEF3DwV1n87JGQY4qUcMtkHJ8zNChfGhfzIguCUXfbVcuN Xq/g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:cc:to:from :dkim-signature:dkim-filter; bh=TODuuixcRLtErqip8qybtbJ+d9Czo7e1uLIBPHVy8TQ=; fh=xlY4TP4PFLTNnS8QU/XhHrQo7VHPn1XEmWNI9VWYw90=; b=AmFmn5a+FDlRoloDti180L0ssL9JIYjq4uwELsQ6FwVRKAsEZxKcVEEkN+XFfalrTd 1eOSfbK29/4TCgKKQ8FHQV+6wBGATPG5Q28NgpN5uyUfrLg4cREh5MrMoRRGImMvfTmm BuOefBWQgWafk70SVhmZ3WrB+IwXyO3ZPeSHtwI0fZR6f57G6ZeGAN83fJjBQ0QGm0mx WEhH+0qcgdo8vqEq4lFx/aerT8zKOZkbwCKdDV8VOqF8sgyE6/wEuj2WjHk76p/v7tTg obQwjfymIZuxLLU7P7n8LVPx0qOK9bh4arfzZvfIw9w7ROl6AuCFZRXWH+4nZ53BlrSc xsHg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=EbH0kTsK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com Received: from snail.vger.email (snail.vger.email. [23.128.96.37]) by mx.google.com with ESMTPS id kf16-20020a17090305d000b001c71e907ee6si5041273plb.124.2023.11.09.08.15.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Nov 2023 08:15:54 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=EbH0kTsK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 8B0A78376E61; Thu, 9 Nov 2023 08:15:22 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232511AbjKIQPL (ORCPT <rfc822;lhua1029@gmail.com> + 31 others); Thu, 9 Nov 2023 11:15:11 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32902 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234617AbjKIQPG (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 9 Nov 2023 11:15:06 -0500 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 632583588; Thu, 9 Nov 2023 08:15:03 -0800 (PST) Received: by linux.microsoft.com (Postfix, from userid 1112) id 909A820B74C0; Thu, 9 Nov 2023 08:15:02 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 909A820B74C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1699546502; bh=TODuuixcRLtErqip8qybtbJ+d9Czo7e1uLIBPHVy8TQ=; h=From:To:Cc:Subject:Date:From; b=EbH0kTsKFErwQ4gtiA3IOIQQBjpes92qMHSU1zZjXoJrLQs45sUfgAoQm8FsVgh31 xzpgR0xXT54AtG9TZrq49fL9GmkSzfFCEi4x0UxNxprEzSpmWgXxlb4WK2lY4ORMDW UH54N3QvpvKuLxTaFSyLI4FohPfV/TRUVoFUAXPQ= From: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com> To: Dave Hansen <dave.hansen@linux.intel.com>, Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org, Michael Kelley <mhklinux@outlook.com>, Dexuan Cui <decui@microsoft.com> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>, linux-hyperv@vger.kernel.org, stefan.bader@canonical.com, tim.gardner@canonical.com, roxana.nicolescu@canonical.com, cascardo@canonical.com, kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org, kirill.shutemov@linux.intel.com, sashal@kernel.org Subject: [PATCH] x86/mm: Check cc_vendor when printing memory encryption info Date: Thu, 9 Nov 2023 08:14:49 -0800 Message-Id: <1699546489-4606-1-git-send-email-jpiotrowski@linux.microsoft.com> X-Mailer: git-send-email 1.8.3.1 Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Thu, 09 Nov 2023 08:15:22 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782103727278726886 X-GMAIL-MSGID: 1782103727278726886 |
Series |
x86/mm: Check cc_vendor when printing memory encryption info
|
|
Commit Message
Jeremi Piotrowski
Nov. 9, 2023, 4:14 p.m. UTC
Check the value of cc_vendor to see if we're in an Intel TDX protected VM
instead of checking for the TDX_GUEST CPU feature. The rest of the function
already uses the abstractions available in cc_platform.h to check for
confidential computing features. For Intel, cc_vendor is set from
tdx_early_init() or hv_vtom_init(), so the new code correctly handles both
cases. The previous check relied on the Linux-controlled TDX_GUEST CPU feature
which is only set in tdx_early_init().
Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
---
arch/x86/mm/mem_encrypt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 11/9/23 08:14, Jeremi Piotrowski wrote: ... > pr_info("Memory Encryption Features active:"); > > - if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { > + if (cc_vendor == CC_VENDOR_INTEL) { > pr_cont(" Intel TDX\n"); > return; > } Why aren't these guests setting X86_FEATURE_TDX_GUEST?
On 09/11/2023 17:25, Dave Hansen wrote: > On 11/9/23 08:14, Jeremi Piotrowski wrote: > ... >> pr_info("Memory Encryption Features active:"); >> >> - if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { >> + if (cc_vendor == CC_VENDOR_INTEL) { >> pr_cont(" Intel TDX\n"); >> return; >> } > > Why aren't these guests setting X86_FEATURE_TDX_GUEST? They could if we can confirm that the code gated behind cpu_feature_enabled(X86_FEATURE_TDX_GUEST) is correct when running with TD partitioning. It still makes sense to have these prints based on the cc_xxx abstractions. Jeremi
On 11/9/23 08:35, Jeremi Piotrowski wrote: > On 09/11/2023 17:25, Dave Hansen wrote: >> On 11/9/23 08:14, Jeremi Piotrowski wrote: >> ... >>> pr_info("Memory Encryption Features active:"); >>> >>> - if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { >>> + if (cc_vendor == CC_VENDOR_INTEL) { >>> pr_cont(" Intel TDX\n"); >>> return; >>> } >> >> Why aren't these guests setting X86_FEATURE_TDX_GUEST? > > They could if we can confirm that the code gated behind > cpu_feature_enabled(X86_FEATURE_TDX_GUEST) is correct when running with TD partitioning. Let me elaborate a bit on my question. X86_FEATURE_TDX_GUEST is set based on some specific gunk that shows up in CPUID in TDX guests. I *believe* it's in one of the CPUID leaves that the VMM has no control over. So, first, what in practice is keeping tdx_early_init() from running on these "TD partitioning" systems? Because it's either not running, or I'm misreading the code horribly. > It still makes sense to have these prints based on the cc_xxx abstractions. I'm not sure I'm following. For instance, let's say that someone came up with a nutty reason to do MKTME in Linux. We'd need a host-side contraption that sets CC_ATTR_MEM_ENCRYPT and so forth. It would also, obviously, set cc_vendor=CC_VENDOR_INTEL just like host-side SME sets cc_vendor=CC_VENDOR_AMD. Then we'd have to go back and disentangle all the places where we assumed CC_VENDOR_INTEL==TDX. That, combined with this patch's apparent disregard for why X86_FEATURE_TDX_GUEST isn't getting set makes me think that the big picture was not considered here and this patch represents the quickest hack to get the right spew out to dmesg that is desired.
On 09/11/2023 17:50, Dave Hansen wrote: > On 11/9/23 08:35, Jeremi Piotrowski wrote: >> On 09/11/2023 17:25, Dave Hansen wrote: >>> On 11/9/23 08:14, Jeremi Piotrowski wrote: >>> ... >>>> pr_info("Memory Encryption Features active:"); >>>> >>>> - if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { >>>> + if (cc_vendor == CC_VENDOR_INTEL) { >>>> pr_cont(" Intel TDX\n"); >>>> return; >>>> } >>> >>> Why aren't these guests setting X86_FEATURE_TDX_GUEST? >> >> They could if we can confirm that the code gated behind >> cpu_feature_enabled(X86_FEATURE_TDX_GUEST) is correct when running with TD partitioning. > > Let me elaborate a bit on my question. > > X86_FEATURE_TDX_GUEST is set based on some specific gunk that shows up > in CPUID in TDX guests. I *believe* it's in one of the CPUID leaves > that the VMM has no control over.> Right - you're talking about TDX_CPUID_LEAF_ID 0x21. > So, first, what in practice is keeping tdx_early_init() from running on > these "TD partitioning" systems? Because it's either not running, or > I'm misreading the code horribly. > You're right, it is not running in this case. Not super familiar with TD partitioning, but as I understand it in TD partitioning Linux runs as a kind of L2 TD guest with the paravisor acting as the L1 TD VM(M). tdx_early_init() changes kernel behavior with the assumption that it can talk directly to the TD module or change page visibility in a certain way, instead of talking to a paravisor. So that CPUID is hidden to prevent this. Otherwise tdx_early_init() would need to be modified to check "am I running with TD partitioning and if so - switch to other implementations". >> It still makes sense to have these prints based on the cc_xxx abstractions. > > I'm not sure I'm following. > > For instance, let's say that someone came up with a nutty reason to do > MKTME in Linux. We'd need a host-side contraption that sets > CC_ATTR_MEM_ENCRYPT and so forth. It would also, obviously, set > cc_vendor=CC_VENDOR_INTEL just like host-side SME sets > cc_vendor=CC_VENDOR_AMD. > > Then we'd have to go back and disentangle all the places where we > assumed CC_VENDOR_INTEL==TDX. We're not baking that generalization into any other place, but when printing coco features here it made sense to me to make the shortcut. I can also post a v2 that is more accurate: if (cc_vendor == CC_VENDOR_INTEL) { pr_cont(" Intel"); if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) pr_cont(" TDX\n"); return; } Dexuan and Michael would like to see callbacks added that allow kernel code to more accurately report coco features when a paravisor is involved. > > That, combined with this patch's apparent disregard for why > X86_FEATURE_TDX_GUEST isn't getting set makes me think that the big > picture was not considered here and this patch represents the quickest > hack to get the right spew out to dmesg that is desired. It's not disregard, the way the kernel behaves in this case is correct except for the error in reporting CPU vendor. Users care about seeing the correct information in dmesg. Setting X86_FEATURE_TDX_GUEST might be interesting for a different reason: so that we also see TDX in /proc/cpuinfo. But then we would want to consider whether we should add a feature for SNP_GUEST, or else every platform reports things differently. The cc_platform stuff in the kernel was introduced to avoid this kind of differentiation. Jeremi
On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote: > It's not disregard, the way the kernel behaves in this case is correct except > for the error in reporting CPU vendor. Users care about seeing the correct > information in dmesg. I think it is wrong place to advertise CoCo features. It better fits into Intel/AMD-specific code that knows better what it is running on. Inferring it from generic code has been proved problematic as you showed. Maybe just remove incorrect info and that's it? diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c index c290c55b632b..f573a97c0524 100644 --- a/arch/x86/mm/mem_encrypt.c +++ b/arch/x86/mm/mem_encrypt.c @@ -40,42 +40,6 @@ bool force_dma_unencrypted(struct device *dev) return false; } -static void print_mem_encrypt_feature_info(void) -{ - pr_info("Memory Encryption Features active:"); - - if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { - pr_cont(" Intel TDX\n"); - return; - } - - pr_cont(" AMD"); - - /* Secure Memory Encryption */ - if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) { - /* - * SME is mutually exclusive with any of the SEV - * features below. - */ - pr_cont(" SME\n"); - return; - } - - /* Secure Encrypted Virtualization */ - if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) - pr_cont(" SEV"); - - /* Encrypted Register State */ - if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) - pr_cont(" SEV-ES"); - - /* Secure Nested Paging */ - if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) - pr_cont(" SEV-SNP"); - - pr_cont("\n"); -} - /* Architecture __weak replacement functions */ void __init mem_encrypt_init(void) { @@ -85,7 +49,7 @@ void __init mem_encrypt_init(void) /* Call into SWIOTLB to update the SWIOTLB DMA buffers */ swiotlb_update_mem_attributes(); - print_mem_encrypt_feature_info(); + pr_info("Memory Encryption is active\n"); } void __init mem_encrypt_setup_arch(void)
On 10/11/2023 13:06, kirill.shutemov@linux.intel.com wrote: > On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote: >> It's not disregard, the way the kernel behaves in this case is correct except >> for the error in reporting CPU vendor. Users care about seeing the correct >> information in dmesg. > > I think it is wrong place to advertise CoCo features. It better fits into > Intel/AMD-specific code that knows better what it is running on. Inferring > it from generic code has been proved problematic as you showed. > Let's not make this into a bigger issue than it is. There is a check that does not cover all cases, and needs to be changed into a different check that covers all cases. I'd wouldn't call that problematic. I would be open to moving the function to arch/x86/coco/core.c or providing a mechanism for cpu/paravisor-specific code to do the reporting (which is what Dexuan originally proposed [1]). [1]: https://lore.kernel.org/lkml/20231019062030.3206-1-decui@microsoft.com/ > Maybe just remove incorrect info and that's it? > I disagree, other users and I find the print very useful to see which coco platform the kernel is running on and which confidential computing features the kernel detected. I'm willing to fix the code to report correct info. > diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c > index c290c55b632b..f573a97c0524 100644 > --- a/arch/x86/mm/mem_encrypt.c > +++ b/arch/x86/mm/mem_encrypt.c > @@ -40,42 +40,6 @@ bool force_dma_unencrypted(struct device *dev) > return false; > } > > -static void print_mem_encrypt_feature_info(void) > -{ > - pr_info("Memory Encryption Features active:"); > - > - if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { > - pr_cont(" Intel TDX\n"); > - return; > - } > - > - pr_cont(" AMD"); > - > - /* Secure Memory Encryption */ > - if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) { > - /* > - * SME is mutually exclusive with any of the SEV > - * features below. > - */ > - pr_cont(" SME\n"); > - return; > - } > - > - /* Secure Encrypted Virtualization */ > - if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) > - pr_cont(" SEV"); > - > - /* Encrypted Register State */ > - if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) > - pr_cont(" SEV-ES"); > - > - /* Secure Nested Paging */ > - if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) > - pr_cont(" SEV-SNP"); > - > - pr_cont("\n"); > -} > - > /* Architecture __weak replacement functions */ > void __init mem_encrypt_init(void) > { > @@ -85,7 +49,7 @@ void __init mem_encrypt_init(void) > /* Call into SWIOTLB to update the SWIOTLB DMA buffers */ > swiotlb_update_mem_attributes(); > > - print_mem_encrypt_feature_info(); > + pr_info("Memory Encryption is active\n"); > } > > void __init mem_encrypt_setup_arch(void)
On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote: > > Maybe just remove incorrect info and that's it? > > > > I disagree, other users and I find the print very useful to see which coco > platform the kernel is running on and which confidential computing features > the kernel detected. I'm willing to fix the code to report correct info. For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init() can do something similar.
On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote: > tdx_early_init() changes kernel behavior with the assumption that it > can talk directly to the TD module or change page visibility in > a certain way, instead of talking to a paravisor. So that CPUID is > hidden to prevent this. Otherwise tdx_early_init() would need to be > modified to check "am I running with TD partitioning and if so > - switch to other implementations". Here we go with the virt zoo again. If you hide TDX_CPUID_LEAF_ID from it, then it of course doesn't know that it is a TDX guest. This is the same thing as the SNP vTom thing: the only viable way going forward is for the guest kernel to detect correctly what it runs on and act accordingly. You can't just do some semi-correct tests for vendor - correct only if you squint hard enough - and hope that it works because it'll break apart eventually, when that second-level TDX fun needs to add more hackery to the guest kernel. So, instead, think about how the paravisor tells the guest it is running on one - a special CPUID leaf or an MSR in the AMD case - and use that to detect it properly. Everything else is a mess waiting to happen. Thx.
On 10/11/2023 13:46, kirill.shutemov@linux.intel.com wrote: > On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote: >>> Maybe just remove incorrect info and that's it? >>> >> >> I disagree, other users and I find the print very useful to see which coco >> platform the kernel is running on and which confidential computing features >> the kernel detected. I'm willing to fix the code to report correct info. > > For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init() > can do something similar. > That doesn't cover TDX guests with TD partitioning on Hyper-V.
On 10/11/2023 14:17, Borislav Petkov wrote: > On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote: >> tdx_early_init() changes kernel behavior with the assumption that it >> can talk directly to the TD module or change page visibility in >> a certain way, instead of talking to a paravisor. So that CPUID is >> hidden to prevent this. Otherwise tdx_early_init() would need to be >> modified to check "am I running with TD partitioning and if so >> - switch to other implementations". > > Here we go with the virt zoo again. If you hide TDX_CPUID_LEAF_ID from > it, then it of course doesn't know that it is a TDX guest. This is the > same thing as the SNP vTom thing: the only viable way going forward is > for the guest kernel to detect correctly what it runs on and act > accordingly. That part already works correctly. The kernel knows very well that it is a TDX guest because TD partitioning (same as SNP vTOM) support uses the standard coco mechanisms to indicate that to the kernel. The kernel is well aware of how to operate in this case: use bounce buffers, flip page visibility by calling the correct hoosk, etc. Same flow as for every other flavor of confidential guest. > > You can't just do some semi-correct tests for vendor - correct only > if you squint hard enough - and hope that it works because it'll break > apart eventually, when that second-level TDX fun needs to add more > hackery to the guest kernel. > What's semi-correct about checking for CC_VENDOR_INTEL and then printing Intel? I can post a v2 that checks CC_ATTR_GUEST_MEM_ENCRYPT before printing "TDX". Feature printing needs to evolve as new technologies come along. > So, instead, think about how the paravisor tells the guest it is running > on one - a special CPUID leaf or an MSR in the AMD case - and use that> to detect it properly. The paravisor *is* telling the guest it is running on one - using a CPUID leaf (HYPERV_CPUID_ISOLATION_CONFIG). A paravisor is a hypervisor for a confidential guest, that's why paravisor detection shares logic with hypervisor detection. tdx_early_init() runs extremely early, way before hypervisor(/paravisor) detection. If the TDX_CPUID_LEAF_ID leaf were present it would require duplicating hypervisor/paravisor logic in that function (and in sme_early_init()). As soon as we'd detect the paravisor we'd need to avoid performing tdx_module_calls() (because they're not allowed) so the function would return without doing anything useful: void __init tdx_early_init(void) { u64 cc_mask; u32 eax, sig[3]; cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2], &sig[1]); if (memcmp(TDX_IDENT, sig, sizeof(sig))) return; setup_force_cpu_cap(X86_FEATURE_TDX_GUEST); cc_vendor = CC_VENDOR_INTEL; /* Can't perform tdx_module_calls when a paravisor is present */ if (early_detect_paravisor()) goto exit; .... exit: pr_info("Guest detected\n"); } Additionally we'd need to sprinkle paravisor checks along with existing X86_FEATURE_TDX_GUEST checks. And any time someone adds a new feature that depends solely on X86_FEATURE_TDX_GUEST we'd run the chance of it breaking things. That would be a mess. Jeremi > > Everything else is a mess waiting to happen. > > Thx. >
On Fri, Nov 10, 2023 at 04:51:43PM +0100, Jeremi Piotrowski wrote: > What's semi-correct about checking for CC_VENDOR_INTEL and then > printing Intel? I can post a v2 that checks CC_ATTR_GUEST_MEM_ENCRYPT > before printing "TDX". How is it that you're not seeing the conflict: Your TD partitioning guest *is* a TDX guest so X86_FEATURE_TDX_GUEST should be set there. But it isn't. Which means, that is already wrong. Or insufficient. if (cc_vendor == CC_VENDOR_INTEL) just *happens* to work for your case. What the detection code should do, rather, is: if (guest type == TD partioning) set bla; else if (TDX_CPUID_LEAF_ID) "normal" TDX guest; and those rules need to be spelled out so that everyone is on the same page as to how a TD partitioning guest is detected, how a normal TDX guest is detected, a SEV-ES, a SNP one, yadda yadda. > The paravisor *is* telling the guest it is running on one - using a CPUID leaf > (HYPERV_CPUID_ISOLATION_CONFIG). A paravisor is a hypervisor for a confidential > guest, that's why paravisor detection shares logic with hypervisor detection. > > tdx_early_init() runs extremely early, way before hypervisor(/paravisor) detection. What? Why can't tdx_early_init() run CPUID(HYPERV_CPUID_ISOLATION_CONFIG) if it can't find a valid TDX_CPUID_LEAF_ID and set X86_FEATURE_TDX_GUEST then? > Additionally we'd need to sprinkle paravisor checks along with > existing X86_FEATURE_TDX_GUEST checks. And any time someone adds a new > feature that depends solely on X86_FEATURE_TDX_GUEST we'd run the > chance of it breaking things. Well, before anything, you'd need to define what exactly the guest kernel needs to do when running as a TD partitioning guest and how exactly that is going to be detected and checked using the current cc_* and cpufeatures infra. If it doesn't work with the current scheme, then the current scheme should be extended. Then, that should be properly written out: "if bit X is set, then that is a guest type Y" "if feature foo present, then so and so are given" If the current guest type detection is insufficient, then that should be extended/amended. That's the only viable way where the kernel would support properly and reliably a given guest type. There'll be no sprinkling of checks anywhere. Thx.
On Fri, Nov 10, 2023 at 02:42:31PM +0100, Jeremi Piotrowski wrote: > On 10/11/2023 13:46, kirill.shutemov@linux.intel.com wrote: > > On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote: > >>> Maybe just remove incorrect info and that's it? > >>> > >> > >> I disagree, other users and I find the print very useful to see which coco > >> platform the kernel is running on and which confidential computing features > >> the kernel detected. I'm willing to fix the code to report correct info. > > > > For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init() > > can do something similar. > > > > That doesn't cover TDX guests with TD partitioning on Hyper-V. I am sure Hyper-V can report what mode it runs.
On 10/11/2023 17:45, Borislav Petkov wrote: > On Fri, Nov 10, 2023 at 04:51:43PM +0100, Jeremi Piotrowski wrote: >> What's semi-correct about checking for CC_VENDOR_INTEL and then >> printing Intel? I can post a v2 that checks CC_ATTR_GUEST_MEM_ENCRYPT >> before printing "TDX". > > How is it that you're not seeing the conflict: > > Your TD partitioning guest *is* a TDX guest so X86_FEATURE_TDX_GUEST > should be set there. But it isn't. Which means, that is already wrong. > Or insufficient. > > if (cc_vendor == CC_VENDOR_INTEL) > > just *happens* to work for your case. > > What the detection code should do, rather, is: > > if (guest type == TD partioning) > set bla; > else if (TDX_CPUID_LEAF_ID) > "normal" TDX guest; > > and those rules need to be spelled out so that everyone is on the same > page as to how a TD partitioning guest is detected, how a normal TDX > guest is detected, a SEV-ES, a SNP one, yadda yadda. > >> The paravisor *is* telling the guest it is running on one - using a CPUID leaf >> (HYPERV_CPUID_ISOLATION_CONFIG). A paravisor is a hypervisor for a confidential >> guest, that's why paravisor detection shares logic with hypervisor detection. >> >> tdx_early_init() runs extremely early, way before hypervisor(/paravisor) detection. > > What? > > Why can't tdx_early_init() run CPUID(HYPERV_CPUID_ISOLATION_CONFIG) if > it can't find a valid TDX_CPUID_LEAF_ID and set X86_FEATURE_TDX_GUEST > then? I guess it can if no one has an issue with it. Thank you for the review, I've posted a patchset that implements this idea here: https://lore.kernel.org/lkml/20231122170106.270266-1-jpiotrowski@linux.microsoft.com/T/#u > >> Additionally we'd need to sprinkle paravisor checks along with >> existing X86_FEATURE_TDX_GUEST checks. And any time someone adds a new >> feature that depends solely on X86_FEATURE_TDX_GUEST we'd run the >> chance of it breaking things. > > Well, before anything, you'd need to define what exactly the guest kernel > needs to do when running as a TD partitioning guest and how exactly that > is going to be detected and checked using the current cc_* and > cpufeatures infra. If it doesn't work with the current scheme, then the > current scheme should be extended. > > Then, that should be properly written out: > > "if bit X is set, then that is a guest type Y" > "if feature foo present, then so and so are given" > > If the current guest type detection is insufficient, then that should be > extended/amended. > > That's the only viable way where the kernel would support properly and > reliably a given guest type. There'll be no sprinkling of checks > anywhere. > > Thx. > OK. In the new submission I've added CC_ATTR_TDX_MODULE_CALLS because that is what we really need to guard against, and these guests can then have X86_FEATURE_TDX_GUEST set normally. Jeremi
On 10/11/2023 19:57, kirill.shutemov@linux.intel.com wrote: > On Fri, Nov 10, 2023 at 02:42:31PM +0100, Jeremi Piotrowski wrote: >> On 10/11/2023 13:46, kirill.shutemov@linux.intel.com wrote: >>> On Fri, Nov 10, 2023 at 01:27:08PM +0100, Jeremi Piotrowski wrote: >>>>> Maybe just remove incorrect info and that's it? >>>>> >>>> >>>> I disagree, other users and I find the print very useful to see which coco >>>> platform the kernel is running on and which confidential computing features >>>> the kernel detected. I'm willing to fix the code to report correct info. >>> >>> For TDX, we already have "tdx: Guest detected" in dmesg. sme_early_init() >>> can do something similar. >>> >> >> That doesn't cover TDX guests with TD partitioning on Hyper-V. > > I am sure Hyper-V can report what mode it runs. > You're right, it could. In that case I would move the AMD print and keep only "Memory Encryption is active" here. Lets see whether we find agreement on my new submission first. Thanks, Jeremi
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c index c290c55b632b..d3bd39aad8b6 100644 --- a/arch/x86/mm/mem_encrypt.c +++ b/arch/x86/mm/mem_encrypt.c @@ -44,7 +44,7 @@ static void print_mem_encrypt_feature_info(void) { pr_info("Memory Encryption Features active:"); - if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) { + if (cc_vendor == CC_VENDOR_INTEL) { pr_cont(" Intel TDX\n"); return; }