From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37818C4332F for ; Tue, 22 Nov 2022 16:50:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234024AbiKVQun (ORCPT ); Tue, 22 Nov 2022 11:50:43 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50648 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232355AbiKVQuk (ORCPT ); Tue, 22 Nov 2022 11:50:40 -0500 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 48BE76C729; Tue, 22 Nov 2022 08:50:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1669135839; x=1700671839; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=puqtoUPvntcrMHUyIfxv2XTasYKx66NRxAdTJ/YKsJo=; b=B0RO81aP0AKXo6JdOrrYUo70nZJTQ/JsyOuUN3q11s3vvUvevukKQLvs bWWB57NQKaAb1M8lLhNQtyHBLs9FsuFmoft0fRqXgfjZhXGmXAzgH9Cy9 SQcHvR9XR//yIl3KMmFPdTLjz1LEw98GEc7zg2Krg2wYOG/AxvGdsDPmq eggR792mCyzNYo5QPQxDAjkDrY45N+6/F7zNRB/geK0ltUAQqki3BdZFV rX14B66tOI0F0rtQNxTFCTYGRrtZjR2aKD1VfbQ8rHuvI4b7oCu5/Mp0/ m9u86vEg17sBHrZ9BevQyISd9dnNeKCLQppwdSDK2IKcJNfmkTX56qahA g==; X-IronPort-AV: E=McAfee;i="6500,9779,10539"; a="315680098" X-IronPort-AV: E=Sophos;i="5.96,184,1665471600"; d="scan'208";a="315680098" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Nov 2022 08:50:25 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10539"; a="592201627" X-IronPort-AV: E=Sophos;i="5.96,184,1665471600"; d="scan'208";a="592201627" Received: from lcano-mobl1.amr.corp.intel.com (HELO [10.255.231.75]) ([10.255.231.75]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Nov 2022 08:50:23 -0800 Message-ID: Date: Tue, 22 Nov 2022 08:50:23 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH v7 02/20] x86/virt/tdx: Detect TDX during kernel boot Content-Language: en-US To: "Huang, Kai" , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" Cc: "Luck, Tony" , "bagasdotme@gmail.com" , "ak@linux.intel.com" , "Wysocki, Rafael J" , "kirill.shutemov@linux.intel.com" , "Christopherson,, Sean" , "Chatre, Reinette" , "pbonzini@redhat.com" , "linux-mm@kvack.org" , "Yamahata, Isaku" , "peterz@infradead.org" , "Shahar, Sagi" , "imammedo@redhat.com" , "Gao, Chao" , "Brown, Len" , "sathyanarayanan.kuppuswamy@linux.intel.com" , "Huang, Ying" , "Williams, Dan J" References: <9db9599fba11490cebbe59cbb7c145e9c119ab0f.camel@intel.com> From: Dave Hansen In-Reply-To: <9db9599fba11490cebbe59cbb7c145e9c119ab0f.camel@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/22/22 03:28, Huang, Kai wrote: >>> + /* >>> + * KeyID 0 is for TME. MKTME KeyIDs start from 1. TDX private >>> + * KeyIDs start after the last MKTME KeyID. >>> + */ >> >> Is the TME key a "MKTME KeyID"? > > I don't think so. Hardware handles TME KeyID 0 differently from non-0 MKTME > KeyIDs. And PCONFIG only accept non-0 KeyIDs. Let's say we have 4 MKTME hardware bits, we'd have: 0: TME Key 1->3: MKTME Keys 4->7: TDX Private Keys First, the MSR values: > + * IA32_MKTME_KEYID_PARTIONING: > + * Bit [31:0]: Number of MKTME KeyIDs. > + * Bit [63:32]: Number of TDX private KeyIDs. These would be: Bit [ 31:0] = 3 Bit [63:22] = 4 And in the end the variables: tdx_keyid_start would be 4 and tdx_keyid_num would be 4. Right? That's a bit wonky for my brain because I guess I know too much about the internal implementation and how the key space is split up. I guess I (wrongly) expected Bit[31:0]==Bit[63:22]. >>> +static void __init clear_tdx(void) >>> +{ >>> + tdx_keyid_start = tdx_keyid_num = 0; >>> +} >> >> This is where a comment is needed and can actually help. >> >> /* >> * tdx_keyid_start/num indicate that TDX is uninitialized. This >> * is used in TDX initialization error paths to take it from >> * initialized -> uninitialized. >> */ > > Just want to point out after removing the !x2apic_enabled() check, the only > thing need to do here is to detect/record the TDX KeyIDs. > > And the purpose of this TDX boot-time initialization code is to provide > platform_tdx_enabled() function so that kexec() can use. > > To distinguish boot-time TDX initialization from runtime TDX module > initialization, how about change the comment to below? > > static void __init clear_tdx(void) > { > /* > * tdx_keyid_start and nr_tdx_keyids indicate that TDX is not > * enabled by the BIOS. This is used in TDX boot-time > * initializatiton error paths to take it from enabled to not > * enabled. > */ > tdx_keyid_start = nr_tdx_keyids = 0; > } > > [...] I honestly have no idea what "boot-time TDX initialization" is versus "runtime TDX module initialization". This doesn't hel. > And below is the updated patch. How does it look to you? Let's see... ... > +static u32 tdx_keyid_start __ro_after_init; > +static u32 nr_tdx_keyids __ro_after_init; > + > +static int __init record_keyid_partitioning(void) > +{ > + u32 nr_mktme_keyids; > + int ret; > + > + /* > + * IA32_MKTME_KEYID_PARTIONING: > + * Bit [31:0]: Number of MKTME KeyIDs. > + * Bit [63:32]: Number of TDX private KeyIDs. > + */ > + ret = rdmsr_safe(MSR_IA32_MKTME_KEYID_PARTITIONING, &nr_mktme_keyids, > + &nr_tdx_keyids); > + if (ret) > + return -ENODEV; > + > + if (!nr_tdx_keyids) > + return -ENODEV; > + > + /* TDX KeyIDs start after the last MKTME KeyID. */ > + tdx_keyid_start++; tdx_keyid_start is uniniitalized here. So, it'd be 0, then ++'d. Kai, please take a moment and slow down. This isn't a race. I offered some replacement code here, which you've discarded, missed or ignored and in the process broken this code. This approach just wastes reviewer time. It's not working for me. I'm going to make a suggestion (aka. a demand): You can post these patches at most once a week. You get a whole week to (carefully) incorporate reviewer feedback, make the patch better, and post a new version. Need more time? Go ahead and take it. Take as much time as you want.