Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <98f81eed-e532-75bc-d2d8-4e020517b634@intel.com>
Date:   Thu, 28 Apr 2022 07:06:52 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v3 09/21] x86/virt/tdx: Get information about TDX module
 and convertible memory
Content-Language: en-US
To:     Kai Huang <kai.huang@intel.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     seanjc@google.com, pbonzini@redhat.com, len.brown@intel.com,
        tony.luck@intel.com, rafael.j.wysocki@intel.com,
        reinette.chatre@intel.com, dan.j.williams@intel.com,
        peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        isaku.yamahata@intel.com
References: <cover.1649219184.git.kai.huang@intel.com>
 <145620795852bf24ba2124a3f8234fd4aaac19d4.1649219184.git.kai.huang@intel.com>
 <f929fb7a-5bdc-2567-77aa-762a098c8513@intel.com>
 <0bab7221179229317a11311386c968bd0d40e344.camel@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <0bab7221179229317a11311386c968bd0d40e344.camel@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/27/22 17:15, Kai Huang wrote:
> On Wed, 2022-04-27 at 15:15 -0700, Dave Hansen wrote:
>> On 4/5/22 21:49, Kai Huang wrote:
>>> TDX provides increased levels of memory confidentiality and integrity.
>>> This requires special hardware support for features like memory
>>> encryption and storage of memory integrity checksums.  Not all memory
>>> satisfies these requirements.
>>>
>>> As a result, TDX introduced the concept of a "Convertible Memory Region"
>>> (CMR).  During boot, the firmware builds a list of all of the memory
>>> ranges which can provide the TDX security guarantees.  The list of these
>>> ranges, along with TDX module information, is available to the kernel by
>>> querying the TDX module via TDH.SYS.INFO SEAMCALL.
>>>
>>> Host kernel can choose whether or not to use all convertible memory
>>> regions as TDX memory.  Before TDX module is ready to create any TD
>>> guests, all TDX memory regions that host kernel intends to use must be
>>> configured to the TDX module, using specific data structures defined by
>>> TDX architecture.  Constructing those structures requires information of
>>> both TDX module and the Convertible Memory Regions.  Call TDH.SYS.INFO
>>> to get this information as preparation to construct those structures.
>>>
>>> Signed-off-by: Kai Huang <kai.huang@intel.com>
>>> ---
>>>  arch/x86/virt/vmx/tdx/tdx.c | 131 ++++++++++++++++++++++++++++++++++++
>>>  arch/x86/virt/vmx/tdx/tdx.h |  61 +++++++++++++++++
>>>  2 files changed, 192 insertions(+)
>>>
>>> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
>>> index ef2718423f0f..482e6d858181 100644
>>> --- a/arch/x86/virt/vmx/tdx/tdx.c
>>> +++ b/arch/x86/virt/vmx/tdx/tdx.c
>>> @@ -80,6 +80,11 @@ static DEFINE_MUTEX(tdx_module_lock);
>>>  
>>>  static struct p_seamldr_info p_seamldr_info;
>>>  
>>> +/* Base address of CMR array needs to be 512 bytes aligned. */
>>> +static struct cmr_info tdx_cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
>>> +static int tdx_cmr_num;
>>> +static struct tdsysinfo_struct tdx_sysinfo;
>>
>> I really dislike mixing hardware and software structures.  Please make
>> it clear which of these are fully software-defined and which are part of
>> the hardware ABI.
> 
> Both 'struct tdsysinfo_struct' and 'struct cmr_info' are hardware structures. 
> They are defined in tdx.h which has a comment saying the data structures below
> this comment is hardware structures:
> 
> 	+/*
> 	+ * TDX architectural data structures
> 	+ */
> 
> It is introduced in the P-SEAMLDR patch.
> 
> Should I explicitly add comments around the variables saying they are used by
> hardware, something like:
> 
> 	/*
> 	 * Data structures used by TDH.SYS.INFO SEAMCALL to return CMRs and
> 	 * TDX module system information.
> 	 */

I think we know they are data structures. :)

But, saying:

	/* Used in TDH.SYS.INFO SEAMCALL ABI: */

*is* actually helpful.  It (probably) tells us where in the spec we can
find the definition and tells how it gets used.  Plus, it tells us this
isn't a software data structure.

>>> +	/* Get TDX module information and CMRs */
>>> +	ret = tdx_get_sysinfo();
>>> +	if (ret)
>>> +		goto out;
>>
>> Couldn't we get rid of that comment if you did something like:
>>
>> 	ret = tdx_get_sysinfo(&tdx_cmr_array, &tdx_sysinfo);
> 
> Yes will do.
> 
>> and preferably make the variables function-local.
> 
> 'tdx_sysinfo' will be used by KVM too.

In other words, it's not a part of this series so I can't review whether
this statement is correct or whether there's a better way to hand this
information over to KVM.

This (minor) nugget influencing the design also isn't even commented or
addressed in the changelog.