Received-SPF: pass (google.com: domain of linux-kernel+bounces-103833-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <075d7013-6f5b-4446-a41c-d9ae754d4bd9@intel.com>
Date: Fri, 15 Mar 2024 11:27:56 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 033/130] KVM: TDX: Add helper function to read TDX
 metadata in array
Content-Language: en-US
To: <isaku.yamahata@intel.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
CC: <isaku.yamahata@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>,
	<erdemaktas@google.com>, Sean Christopherson <seanjc@google.com>, Sagi Shahar
	<sagis@google.com>, <chen.bo@intel.com>, <hang.yuan@intel.com>,
	<tina.zhang@intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <72b528863d14b322df553efa285ef4637ee67581.1708933498.git.isaku.yamahata@intel.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <72b528863d14b322df553efa285ef4637ee67581.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UDVjdGc5YWZlYUt0bXNTUkVzVWlSVDFuNFA2bGtNZzBMcVpWbXlES2NrOTFB?=
 =?utf-8?B?aWs5RERWWGZvWmpFVlhEYWxrR01IQjY1MWpzTU93ZXVLU2EzTlRWOGtnWG0y?=
 =?utf-8?B?V2FBb3N2Q0tNd0NQN3pVK2ljRnNKU3BJZXBFQ1FWaU9NNjkxdDl0dWdWTXdO?=
 =?utf-8?B?a0JQRW1oS3JmaGZFb2YvQzlvRUdmZXJqRGVhUzR3QXprc1hkSVJJNExRcXEx?=
 =?utf-8?B?cU1PSGxSRzc1Q3VPWkRQMEVWMDF2c0JrL0pQRDd2NnJSWkRsZ2hHWUNvUVlq?=
 =?utf-8?B?RWFXTHE5NUJRdjl4Mm1HeElKblN2SG1XbjNSbzA3Y2g0OWZDdHNyYlNwVHlM?=
 =?utf-8?B?ZjlmY3NLVkorYk5kdGRSMkNiZ283NDVXN2pmMURjZUNzZUZjMDJuL2RZamxi?=
 =?utf-8?B?ekRpQzhOWkRDN2lQZlBLd3doWHBkcDlsamo3U2JNVWdDQkR0RnFsUTl5S1Ux?=
 =?utf-8?B?M25ZNm9xRFRFSDRWRUc2K09lSmJyaWo5UGYwUVNpQzlnQVo0RkJZUFlVR0xJ?=
 =?utf-8?B?bnFELzBCbGZoOVJOdzkxWmRmY1N3OXJScUZ0bWNlUUpITG52TzUrUmJRUzdu?=
 =?utf-8?B?ODBET20rbWFJVFhtSDFkVFh5L3hueDNuZGVyd2Rwc2Z1ZmdWYk9NclYvbm5J?=
 =?utf-8?B?YkpmdDM1WnUxZmdSKzFrWVk2cU1qRW53UnZTZFUvalltN29tM1FrckZ1U0l6?=
 =?utf-8?B?bEtZYldNRm9vNHhiRU5PRW96dkpsdkhEWjZMUko1TTRXYlVCdzQwNk9DcUky?=
 =?utf-8?B?M3QyN3ZnbFNhUE91QVhwaWFiQVNsSFdBamlwRnhsdGZVMThoc1o1VjJRTmYy?=
 =?utf-8?B?YnB0KzNTUHVTQTN5QWwrMlQ0M0l6SHFKSCtQZzI0OW10cWMrdlZtWkhXQ2d1?=
 =?utf-8?B?TzRXcCtMU0V6eVF0ZHVyK3Y1Yy9hY0krQXpVM3hOUzM5cVJhcWVKYUFieDc5?=
 =?utf-8?B?V21LWGZQQ2tMTWxheTJ1MThXTTdCOTdMYksxcWtDazhEaEZyNVkrdHIvQ3B5?=
 =?utf-8?B?THJtWS9Qb1doSkZtVGFwVjFUQ3NoVTFwc0s1OFJlTlQzVzg2THNiNUU1WjBK?=
 =?utf-8?B?SkxOWUQ4OHE4TENDaWZvZUtQVkNlZzVCNEI1MzRGQWJSTSszaXQxclMyUDBr?=
 =?utf-8?B?aHYzenM1WEZ5MDlObWpneEpsR0pkTjNEc05PYmM4WitrTnZkOFkrMzVITmNk?=
 =?utf-8?B?RUlyZG1HV1l4UEpiK0E3UG9hc001TlYzNU9hQk1GaEFVRUNiTU5zZEdxVlgr?=
 =?utf-8?B?YWNVU2tOcWhqMVR4T3NoVTlYdXc1UytXc2tTTkdkVjFzQTNVeVJpNWpTM3p6?=
 =?utf-8?B?SXRqMXY4cTZLSm9LODFwQkZvT294TVdjUk9BYi96b1JNLzZDbnVjaTkyRmto?=
 =?utf-8?B?R2d3WE1DcHNibU51K3RKcjVMdlNmakd4eTVqNDNDNTVMcmE3U0tybkVQUkx4?=
 =?utf-8?B?b0kzQ3ZCNEpneXVoTHpWR3ZnZXZ2MHR3clNXZU1TbzZ1dHR0RmdoWmxDZ3NV?=
 =?utf-8?B?dVN0cHZyYjlna0NUQ1FIYkxMSlRkNlRtQ0NSNjNkZlhjenVnZ0w5cXg1ejFx?=
 =?utf-8?B?c2toL2ZsL216Ym91OTZNUFdzdmlFU1NnanB3L3hORVVncmhKVldGaXRiWTFC?=
 =?utf-8?B?LzhIcVRGbkVuM1Y2eGtqSXhDNG90VkxGVmsvSjN0KzFrdXUrL285ZHhvOElL?=
 =?utf-8?B?dWdlb1lqTFNkQk1iQzFYT0VyVVZwbVNuWCtyNmxhL0YxRFFWSHp3Ny9WSXNB?=
 =?utf-8?B?WjBXdW9aNVVPN1AzZUt2NGdOZGhMTXV2dzJCeXBIdVFXZ09ZM2d2RmJKVncw?=
 =?utf-8?B?TUkzelZLUXBKSy9mV0hCRndpUDQyUmR1UUVyNUdOcVp5VnFaYmxDNFJnUXF3?=
 =?utf-8?B?dk5ZdXowb3dZbWhYZG5vSjNzQ01ZdlZ5VDNULzNUWG42Y2xZNG8yTmtaTmpw?=
 =?utf-8?B?ZnUrZUl4eVBwczM3cDZkbHhEZUpua1UzeXN6M0VFRk1pbkZCMHZHeWZELzFl?=
 =?utf-8?B?dFdyS3N5b01mRG9MZjgwVGY3Qlp1K3V3dy9FT3JzaHpRRCtCRVcxT0JIL2hY?=
 =?utf-8?B?ZnByNm1hL3QxWXpNZVVKQWJMM2FUaXB2alZNSFVkaTBUbUFpVVFUZ1l2OEMv?=
 =?utf-8?Q?hVXQibJ3jBfs4Pc5YSU3HwGnx?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a1734418-03e9-4042-3fe5-08dc447603f7
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2024 22:28:04.5181
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JrOsbJ0aanigy1JR2UkYTeHfNZaPrNqTD6JtUxDDviJYIpMQ4KWz4hrAYdlROdVux7OXZbiXEXFdtFM8wAi+4w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6602
X-OriginatorOrg: intel.com


On 26/02/2024 9:25 pm, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> To read meta data in series, use table.
> Instead of metadata_read(fid0, &data0); metadata_read(...); ...
> table = { {fid0, &data0}, ...}; metadata-read(tables).

This explains nothing why the code introduced in patch 5 cannot be used.

> TODO: Once the TDX host code introduces its framework to read TDX metadata,
> drop this patch and convert the code that uses this.

Seriously, what is this?? Please treat your patches as "official" patches.

> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
> v18:
> - newly added
> ---
>   arch/x86/kvm/vmx/tdx.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index cde971122c1e..dce21f675155 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -6,6 +6,7 @@
>   #include "capabilities.h"
>   #include "x86_ops.h"
>   #include "x86.h"
> +#include "tdx_arch.h"
>   #include "tdx.h"
>   
>   #undef pr_fmt
> @@ -39,6 +40,50 @@ static void __used tdx_guest_keyid_free(int keyid)
>   	ida_free(&tdx_guest_keyid_pool, keyid);
>   }
>   
> +#define TDX_MD_MAP(_fid, _ptr)			\
> +	{ .fid = MD_FIELD_ID_##_fid,		\
> +	  .ptr = (_ptr), }
> +
> +struct tdx_md_map {
> +	u64 fid;
> +	void *ptr;
> +};
> +
> +static size_t tdx_md_element_size(u64 fid)
> +{
> +	switch (TDX_MD_ELEMENT_SIZE_CODE(fid)) {
> +	case TDX_MD_ELEMENT_SIZE_8BITS:
> +		return 1;
> +	case TDX_MD_ELEMENT_SIZE_16BITS:
> +		return 2;
> +	case TDX_MD_ELEMENT_SIZE_32BITS:
> +		return 4;
> +	case TDX_MD_ELEMENT_SIZE_64BITS:
> +		return 8;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return 0;
> +	}
> +}
> +
> +static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> +{
> +	struct tdx_md_map *m;
> +	int ret, i;
> +	u64 tmp;
> +
> +	for (i = 0; i < nr_maps; i++) {
> +		m = &maps[i];
> +		ret = tdx_sys_metadata_field_read(m->fid, &tmp);
> +		if (ret)
> +			return ret;
> +
> +		memcpy(m->ptr, &tmp, tdx_md_element_size(m->fid));
> +	}
> +
> +	return 0;
> +}
> +

It's just insane to have two duplicated mechanism for metadata reading.

This will only confuse people.

If there's anything missing in patch 1-5, we can enhance them.