On 12/4/2024 7:55 PM, Adrian Hunter wrote:
On 4/12/24 13:13, Chao Gao wrote:
On Wed, Dec 04, 2024 at 08:57:23AM +0200, Adrian Hunter wrote:
On 4/12/24 08:37, Chao Gao wrote:
On Wed, Dec 04, 2024 at 08:18:32AM +0200, Adrian Hunter wrote:
On 4/12/24 03:25, Chao Gao wrote:
+#define TDX_FEATURE_TSX (__feature_bit(X86_FEATURE_HLE) | __feature_bit(X86_FEATURE_RTM))
+
+static bool has_tsx(const struct kvm_cpuid_entry2 *entry)
+{
+ return entry->function == 7 && entry->index == 0 &&
+ (entry->ebx & TDX_FEATURE_TSX);
+}
+
+static void clear_tsx(struct kvm_cpuid_entry2 *entry)
+{
+ entry->ebx &= ~TDX_FEATURE_TSX;
+}
+
+static bool has_waitpkg(const struct kvm_cpuid_entry2 *entry)
+{
+ return entry->function == 7 && entry->index == 0 &&
+ (entry->ecx & __feature_bit(X86_FEATURE_WAITPKG));
+}
+
+static void clear_waitpkg(struct kvm_cpuid_entry2 *entry)
+{
+ entry->ecx &= ~__feature_bit(X86_FEATURE_WAITPKG);
+}
+
+static void tdx_clear_unsupported_cpuid(struct kvm_cpuid_entry2 *entry)
+{
+ if (has_tsx(entry))
+ clear_tsx(entry);
+
+ if (has_waitpkg(entry))
+ clear_waitpkg(entry);
+}
+
+static bool tdx_unsupported_cpuid(const struct kvm_cpuid_entry2 *entry)
+{
+ return has_tsx(entry) || has_waitpkg(entry);
+}
No need to check TSX/WAITPKG explicitly because setup_tdparams_cpuids() already
ensures that unconfigurable bits are not set by userspace.
Aren't they configurable?
They are cleared from the configurable bitmap by tdx_clear_unsupported_cpuid(),
so they are not configurable from a userspace perspective. Did I miss anything?
KVM should check user inputs against its adjusted configurable bitmap, right?
Maybe I misunderstand but we rely on the TDX module to reject
invalid configuration. We don't check exactly what is configurable
for the TDX Module.
Ok, this is what I missed. I thought KVM validated user input and masked
out all unsupported features. sorry for this.
TSX and WAITPKG are not invalid for the TDX Module, but KVM
must either support them by restoring their MSRs, or disallow
them. This patch disallows them for now.
Yes. I agree. what if a new feature (supported by a future TDX module) also
needs KVM to restore some MSRs? current KVM will allow it to be exposed (since
only TSX/WAITPKG are checked); then some MSRs may get corrupted. I may think
this is not a good design. Current KVM should work with future TDX modules.
With respect to CPUID, I gather this kind of thing has been
discussed, such as here:
https://lore.kernel.org/all/ZhVsHVqaff7AKagu@xxxxxxxxxx/
and Rick and Xiaoyao are working on something.
In general, I would expect a new TDX Module would advertise support for
new features, but KVM would have to opt in to use them.
There were discussion[1] on whether KVM to gatekeep the
configurable/supported CPUIDs for TDX. I stand by Sean that KVM needs to
do so.
Regarding KVM opt in the new feature, KVM gatekeeps the CPUID bit that
can be set by userspace is exactly the behavior of opt-in. i.e., for a
given KVM, it only allows a CPUID set {S} to be configured by userspace,
if new TDX module supports new feature X, it needs KVM to opt-in X by
adding X to {S} so that X is allowed to be configured by userspace.
Besides, I find current interface between KVM and userspace lacks the
ability to tell userspace what bits are not supported by KVM.
KVM_TDX_CAPABILITIES.cpuid doesn't work because it represents the
configurable CPUIDs, not supported CPUIDs (I think we might rename it to
configurable_cpuid to better reflect its meaning). So userspace has to
hardcode that TSX and WAITPKG is not support itself.
[1] https://lore.kernel.org/all/ZuM12EFbOXmpHHVQ@xxxxxxxxxx/
