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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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
>>