On 17/04/2024 4:28 am, Yamahata, Isaku wrote:
On Tue, Apr 16, 2024 at 12:55:33PM +1200,
"Huang, Kai" <kai.huang@xxxxxxxxx> 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@xxxxxxxxx> 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).
