Received-SPF: pass (google.com: domain of linux-kernel+bounces-147668-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <e0713aaa-0537-403b-9abe-db267e21f0b7@intel.com>
Date: Wed, 17 Apr 2024 10:06:03 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural
 definitions
To: "Yamahata, Isaku" <isaku.yamahata@intel.com>
CC: Isaku Yamahata <isaku.yamahata@linux.intel.com>, "Zhao, Yan Y"
	<yan.y.zhao@intel.com>, "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>, "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>
 <ZeGC64sAzg4EN3G5@yzhao56-desk.sh.intel.com>
 <20240305082138.GD10568@ls.amr.corp.intel.com>
 <34d64c12-9ed5-4c63-8465-29f7fdce20dc@intel.com>
 <20240416162829.GX3039520@ls.amr.corp.intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240416162829.GX3039520@ls.amr.corp.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?ckpuUDF6QzQ4aHY5aURHZDVETlVUSjlDeXplREF0b20wdmRlK0lScWIrdTFs?=
 =?utf-8?B?dTk1a0NWNWoyeml5dmIwdmxRT0w2NXE2VlExR2NxRUsrZkpEeFI3QW85UXFZ?=
 =?utf-8?B?ajQ4MlhmSkxUZzUxRGt5NmtDZ3ZCWWxnWnlVaUlJalg0N1VENnZBV0dOQ1Ex?=
 =?utf-8?B?MlBjeEZ4Y3daRENMbnZxTWRlOVl6cjZMUi96MytNYmgwRzRHZTdYWXZ3aWtq?=
 =?utf-8?B?cGZNVTdVVnNyMmx4L2l2VkZaM2ZOWkt5cm1ucXYraVlVRElpVXlMeW16dkw2?=
 =?utf-8?B?NWpTM3pEeXRNZnI5cDFoemlKdkJ5dmo2Q0N3Tk1IS3JzekE4TXMzd1kxS3NX?=
 =?utf-8?B?WnFqWTkveU0vYzdHRkxIdEptYnVSWkExZHBOS0N0WVlZbUU5WXFadHgrcHA5?=
 =?utf-8?B?TWxyTDYzUTQ5NDlickJOZG55bkdGeUNXZmRmTExERGU5Mi9OL3ZFQVNUNkJX?=
 =?utf-8?B?VU8zN1VITUNHT0MrcEMzWndORFY0VXJRb3dZOVhqRHVrSmc5cEpxVzgvK0dV?=
 =?utf-8?B?eXdmcnpKZnFWdUZ5MTIyd1RQYlQwZm5ra2FlZDhrQUtCZUV2L3FKL2FSWjZn?=
 =?utf-8?B?RWQ1M2RBQUU1QzR3UnltWE9ibjBnOTZKL3FCTllkOUpUOEg1WE9JMVp5NnR1?=
 =?utf-8?B?Wk1HeDN1Ujc3emlSOUhlbkR5RTNGVUZhMjBrNzRBS3IwcmRBRFJlelZuQWho?=
 =?utf-8?B?TmFiU2dWYm5zZ0ZHbmdZTWgrejh0cm5TZk96NDNvTWRJZ0dXZExwbHlYL2VQ?=
 =?utf-8?B?aWd4ODFORDJiUVRXemI5WEdGQ0RodEM2aHlYL1B0Q1FpVjN2UVNmQmlJTFhr?=
 =?utf-8?B?STZ2SjhMcHNjZHp3aVdPdmNVdTB2T0ZUL0RoODU3dGExYUdMNkhvejloZFA3?=
 =?utf-8?B?UHpJYkdTZGt2c1BoVTBycERiZnROMTlvQ2NYNVIyM2tZOUZveWRSUHFiTCsx?=
 =?utf-8?B?bitUSk9BMnpianltVzdFckh0c1U2cjlrNGpmeENmRHlYT29UcnBONTV0NCt4?=
 =?utf-8?B?STZ1djZQbkljM3piSFhyRGpRb0IvOThrNE5TNkNGcHdzUTZWckZ6SXdlU0RF?=
 =?utf-8?B?VVBTWmY4cDIrQ3Npem5tWlEzdFFwTXVINnNwSmRoME8wZ0V4WUJySXdZeVB5?=
 =?utf-8?B?bmhGWVZzTVhRMzRlNTVpVEJVdHI2OXU5SnBVRnFYOXFrS2xxWmwwd3lMekEy?=
 =?utf-8?B?dG5XRjUxZjFLU3NMQlZMUHVDNk4wT1JjaEswcm9lQ2J1eHlTYS9GNWx4MDNK?=
 =?utf-8?B?bzh1QnZIcHM5ZjV0TVRST1VnYVB2SWJROGMxVFNTQ2NSQks4VGRqVEpXa1h2?=
 =?utf-8?B?VnNWZUJtZXQrQzlreU5BNi9vUldNMmtPTHIrSURKNU5wQTZhWUJ6SkJXZDVH?=
 =?utf-8?B?K3VQZWtjYjIyMFFCQkE3V0cwaVZnRzlVbUR6VktYcDFNQ2wxYUMvS09rZUo0?=
 =?utf-8?B?WVJyeThzN0xVRWFRaGovNU91OU5oNmZVK0EvNklOeTA2QzNGZEpnN2lyaFMz?=
 =?utf-8?B?b0J4WWNIdDBUTnZXcE9iS25pZEt0NDNaNExzc3dZNHU5WW5JK2tZb29Od2Rz?=
 =?utf-8?B?ZXA4SStRc3FVN1hVNFc1Tk83OCtWMUJUaEtGMlVSZUNPM3RBeHJyUnNpVTFt?=
 =?utf-8?B?cG4xcTQxZzdLNVIyTy8rRmtyZXMzZ1NhMmQrRWtCNm4zWG4rejRGT3pKcjU4?=
 =?utf-8?B?MVAwOTVYS1c1VmxJd3JKNXJQeG1DSW9SRWQ2YXFOdlMyNHdYSWRLNXZPYUxX?=
 =?utf-8?B?V2p4clh5UElkTVVrdm9JYXhvaHZrY3JoOVpDNjlUam5nTmg3Z09wOUJmcjRh?=
 =?utf-8?B?SEphc2dabDFEamZoanUydlZ6b2RybCthaG5hdm9PNEt6U2tPZ0RscEorb0hs?=
 =?utf-8?B?ZCtyNTFMOEFWNHA2NGpwVWdMQTRRczM5SnZzOGV6OEd1dkJPbnRPNFJiVmlH?=
 =?utf-8?B?eVlObU1jTW53V2ExVG1TVUZXVHV3ZkZoTUVrRzJOZnQzc1Nnbzd4ekI0Mk5m?=
 =?utf-8?B?bjBwRHZaVENyVG56RTRXYmlDMHVRWWlqQUF2UTB4M3Q2bDUyaUQvdTUvemds?=
 =?utf-8?B?Z0U0ejNZbWVPNDBvQ2tSb0F6UEhmMER2SlUyUFdFT0JDYkFoeDFVRGdwdWpP?=
 =?utf-8?Q?a3optrhEVg/dqPzStdZxKSIAn?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 96275577-5444-41b5-b82e-08dc5e616eb6
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Apr 2024 22:06:14.3809
 (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: Zc+3t/d6bVAgdusxiZQDpVg3w78WPNnyzFdBxO9N/bbI/MRvi7rqWdLSDdBIzXDsi5CBcSQR5M07wT1bBLbrFA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7755
X-OriginatorOrg: intel.com


On 17/04/2024 4:28 am, Yamahata, Isaku wrote:
> On Tue, Apr 16, 2024 at 12:55:33PM +1200,
> "Huang, Kai" <kai.huang@intel.com> wrote:
> 
>>
>>
>> On 5/03/2024 9:21 pm, Isaku Yamahata wrote:
>>> On Fri, Mar 01, 2024 at 03:25:31PM +0800,
>>> Yan Zhao <yan.y.zhao@intel.com> wrote:
>>>
>>>>> + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is 1024B.
>>>>> + */
>>>>> +#define TDX_MAX_VCPUS	(~(u16)0)
>>>> This value will be treated as -1 in tdx_vm_init(),
>>>> 	"kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);"
>>>>
>>>> This will lead to kvm->max_vcpus being -1 by default.
>>>> Is this by design or just an error?
>>>> If it's by design, why not set kvm->max_vcpus = -1 in tdx_vm_init() directly.
>>>> If an unexpected error, may below is better?
>>>>
>>>> #define TDX_MAX_VCPUS   (int)((u16)(~0UL))
>>>> or
>>>> #define TDX_MAX_VCPUS 65536
>>>
>>> You're right. I'll use ((int)U16_MAX).
>>> As TDX 1.5 introduced metadata MAX_VCPUS_PER_TD, I'll update to get the value
>>> and trim it further. Something following.
>>>
>>
>> [...]
>>
>>> +	u16 max_vcpus_per_td;
>>> +
>>
>> [...]
>>
>>> -	kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);
>>> +	kvm->max_vcpus = min3(kvm->max_vcpus, tdx_info->max_vcpus_per_td,
>>> +			     TDX_MAX_VCPUS);
>>
>> [...]
>>
>>> -#define TDX_MAX_VCPUS	(~(u16)0)
>>> +#define TDX_MAX_VCPUS	((int)U16_MAX)
>>
>> Why do you even need TDX_MAX_VCPUS, given it cannot exceed U16_MAX and you
>> will have the 'u16 max_vcpus_per_td' anyway?
>>
>> IIUC, in KVM_ENABLE_CAP(KVM_CAP_MAX_VCPUS), we can overwrite the
>> kvm->max_vcpus to the 'max_vcpus' provided by the userspace, and make sure
>> it doesn't exceed tdx_info->max_vcpus_per_td.
>>
>> Anything I am missing?
> 
> With the latest TDX 1.5 module, we don't need TDX_MAX_VCPUS.
> 
> The metadata MD_FIELD_ID_MAX_VCPUS_PER_TD was introduced at the middle version
> of TDX 1.5. (I don't remember the exact version.), the logic was something
> like as follows.  Now if we fail to read the metadata, disable TDX.
> 
> read metadata MD_FIELD_ID_MAX_VCPUS_PER_TD;
> if success
>    tdx_info->max_vcpu_per_td = the value read metadata
> else
>    tdx_info->max_vcpu_per_td = TDX_MAX_VCPUS;
> 

OK.  But even the SEAMCALL can fail, we can just use U16_MAX directly 
when it fails given we can see clearly the type of max_vcpu_per_td is 'u16'.

if success
	tdx_info->max_vcpu_per_td = the value read metadata
else
	tdx_info->max_vcpu_per_td = U16_MAX;

So I don't see why TDX_MAX_VCPUS is needed (especially in tdx_arch.h as 
it is not an architectural value but just some value chosen for our 
convenience).