Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific parameters

[Xiaoyao Li]

On 3/28/2024 9:12 AM, Edgecombe, Rick P wrote:
> On Thu, 2024-03-21 at 08:55 -0700, Isaku Yamahata wrote:
> > On Wed, Mar 20, 2024 at 02:12:49PM +0800,
> > Chao Gao <chao.gao@xxxxxxxxx> wrote:
> >
> > > > +static void setup_tdparams_cpuids(struct kvm_cpuid2 *cpuid,
> > > > +                                 struct td_params *td_params)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       /*
> > > > +        * td_params.cpuid_values: The number and the order of cpuid_value must
> > > > +        * be same to the one of struct tdsysinfo.{num_cpuid_config, cpuid_configs}
> > > > +        * It's assumed that td_params was zeroed.
> > > > +        */
> > > > +       for (i = 0; i < tdx_info->num_cpuid_config; i++) {
> > > > +               const struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
> > > > +               /* KVM_TDX_CPUID_NO_SUBLEAF means index = 0. */
> > > > +               u32 index = c->sub_leaf == KVM_TDX_CPUID_NO_SUBLEAF ? 0 : c->sub_leaf;
> > > > +               const struct kvm_cpuid_entry2 *entry =
> > > > +                       kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent,
> > > > +                                             c->leaf, index);
> > > > +               struct tdx_cpuid_value *value = &td_params->cpuid_values[i];
> > > > +
> > > > +               if (!entry)
> > > > +                       continue;
> > > > +
> > > > +               /*
> > > > +                * tdsysinfo.cpuid_configs[].{eax, ebx, ecx, edx}
> > > > +                * bit 1 means it can be configured to zero or one.
> > > > +                * bit 0 means it must be zero.
> > > > +                * Mask out non-configurable bits.
> > > > +                */
> > > > +               value->eax = entry->eax & c->eax;
> > > > +               value->ebx = entry->ebx & c->ebx;
> > > > +               value->ecx = entry->ecx & c->ecx;
> > > > +               value->edx = entry->edx & c->edx;
> > >
> > > Any reason to mask off non-configurable bits rather than return an error? this
> > > is misleading to userspace because guest sees the values emulated by TDX module
> > > instead of the values passed from userspace (i.e., the request from userspace
> > > isn't done but there is no indication of that to userspace).
> >
> > Ok, I'll eliminate them.  If user space passes wrong cpuids, TDX module will
> > return error. I'll leave the error check to the TDX module.
>
> I was just looking at this. Agreed. It breaks the selftests though.

If all you prefer to go this direction, then please update the error 
handling of this specific SEAMCALL.