Re: [PATCH v6 55/60] i386/tdx: Fetch and validate CPUID of TD guest

Xiaoyao Li
2025-01-14T13:04:01Z

On 12/13/2024 1:52 AM, Ira Weiny wrote:
> On Tue, Nov 05, 2024 at 01:24:03AM -0500, Xiaoyao Li wrote:
>> Use KVM_TDX_GET_CPUID to get the CPUIDs that are managed and enfored
>> by TDX module for TD guest. Check QEMU's configuration against the
>> fetched data.
>>
>> Print wanring message when 1. a feature is not supported but requested
>> by QEMU or 2. QEMU doesn't want to expose a feature while it is enforced
>> enabled.
>>
>> - If cpu->enforced_cpuid is not set, prints the warning message of both
>> 1) and 2) and tweak QEMU's configuration.
>>
>> - If cpu->enforced_cpuid is set, quit if any case of 1) or 2).
>
> Patches 52, 53, 54, and this one should probably be squashed
>
> 53's commit message is non-existent and really only makes sense because the
> function is used here. 52's commit message is pretty thin. Both 52 and 53 are
> used here, the size of this patch is not adversely affected, and the reason for
> the changes are more clearly shown in this patch.

It's my fault to forget adding the commit message for patch 53 before
posting.

> 54 somewhat stands on its own. But really it is just calling the functionality
> of this patch. So I don't see a big reason for it to be on its own but up to
> you.

I'll squash patch 52 and 53 into this one and leave patch 54 as-is.

>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> target/i386/kvm/tdx.c | 81 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 81 insertions(+)
>>
>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>> index e7e0f073dfc9..9cb099e160e4 100644
>> --- a/target/i386/kvm/tdx.c
>> +++ b/target/i386/kvm/tdx.c
>> @@ -673,6 +673,86 @@ static uint32_t tdx_adjust_cpuid_features(X86ConfidentialGuest *cg,
>> return value;
>> }
>>
>> +
>> +static void tdx_fetch_cpuid(CPUState *cpu, struct kvm_cpuid2 *fetch_cpuid)
>> +{
>> + int r;
>> +
>> + r = tdx_vcpu_ioctl(cpu, KVM_TDX_GET_CPUID, 0, fetch_cpuid);
>> + if (r) {
>> + error_report("KVM_TDX_GET_CPUID failed %s", strerror(-r));
>> + exit(1);
>> + }
>> +}
>> +
>> +static int tdx_check_features(X86ConfidentialGuest *cg, CPUState *cs)
>> +{
>> + uint64_t actual, requested, unavailable, forced_on;
>> + g_autofree struct kvm_cpuid2 *fetch_cpuid;
>> + const char *forced_on_prefix = NULL;
>> + const char *unav_prefix = NULL;
>> + struct kvm_cpuid_entry2 *entry;
>> + X86CPU *cpu = X86_CPU(cs);
>> + CPUX86State *env = &cpu->env;
>> + FeatureWordInfo *wi;
>> + FeatureWord w;
>> + bool mismatch = false;
>> +
>> + fetch_cpuid = g_malloc0(sizeof(*fetch_cpuid) +
>> + sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
>
> Is this a memory leak? I don't see fetch_cpuid returned or free'ed. If so, it
> might be better to use g_autofree() for this allocation.
>
> Alternatively, this allocation size is constant, could this be on the heap and
> not allocated at all? (I assume it is big enough that a stack allocation is
> unwanted.)
>
> Ira
>
>> + tdx_fetch_cpuid(cs, fetch_cpuid);
>> +
>> + if (cpu->check_cpuid || cpu->enforce_cpuid) {
>> + unav_prefix = "TDX doesn't support requested feature";
>> + forced_on_prefix = "TDX forcibly sets the feature";
>> + }
>> +
>> + for (w = 0; w < FEATURE_WORDS; w++) {
>> + wi = &feature_word_info[w];
>> + actual = 0;
>> +
>> + switch (wi->type) {
>> + case CPUID_FEATURE_WORD:
>> + entry = cpuid_find_entry(fetch_cpuid, wi->cpuid.eax, wi->cpuid.ecx);
>> + if (!entry) {
>> + /*
>> + * If KVM doesn't report it means it's totally configurable
>> + * by QEMU
>> + */
>> + continue;
>> + }
>> +
>> + actual = cpuid_entry_get_reg(entry, wi->cpuid.reg);
>> + break;
>> + case MSR_FEATURE_WORD:
>> + /*
>> + * TODO:
>> + * validate MSR features when KVM has interface report them.
>> + */
>> + continue;
>> + }
>> +
>> + requested = env->features[w];
>> + unavailable = requested & ~actual;
>> + mark_unavailable_features(cpu, w, unavailable, unav_prefix);
>> + if (unavailable) {
>> + mismatch = true;
>> + }
>> +
>> + forced_on = actual & ~requested;
>> + mark_forced_on_features(cpu, w, forced_on, forced_on_prefix);
>> + if (forced_on) {
>> + mismatch = true;
>> + }
>> + }
>> +
>> + if (cpu->enforce_cpuid && mismatch) {
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int tdx_validate_attributes(TdxGuest *tdx, Error **errp)
>> {
>> if ((tdx->attributes & ~tdx_caps->supported_attrs)) {
>> @@ -1019,4 +1099,5 @@ static void tdx_guest_class_init(ObjectClass *oc, void *data)
>> x86_klass->cpu_instance_init = tdx_cpu_instance_init;
>> x86_klass->cpu_realizefn = tdx_cpu_realizefn;
>> x86_klass->adjust_cpuid_features = tdx_adjust_cpuid_features;
>> + x86_klass->check_features = tdx_check_features;
>> }
>> --
>> 2.34.1
>>