Received-SPF: pass (google.com: domain of linux-kernel+bounces-178247-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <50e09676-4dfc-473f-8b34-7f7a98ab5228@intel.com>
Date: Tue, 14 May 2024 14:01:12 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 037/130] KVM: TDX: Make KVM_CAP_MAX_VCPUS backend
 specific
To: Sean Christopherson <seanjc@google.com>, Isaku Yamahata
	<isaku.yamahata@intel.com>
CC: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, Paolo Bonzini
	<pbonzini@redhat.com>, Erdem Aktas <erdemaktas@google.com>, Sagi Shahar
	<sagis@google.com>, Bo2 Chen <chen.bo@intel.com>, Hang Yuan
	<hang.yuan@intel.com>, Tina Zhang <tina.zhang@intel.com>,
	<isaku.yamahata@linux.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <9bd868a287599eb2a854f6983f13b4500f47d2ae.1708933498.git.isaku.yamahata@intel.com>
 <Zjz7bRcIpe8nL0Gs@google.com>
 <5ba2b661-0db5-4b49-9489-4d3e72adf7d2@intel.com>
 <Zj1Ty6bqbwst4u_N@google.com>
 <49b7402c-8895-4d53-ad00-07ce7863894d@intel.com>
 <20240509235522.GA480079@ls.amr.corp.intel.com> <Zj4phpnqYNoNTVeP@google.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <Zj4phpnqYNoNTVeP@google.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?K1NCdHBTME5XTW9kSVg3VlhGME9tRCtHOTJxNVZkeXdSRmJHWjkwVzVqdG9E?=
 =?utf-8?B?RUhQakhVdk5xczFXQkJIM0krWkd5NWM5UHJZMjBRdkpPeWVPMFkzTkRVdHpO?=
 =?utf-8?B?SEI3VzA0c1RkMFhnUWFrbHVDaERvWWJQVFZFTC9FVkkwMmVKVWZVWFAvbnc2?=
 =?utf-8?B?MVE3NGJwcXFrMHF1UFNKeXlmdVI5ek1mRzJnNEh5UEJPRTNBRTdveFN4WW15?=
 =?utf-8?B?U0NrbUc1ekpEbml0eDNxWTVGQ2J4QldZcU9vWDVOdUlhYkxjN0M4aU8velY0?=
 =?utf-8?B?M2E2UDFTa0ZJWHg0cFRteFZvSGsxYTNKV0txQkhFdDhUdThpZGZnM3BHKzcx?=
 =?utf-8?B?RHNrbWJPWmF0eXBmSXFyMXU2Yzl0eUNySXhpbjQzRDBoWWJ4TU5RRzZoc0F6?=
 =?utf-8?B?VHVKTWRsNGNuMTBpeFg4cERyR05NN3hOUTV4clZVRzk0QU5VZTV1UHpqSTJW?=
 =?utf-8?B?TXQxU1UrOGNaeFl1SEl5RjdzaFVWSzQ2bVZZQlM4WEQ1cVFoYStha1V4QXNm?=
 =?utf-8?B?d0IyUmJMOFhhRW5qcmtrSW0yNWtnby9UTUdyUE1pZTBBNUR1TEdhR2t2U0hv?=
 =?utf-8?B?YWNsUVpoZUFLQVdDd3RUcXkycURWL3dJOU5FOUVXTzJaM3Y4eGZjc3lQNnRJ?=
 =?utf-8?B?akYrV2tUWXFBUjE0ajhSZzEvVGRINnBkdGxvTGU5MHhXalhwdlMxakE1M3ZS?=
 =?utf-8?B?ZXE2SGtFRDhNNjdJLzlKZHMyVTYrT3M2TnQzYjJkYUxXVFBPTjFQY2xoc2pw?=
 =?utf-8?B?ZUFMbU8vdlB1am9NSDFaZzBVdS82ZFhVOWxpTEx0R05rbFhBbHp3emMrNy9B?=
 =?utf-8?B?bDdWZ0xzUEczVnhBd2dScTE4clhGcGgzM2tjTXlmVWRUS2RvQkduRmlSMkhJ?=
 =?utf-8?B?ek1BN2xRd2drUzBxTm9paWFNdWpqUFFaZDJZTDlReHBzVjdVMlluQXlBY3hw?=
 =?utf-8?B?ZTJrT1hhZ2RWMndOQzVXY1ByMzliV2JRK2Y4REVDWHI5aU5TNjQ2L0lvR1gw?=
 =?utf-8?B?ZUgxRm80Ui9Qbys1eEpleFUxZHY5NHZINmZpZnNoRVl3MUNaR3M4Uzc0aGdk?=
 =?utf-8?B?UFh6VHBuZlllZEM0bytQZ2FmMFZPOWFOT3p6cENQTmROS1d1Njl4U3h6Mkp0?=
 =?utf-8?B?clBYL3RjZDdJR3NhWU5CdUx3UzhaUEp1YmRBemtmSVg4VDh0bXQvT1hSWGkr?=
 =?utf-8?B?UTllMkgxZGsreGNRcUlDLzFOd1pUMUI4YUwwd0FQdDI0cFU2K0praTNtdGNH?=
 =?utf-8?B?TUZJVUMxWVFOSTJabUJmYlUybXBtaFVCRVlqekVhL2hGZlJpZGJ4emxaSklQ?=
 =?utf-8?B?eWlmTjlaRUFSeWRudTV2bnk3L0dLQTYwbEVyTkdoNERSQkZSYmQ0QkVESTZL?=
 =?utf-8?B?cTFvMmUzMjZDektTYnBSdVhXbW9zSUhXVGFBWFBvSzJ5bDFEMWN5ZldlZldt?=
 =?utf-8?B?S1R1MFFBRVc5cCtrTzZkV1ZNRFhZUUR2SjJ0b2Yrd1YwQm1BSjlXMUovMUtC?=
 =?utf-8?B?QUVzS25mWi9vT2RhWEJRcjlCQzNDMitheWovUTlrcDFKWGFMbTNJR3E0aXUw?=
 =?utf-8?B?eHZkWXA0S0pDeFV4aE5aUE1UNk1ubmZwbFN5eVRrT3doL2tGOFllRUNSVWI2?=
 =?utf-8?B?WmxNZnpsQWlqZTFQa05XUDRxeWl1NUZtZktUN1lxRTNzY1JXdWs3cW1xYWZk?=
 =?utf-8?B?bmtEWjRnNWtScEplNUF3b0wySW5IcHpLMy9LVDJzSmRsdTN2RFJjN2t5R1J4?=
 =?utf-8?B?NjZUVkdZdStSTXdNZjBHazZMYm43S2RaYTRsSUZIcEE5Z0ZjQklyVHVDbk8v?=
 =?utf-8?B?eGcycUFmY2tveXBkT0h2cm1FaEZSNk1nN2J6R1ZneFNlbEdaMURON2Q4OHRl?=
 =?utf-8?B?ZGFjSllkTFdyRlJQZnI0QnFwREVCbjVzd01zYzBZcXlYZE82bXFSWEpIRC9k?=
 =?utf-8?B?TmhrTUNWL0RSUFk0YUpkODZ3bDl5bTNIVUdWSE4wKzdGN2N2K29DZUZQeTY0?=
 =?utf-8?B?R1lFcHFwYWJNMUN5S2dOVVpRQzZSMDgwWE9nNzBQUDY5eXg4N05xMlh3dDJ3?=
 =?utf-8?B?RDhndnNPY3UwUkh5eGJQWW5CaEtZejU5L3VLR09XT1ZnZVB3eE1nQ2NkNHFk?=
 =?utf-8?Q?emSFo9HXpjLf+lQh4EkD6b1Ud?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2985b9f4-db1d-430c-ba11-08dc73b9c11c
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2024 02:01:22.7059
 (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: Nv6MAS1/UeXcq2v1qCvTMqObR0UVqv/sxr6vSWwSmRkTswUctrsdrLvBfoGRGw7gZg57OlAxBEktpcqQQFQAcw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6497
X-OriginatorOrg: intel.com


On 11/05/2024 2:04 am, Sean Christopherson wrote:
> On Thu, May 09, 2024, Isaku Yamahata wrote:
>> On Fri, May 10, 2024 at 11:19:44AM +1200, Kai Huang <kai.huang@intel.com> wrote:
>>> On 10/05/2024 10:52 am, Sean Christopherson wrote:
>>>> On Fri, May 10, 2024, Kai Huang wrote:
>>>>> On 10/05/2024 4:35 am, Sean Christopherson wrote:
>>>>>> KVM x86 limits KVM_MAX_VCPUS to 4096:
>>>>>>
>>>>>>      config KVM_MAX_NR_VCPUS
>>>>>> 	int "Maximum number of vCPUs per KVM guest"
>>>>>> 	depends on KVM
>>>>>> 	range 1024 4096
>>>>>> 	default 4096 if MAXSMP
>>>>>> 	default 1024
>>>>>> 	help
>>>>>>
>>>>>> whereas the limitation from TDX is apprarently simply due to TD_PARAMS taking
>>>>>> a 16-bit unsigned value:
>>>>>>
>>>>>>      #define TDX_MAX_VCPUS  (~(u16)0)
>>>>>>
>>>>>> i.e. it will likely be _years_ before TDX's limitation matters, if it ever does.
>>>>>> And _if_ it becomes a problem, we don't necessarily need to have a different
>>>>>> _runtime_ limit for TDX, e.g. TDX support could be conditioned on KVM_MAX_NR_VCPUS
>>>>>> being <= 64k.
>>>>>
>>>>> Actually later versions of TDX module (starting from 1.5 AFAICT), the module
>>>>> has a metadata field to report the maximum vCPUs that the module can support
>>>>> for all TDX guests.
>>>>
>>>> My quick glance at the 1.5 source shows that the limit is still effectively
>>>> 0xffff, so again, who cares?  Assert on 0xffff compile time, and on the reported
>>>> max at runtime and simply refuse to use a TDX module that has dropped the minimum
>>>> below 0xffff.
>>>
>>> I need to double check why this metadata field was added.  My concern is in
>>> future module versions they may just low down the value.
>>
>> TD partitioning would reduce it much.
> 
> That's still not a reason to plumb in what is effectively dead code.  Either
> partitioning is opt-in, at which I suspect KVM will need yet more uAPI to express
> the limitations to userspace, or the TDX-module is potentially breaking existing
> use cases.

The 'max_vcpus_per_td' global metadata fields is static for the TDX 
module.  If the module supports the TD partitioning, it just reports 
some smaller value regardless whether we opt-in TDX partitioning or not.

I think the point is this 'max_vcpus_per_td' is TDX architectural thing 
and kernel should not make any assumption of the value of it.  The 
architectural behaviour is:

   If the module has this 'max_vcpus_per_td', software should read and
   use it; Otherwise software should treat it as U16_MAX.

Thus I don't think we will need a new uAPI (TDX specific presumably) 
just for TD partitioning.  And this doesn't break existing use cases.

In fact, this doesn't prevent us from making the KVM_CAP_MAX_VCPUS code 
generic, e.g., we can do below:

1) In tdx_vm_init() (called via KVM_VM_CREATE -> vt_vm_init()), we do:

	kvm->max_vcpus = min(kvm->max_vcpus,
				tdx_info->max_vcpus_per_td);

2) In kvm_vm_ioctl_enable_cap_generic(), we add support to handle 
KVM_CAP_MAX_VCPUS to have the generic code to do:

	if (new_max_vcpus > kvm->max_vcpus)
		return -EINVAL;

	kvm->max_vcpus = new_max_vcpus;

However this means we only allow "lowing down" the kvm->max_vcpus in the 
kvm_vm_ioctl_enable_cap_generic(KVM_CAP_MAX_VCPUS), but I think this is 
acceptable?

If it is a concern, alternatively, we can add a new 
'kvm->hard_max_vcpus' (or whatever makes sense), and set it in 
kvm_create_vm() right after kvm_arch_init_vm():

	r = kvm_arch_init_vm(kvm, type);
         if (r)
                 goto out_err_no_arch_destroy_vm;

	kvm->hard_max_vcpus = kvm->max_vcpus;

So it always contains "the max_vcpus limited by the ARCH 
hardware/firmware etc".

And in kvm_vm_ioctl_enable_cap_generic(), we check against 
kvm->hard_max_vcpus instead to get rid of the limitation of only 
allowing lowing down the kvm->max_vcpus.

But I don't think this is necessary at this stage.