Received-SPF: pass (google.com: domain of linux-kernel+bounces-110698-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <dd389847-6f67-4f5d-8358-5d6b6a493797@intel.com>
Date: Fri, 22 Mar 2024 10:37:20 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 024/130] KVM: TDX: Add placeholders for TDX VM/vcpu
 structure
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>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <c857863a346e692837b0c35da8a0e03c45311496.1708933498.git.isaku.yamahata@intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <c857863a346e692837b0c35da8a0e03c45311496.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?NGlKWXgwWDVKZldqQmNabUo5bGphckJQMmxVaUhrMDl1WHBSQk4rclloWWcr?=
 =?utf-8?B?Q3NYZllGSm1HL09iTmhiL2xtQjBPZkpOdTZ2RmVmMzlDYS80Q25ybjNiZUdF?=
 =?utf-8?B?YjYxZ1VPYk1kL2RXeW5ucXJyQ2hqWS9yanZwa2RjdjFPZnZJUGJhSkphMEZl?=
 =?utf-8?B?Q3NLZCtXRDF6d092TEdrWW5LemRjRnQzWmFidllpS2l0SHdMNnhiOFVQZHNs?=
 =?utf-8?B?LzNjRXZOSXZFQXBnckRHVDN1eGp6c0FIbFdCWlcvZldnc09RTHZLZkZGRHc3?=
 =?utf-8?B?VHZqT0VvOWN2VFNYTEkwZXY5MWtpS2E4RzlWa01LdlhKeWFLMTYyWXpaMlBR?=
 =?utf-8?B?VTI1TTFyZzlMcFpWanpTUUx2Vmh5d3dBbHlJNmpOMDNSMW0vcC9oVlcxeU5O?=
 =?utf-8?B?dGR3Z2pZbFoxQlhRaFMrYzBGTU1NTVdKVDViOWFueGFKTXVwdFVmWDM0bzMx?=
 =?utf-8?B?aVRrNXhZUG5uRUh6WjNsMjZsUVhlaEdWWTVCdnhYV0FHNk5aSCtyeGJhUUdl?=
 =?utf-8?B?dE1Zbzczei9KeVRUZ1hGS1VpdnZCZW1NenMxVUxHRVFGWVlRTDBjVzlvdnJX?=
 =?utf-8?B?VWlPWFpMb3Q1bER3WHBaYmtzRjFhR2NjY2E4Z0pDUW5BWWJxV3BkNk53bkxW?=
 =?utf-8?B?TUpiZXg2VWx1MW9uWlNrSjROUnhqUy9wVUU4NFJJY0VWeDR3eHdqeWRvbk9U?=
 =?utf-8?B?eDhIOFRQVm10aGdHV3ZwR0NpNGJUN1pYcWxPVkorQ0pSWThwODFFQWZkKzVX?=
 =?utf-8?B?azdVdU5Vd2VVSkp3MjVIR0w4ZVVsMzdtakxWUlQ1Vzd3THNlMDFSUVk3T1Fn?=
 =?utf-8?B?OUJIT3hnNFRPdHpsOTRvMHA2TldjTG5HMHE4Z0RiVndrTU9OcGdLbE5pWkl5?=
 =?utf-8?B?ZkE4OXVlTmRZRFJzSkR6RTFaT04rOUUwVEsvN1hrdGs3ZVlEMzJVOXN4Mjhz?=
 =?utf-8?B?Uy9tRklIY1FJamhYZlYwWlg0YVdZYXpDOVdBVDZHci9vdFhmQnFYaFdvVW9Z?=
 =?utf-8?B?ZkFVOGx4aC9STzd6MXRoSGRMd1ZsUExkc2dKRjRIWi9nTjJxaklIbXRzRURI?=
 =?utf-8?B?bkVNcHpLUVdhZEkrNDJTM2NnQ0g1R1JRS3pqeEJmL3dNSDJBQ1VtdjhtTE9K?=
 =?utf-8?B?d1JVTUd1ckhJc2x3VFEyZUJJMGhDM3Y3QSt2LzhkdERFR21YV0doMlgwRm1z?=
 =?utf-8?B?TmVqRXkwOEp5bW1NVDFnTTBEZTcvYlk3bUxqU3lKTFYrcXRUeHBXc2JZUTVj?=
 =?utf-8?B?aFNmMFloSDFrUjlTRDYzY2d6VjMvNHMrYi9aWkM4TnE0OXdNNmpUdURRUWZj?=
 =?utf-8?B?cyszL0xRT1ZPL3JKU0xDTG9sQjNsRFpJWU90R095bXpNb3NUVnIzRnVEek5p?=
 =?utf-8?B?U0k3YTlXazZETy93ZU9uUkxnUE94c0w5bS9zUmVMaEZvNVQ1YzZ2VjgzNkJG?=
 =?utf-8?B?SVVTMVhHZEZURGJnRWZsRVJjRW5RejhqWjE3ZERzQW1SbTNERS9acWhGQUNl?=
 =?utf-8?B?SmNFbXNDTGhTRXFsTU9HRzZJVmVaK1RPbzhvQXgxM3BkKzNqQ1JITVdyYlho?=
 =?utf-8?B?SWM3YlBPUHJSN2lKcHRIZ3Y3MHhvSUQxNDBvaVA3c3kvb2l1YmF6ckt4cTZD?=
 =?utf-8?B?QlBwVWhxTFg1NFk1ZjY5MDQ4KytWTXlyRHRzVG1GaDBtZjVnVkxUVExFdGxF?=
 =?utf-8?B?MDliaHRKN1VQZzhiTWlxQk8yNTVXR1pyYUd4b01JdmdvTUw1bjUvZUt3RXpv?=
 =?utf-8?B?Uk1sSm1YSW42Wlk1SmcwRzZYcXRXRkxWZHVuSGlQY0lrVjJTSVFpczVCVFRk?=
 =?utf-8?B?c25za3hKVDdVNmtFTkhBQlRieXVleStOK3V1MFZoMEI1VU1janhMbGdyZUlo?=
 =?utf-8?B?dm1BTlRWYnFuQnE5ekRuMVpQRzFGUXJJRkxBSHZNRlp2Z1Fic1BKSERMbGtU?=
 =?utf-8?B?RTBPSHd3MGxKN3h1WE1SdmRKTCs3THFBbDFNZG12NnBtb1RWdkU1bWdlZGpI?=
 =?utf-8?B?Z09GeW15aGR2UFlTcndxY25CWGNmNHVEcTd6L2hCcTdjOXZSY2VkUU9CT2Ry?=
 =?utf-8?B?NEhiNWRKOGtmU3VrWXBpTkxwWWVsL2pCUTUxNm9KZlNsQUkxSWtEQ1F0UTZy?=
 =?utf-8?Q?8znDAGsMGa6w7EAwqmnhhe/PG?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 11969240-71b1-4ae2-6657-08dc49ef1cc6
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:37:31.0078
 (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: 1kD8SSde3t+FZl/WwbUSsFIXLTxKP2Ju7Ofs6VE1a/7QuEPCBlcfJpSrBO0JE5fhZ7X15W/1JcmJ/rIkVe+HQw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4554
X-OriginatorOrg: intel.com


On 26/02/2024 9:25 pm, Yamahata, Isaku wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Add placeholders TDX VM/vcpu structure that overlays with VMX VM/vcpu
> structures.  Initialize VM structure size and vcpu size/align so that x86
> KVM common code knows those size irrespective of VMX or TDX.  Those
> structures will be populated as guest creation logic develops.
> 
> Add helper functions to check if the VM is guest TD and add conversion
> functions between KVM VM/VCPU and TDX VM/VCPU.

The changelog is essentially only saying "doing what" w/o "why".

Please at least explain why you invented the 'struct kvm_tdx' and 
'struct vcpu_tdx', and why they are invented in this way.

E.g., can we extend 'struct kvm_vmx' for TDX?

struct kvm_tdx {
	struct kvm_vmx vmx;
	...
};

> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> ---
> v19:
> - correctly update ops.vm_size, vcpu_size and, vcpu_align by Xiaoyao
> 
> v14 -> v15:
> - use KVM_X86_TDX_VM
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>   arch/x86/kvm/vmx/main.c | 14 ++++++++++++
>   arch/x86/kvm/vmx/tdx.c  |  1 +
>   arch/x86/kvm/vmx/tdx.h  | 50 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 65 insertions(+)
>   create mode 100644 arch/x86/kvm/vmx/tdx.h
> 
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 18aef6e23aab..e11edbd19e7c 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -5,6 +5,7 @@
>   #include "vmx.h"
>   #include "nested.h"
>   #include "pmu.h"
> +#include "tdx.h"
>   
>   static bool enable_tdx __ro_after_init;
>   module_param_named(tdx, enable_tdx, bool, 0444);
> @@ -18,6 +19,9 @@ static __init int vt_hardware_setup(void)
>   		return ret;
>   
>   	enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> +	if (enable_tdx)
> +		vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
> +					   sizeof(struct kvm_tdx));
>   

Now I see why you included 'struct kvm_x86_ops' as function parameter.

Please move it to this patch.

>   	return 0;
>   }
> @@ -215,8 +219,18 @@ static int __init vt_init(void)
>   	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
>   	 * exposed to userspace!
>   	 */
> +	/*
> +	 * kvm_x86_ops is updated with vt_x86_ops.  vt_x86_ops.vm_size must
> +	 * be set before kvm_x86_vendor_init().
> +	 */
>   	vcpu_size = sizeof(struct vcpu_vmx);
>   	vcpu_align = __alignof__(struct vcpu_vmx);
> +	if (enable_tdx) {
> +		vcpu_size = max_t(unsigned int, vcpu_size,
> +				  sizeof(struct vcpu_tdx));
> +		vcpu_align = max_t(unsigned int, vcpu_align,
> +				   __alignof__(struct vcpu_tdx));
> +	}

Since you are updating vm_size in vt_hardware_setup(), I am wondering 
whether we can do similar thing for vcpu_size and vcpu_align.

That is, we put them both to 'struct kvm_x86_ops', and you update them 
in vt_hardware_setup().

kvm_init() can then just access them directly in this way both 
'vcpu_size' and 'vcpu_align' function parameters can be removed.


>   	r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE);
>   	if (r)
>   		goto err_kvm_init;