Received-SPF: pass (google.com: domain of linux-kernel+bounces-110712-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <b7b919eb-81fd-4f27-86c1-6e160754608e@intel.com>
Date: Fri, 22 Mar 2024 10:57:53 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural
 definitions
To: "Yamahata, Isaku" <isaku.yamahata@intel.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
CC: "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, Paolo Bonzini
	<pbonzini@redhat.com>, "Aktas, Erdem" <erdemaktas@google.com>, "Sean
 Christopherson" <seanjc@google.com>, Sagi Shahar <sagis@google.com>, "Chen,
 Bo2" <chen.bo@intel.com>, "Yuan, Hang" <hang.yuan@intel.com>, "Zhang, Tina"
	<tina.zhang@intel.com>, Sean Christopherson
	<sean.j.christopherson@intel.com>, "Li, Xiaoyao" <Xiaoyao.Li@intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <522cbfe6e5a351f88480790fe3c3be36c82ca4b1.1708933498.git.isaku.yamahata@intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <522cbfe6e5a351f88480790fe3c3be36c82ca4b1.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?UGVId25mQTc1Nzg1K284WDByZjdBYXBHMVdLblRwTVIxeXJnRUxDT2J6L3hJ?=
 =?utf-8?B?MTdYQzJ2K1FOUHo3T3QwaEU1RzdGaXRDYU5xWG83akVjYWI5MzFxUmZpaWF2?=
 =?utf-8?B?K0VHdnl6U2Z1TFY5WXJ5UVg2NVdTejQ0OFcxcFd6bHdFQzlrVXU5OC9RcDZ0?=
 =?utf-8?B?Q1N5YWVlckdla1NuREpGTUpxdEU3emx2RXAya0lqb3ZCQlI0dXNkTkpWaURF?=
 =?utf-8?B?TDg5Vm1KUFU4SmpuUUxnOG5TZHlRNUJGcWxzOUQ4TDhvbjVTeUh5QjF6NUZl?=
 =?utf-8?B?aUwvZmcwakpRdWZzZm1vT3plN1dZMUR3Y0pLbmx1SVhzdU5rS25paVlKbW50?=
 =?utf-8?B?OGMvY0ViWUs0dmxJS1JodWtMQkZzNnJ5a2d1c1ZmcGdXOU5vV0pkWnN0ZjFI?=
 =?utf-8?B?dnh0dU1xeWNMRmhhT25ZTkJ0WkNMTjhXZWZiaWtlcHd1ejYzK0xOWHB2QW0y?=
 =?utf-8?B?eGQxOFFZOERBb1BzNzFPRElTaktQaWdHQ3FKVDZhT2hXNHVCL2h3Z0J3MXB1?=
 =?utf-8?B?WG9KSEs5dnR2amt5cnhqUWJ3T0hQT3VEV1BTcTVMVThlNXZsU0xDSnpXRXFN?=
 =?utf-8?B?NWd6ZDl6M0hML05oZzdEZDVDZEJOYkRJMjZhcWk1Zm1iNWQ4UmR6Y0FVaFBJ?=
 =?utf-8?B?TkVJNk9TRWdvZE8wNW84WGRXZUsrSVZnZXFaRk5LUUFIMC9NeTZnRFZTYTQ2?=
 =?utf-8?B?TkthenpxTXByaG1peGd5UUtSbC9iRFJETGpWSy8xbjVDY21MNUExNGMrdnZh?=
 =?utf-8?B?Sjd2R2VWUDZRRXhxcDFpWklxYld0bVA2S2pLNzFBRWFObU50Vk54KzJXaUxl?=
 =?utf-8?B?WWN0SWQ0OEJFNDk3SlZ5bVFhUm5FaG5EamJmbmdHU25MWGlQT3lHSlR3MUpH?=
 =?utf-8?B?Y0FYVnhvZ0VCYlF1czZ2WXVKRnZKLzRsSWNuZlBLa1VGV09NVU8rQU01Zk1G?=
 =?utf-8?B?YVpjYnI3QXNLak5PT3NCdDhDaGY4KzJVcFVoZmR2TGtkT0hiL2pYeTVjY0Zm?=
 =?utf-8?B?WmhQQThKaUNJejBoK2dNanZGZHRmYVJ1Tk1nNEdqUGt6KzRudzRNMWxPajBE?=
 =?utf-8?B?ZHl6NjlzdHNMYkk1aGtUUWg2Z0dRc29PcXpYYnZ3VW1wVUNJbzVPa1Q2RTNY?=
 =?utf-8?B?WHAyMVcySUI1Qk94ZXBra0s3eks3UU94dURTbU5vVjZUbGRBMFNLRVRuU2NZ?=
 =?utf-8?B?Vm5oSUtVWENqUHRPSkUzajhsb21KakE5Z0o4VE9CcGxWRjNhTXN0MEVCZWgy?=
 =?utf-8?B?WW1TMVB3Vmg3a09pLzU0ZkVteG1sVGxTa1ZySFlNMElmREpkNnZJYk9vTUNN?=
 =?utf-8?B?N3lYVDdDVFI5RzZ1L3JFdEtPblEwRERGODQ0RUpvS2NvVDBJeVgxVXpDL0dW?=
 =?utf-8?B?bHhjVlNMM3E3dG5TaWV5N09SMFEvcHJOWXl4ZUxLZmp6T2N3c1V0b1pldytE?=
 =?utf-8?B?WGNDMjNkc3gzdmo3R2NFRW9UaWVlc1pvMUY3cmxqdEdQbWFobGtkUTljY3k2?=
 =?utf-8?B?Zmc1MXJpYzZJczRzamtvUnZScjRUQVNLc0F4akVyeG91cEJHUUdVNFdrSUFD?=
 =?utf-8?B?bWtwUVNVRW9JbTFpc3N4NHdISU1oYXpheVFMaDhGcUpaOXRLMUFUSXVGUFU4?=
 =?utf-8?B?M001ajBINE1SVEVjcTBtRHFseFR4NFdJY0RlS1lIYWZQRmorVUJuNWwxSVdp?=
 =?utf-8?B?bnhibGZXQkMwM0ZPWXdyZTJQRWh4cjRYZmJMcmh2Z0I0Mm9RcTdQT2tWS2Fp?=
 =?utf-8?B?bi8zS0tYQ0xYSDBuU3dnQkNNOEplQWJNRUg0UjdKdEs3M3hwa2FSTW1NWXls?=
 =?utf-8?B?L0hYR0IrZGxxZEJuKzVINDNXdEpNMkJkRnIzK3Z4Z0YxQUFPQUlyQTJadHEx?=
 =?utf-8?B?ZVRZaG9GSE92NElKbXppQzgvNGhIS0JlT3djaTlqK0Z0bnNocHVHRGtDSzMw?=
 =?utf-8?B?MmpPMmZGMmt2U3ZLYzZrelNJMUZwdEQ1a3VyMkNhNmtpcGxRUFFqOEREbUF2?=
 =?utf-8?B?QTZYMUw2RWhtUklWcGFmUW5RWFBQMXIzS2kvU3k4TnJsdXBOREhmZW1wK1hh?=
 =?utf-8?B?R2VJUlVqb25zdWxSMHZZUmY2MnU4M1dyNm1NK1RMTjA2NUdSVXFzL1RzN2NV?=
 =?utf-8?Q?OsPZIAzKrZv9dfV9Vq1sRTg2Q?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b23fcd5f-3f9c-4ef0-c6ef-08dc49f1fbae
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2024 21:58:03.9260
 (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: s0B6KdvLtEMpFr7mynVvnU74LXrfeMzoMW77ujhkKWmm3B0TjroC/YZgAvrv4q1wKWK4I7CfmTyAA5xYCGzKbQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6683
X-OriginatorOrg: intel.com


> +/*
> + * TDX SEAMCALL API function leaves
> + */
> +#define TDH_VP_ENTER			0
> +#define TDH_MNG_ADDCX			1
> +#define TDH_MEM_PAGE_ADD		2
> +#define TDH_MEM_SEPT_ADD		3
> +#define TDH_VP_ADDCX			4
> +#define TDH_MEM_PAGE_RELOCATE		5

I don't think the "RELOCATE" is needed in this patchset?

> +#define TDH_MEM_PAGE_AUG		6
> +#define TDH_MEM_RANGE_BLOCK		7
> +#define TDH_MNG_KEY_CONFIG		8
> +#define TDH_MNG_CREATE			9
> +#define TDH_VP_CREATE			10
> +#define TDH_MNG_RD			11
> +#define TDH_MR_EXTEND			16
> +#define TDH_MR_FINALIZE			17
> +#define TDH_VP_FLUSH			18
> +#define TDH_MNG_VPFLUSHDONE		19
> +#define TDH_MNG_KEY_FREEID		20
> +#define TDH_MNG_INIT			21
> +#define TDH_VP_INIT			22
> +#define TDH_MEM_SEPT_RD			25
> +#define TDH_VP_RD			26
> +#define TDH_MNG_KEY_RECLAIMID		27
> +#define TDH_PHYMEM_PAGE_RECLAIM		28
> +#define TDH_MEM_PAGE_REMOVE		29
> +#define TDH_MEM_SEPT_REMOVE		30
> +#define TDH_SYS_RD			34
> +#define TDH_MEM_TRACK			38
> +#define TDH_MEM_RANGE_UNBLOCK		39
> +#define TDH_PHYMEM_CACHE_WB		40
> +#define TDH_PHYMEM_PAGE_WBINVD		41
> +#define TDH_VP_WR			43
> +#define TDH_SYS_LP_SHUTDOWN		44

And LP_SHUTDOWN is certainly not needed.

Could you check whether there are others that are not needed?

Perhaps we should just include macros that got used, but anyway.

[...]

> +
> +/*
> + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is 1024B.
> + */

Why is this comment applied to TDX_MAX_VCPUS?

> +#define TDX_MAX_VCPUS	(~(u16)0)

And is (~(16)0) an architectural value defined by TDX spec, or just SW 
value that you just put here for convenience?

I mean, is it possible that different version of TDX module have 
different implementation of MAX_CPU, e.g., module 1.0 only supports X 
but module 1.5 increases to Y where Y > X?

Anyway, looks you can safely move this to the patch to enable CAP_MAX_CPU?

> +
> +struct td_params {
> +	u64 attributes;
> +	u64 xfam;
> +	u16 max_vcpus;
> +	u8 reserved0[6];
> +
> +	u64 eptp_controls;
> +	u64 exec_controls;
> +	u16 tsc_frequency;
> +	u8  reserved1[38];
> +
> +	u64 mrconfigid[6];
> +	u64 mrowner[6];
> +	u64 mrownerconfig[6];
> +	u64 reserved2[4];
> +
> +	union {
> +		DECLARE_FLEX_ARRAY(struct tdx_cpuid_value, cpuid_values);
> +		u8 reserved3[768];

I am not sure you need the 'reseved3[768]', unless you need to make 
sieof(struct td_params) return 1024?

> +	};
> +} __packed __aligned(1024); > +

[...]

> +
> +#define TDX_MD_ELEMENT_SIZE_8BITS      0
> +#define TDX_MD_ELEMENT_SIZE_16BITS     1
> +#define TDX_MD_ELEMENT_SIZE_32BITS     2
> +#define TDX_MD_ELEMENT_SIZE_64BITS     3
> +
> +union tdx_md_field_id {
> +	struct {
> +		u64 field                       : 24;
> +		u64 reserved0                   : 8;
> +		u64 element_size_code           : 2;
> +		u64 last_element_in_field       : 4;
> +		u64 reserved1                   : 3;
> +		u64 inc_size                    : 1;
> +		u64 write_mask_valid            : 1;
> +		u64 context                     : 3;
> +		u64 reserved2                   : 1;
> +		u64 class                       : 6;
> +		u64 reserved3                   : 1;
> +		u64 non_arch                    : 1;
> +	};
> +	u64 raw;
> +};

Could you clarify why we need such detailed definition?  For metadata 
element size you can use simple '&' and '<<' to get the result.

> +
> +#define TDX_MD_ELEMENT_SIZE_CODE(_field_id)			\
> +	({ union tdx_md_field_id _fid = { .raw = (_field_id)};  \
> +		_fid.element_size_code; })
> +
> +#endif /* __KVM_X86_TDX_ARCH_H */