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: <d69c08da-80fa-2001-bbe8-8c45552e74ae@intel.com>
Date:   Wed, 27 Apr 2022 15:24:12 -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 10/21] x86/virt/tdx: Add placeholder to coveret all
 system RAM as TDX 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>
 <6230ef28be8c360ab326c8f592acf1964ac065c1.1649219184.git.kai.huang@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <6230ef28be8c360ab326c8f592acf1964ac065c1.1649219184.git.kai.huang@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.
> 
> In order to provide crypto protection to TD guests, the TDX architecture

There's that "crypto protection" thing again.  I'm not really a fan of
the changes made to this changelog since I wrote it. :)

> also needs additional metadata to record things like which TD guest
> "owns" a given page of memory.  This metadata essentially serves as the
> 'struct page' for the TDX module.  The space for this metadata is not
> reserved by the hardware upfront and must be allocated by the kernel

			    ^ "up front"

...
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 482e6d858181..ec27350d53c1 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -13,6 +13,7 @@
>  #include <linux/cpu.h>
>  #include <linux/smp.h>
>  #include <linux/atomic.h>
> +#include <linux/slab.h>
>  #include <asm/msr-index.h>
>  #include <asm/msr.h>
>  #include <asm/cpufeature.h>
> @@ -594,8 +595,29 @@ static int tdx_get_sysinfo(void)
>  	return sanitize_cmrs(tdx_cmr_array, cmr_num);
>  }
>  
> +static void free_tdmrs(struct tdmr_info **tdmr_array, int tdmr_num)
> +{
> +	int i;
> +
> +	for (i = 0; i < tdmr_num; i++) {
> +		struct tdmr_info *tdmr = tdmr_array[i];
> +
> +		/* kfree() works with NULL */
> +		kfree(tdmr);
> +		tdmr_array[i] = NULL;
> +	}
> +}
> +
> +static int construct_tdmrs(struct tdmr_info **tdmr_array, int *tdmr_num)
> +{
> +	/* Return -EFAULT until constructing TDMRs is done */
> +	return -EFAULT;
> +}
> +
>  static int init_tdx_module(void)
>  {
> +	struct tdmr_info **tdmr_array;
> +	int tdmr_num;
>  	int ret;
>  
>  	/* TDX module global initialization */
> @@ -613,11 +635,36 @@ static int init_tdx_module(void)
>  	if (ret)
>  		goto out;
>  
> +	/*
> +	 * Prepare enough space to hold pointers of TDMRs (TDMR_INFO).
> +	 * TDX requires TDMR_INFO being 512 aligned.  Each TDMR is

					 ^ "512-byte aligned"

Right?

> +	 * allocated individually within construct_tdmrs() to meet
> +	 * this requirement.
> +	 */
> +	tdmr_array = kcalloc(tdx_sysinfo.max_tdmrs, sizeof(struct tdmr_info *),
> +			GFP_KERNEL);

Where, exactly is that alignment provided?  A 'struct tdmr_info *' is 8
bytes so a tdx_sysinfo.max_tdmrs=8 kcalloc() would only guarantee
64-byte alignment.

Also, I'm surprised that this is an array of virtual address pointers.
The previous interactions with the TDX module seemed to all take
physical addresses.  How is it that this hardware structure which has
hardware alignment constraints is holding virtual addresses?

> +	if (!tdmr_array) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* Construct TDMRs to build TDX memory */
> +	ret = construct_tdmrs(tdmr_array, &tdmr_num);
> +	if (ret)
> +		goto out_free_tdmrs;
> +
>  	/*
>  	 * Return -EFAULT until all steps of TDX module
>  	 * initialization are done.
>  	 */
>  	ret = -EFAULT;

There's the -EFAULT again.  I'd replace these with a better error code.

> +out_free_tdmrs:
> +	/*
> +	 * TDMRs are only used during initializing TDX module.  Always
> +	 * free them no matter the initialization was successful or not.
> +	 */
> +	free_tdmrs(tdmr_array, tdmr_num);
> +	kfree(tdmr_array);
>  out:
>  	return ret;
>  }
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 2f21c45df6ac..05bf9fe6bd00 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -89,6 +89,29 @@ struct tdsysinfo_struct {
>  	};
>  } __packed __aligned(TDSYSINFO_STRUCT_ALIGNMENT);
>  
> +struct tdmr_reserved_area {
> +	u64 offset;
> +	u64 size;
> +} __packed;
> +
> +#define TDMR_INFO_ALIGNMENT	512
> +
> +struct tdmr_info {
> +	u64 base;
> +	u64 size;
> +	u64 pamt_1g_base;
> +	u64 pamt_1g_size;
> +	u64 pamt_2m_base;
> +	u64 pamt_2m_size;
> +	u64 pamt_4k_base;
> +	u64 pamt_4k_size;
> +	/*
> +	 * Actual number of reserved areas depends on
> +	 * 'struct tdsysinfo_struct'::max_reserved_per_tdmr.
> +	 */
> +	struct tdmr_reserved_area reserved_areas[0];
> +} __packed __aligned(TDMR_INFO_ALIGNMENT);
> +
>  /*
>   * P-SEAMLDR SEAMCALL leaf function
>   */