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

[Ira Weiny <ira.weiny@intel.com>]

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.

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.

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