Received-SPF: pass (google.com: domain of linux-kernel+bounces-122290-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <b065cf99-74bc-42d1-95a3-8a0b018218ee@intel.com>
Date: Thu, 28 Mar 2024 09:36:21 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific
 parameters
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>,
 "Gao, Chao" <chao.gao@intel.com>, "Yamahata, Isaku"
 <isaku.yamahata@intel.com>
Cc: "Zhang, Tina" <tina.zhang@intel.com>,
 "isaku.yamahata@linux.intel.com" <isaku.yamahata@linux.intel.com>,
 "seanjc@google.com" <seanjc@google.com>, "Huang, Kai" <kai.huang@intel.com>,
 "sagis@google.com" <sagis@google.com>, "Chen, Bo2" <chen.bo@intel.com>,
 "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
 "Aktas, Erdem" <erdemaktas@google.com>,
 "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
 "pbonzini@redhat.com" <pbonzini@redhat.com>, "Yuan, Hang"
 <hang.yuan@intel.com>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <5eca97e6a3978cf4dcf1cff21be6ec8b639a66b9.1708933498.git.isaku.yamahata@intel.com>
 <Zfp+YWzHV0DxVf1+@chao-email>
 <20240321155513.GL1994522@ls.amr.corp.intel.com>
 <5470570d804b52dcf24b454d5fdfc2320f735e80.camel@intel.com>
Content-Language: en-US
From: Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <5470570d804b52dcf24b454d5fdfc2320f735e80.camel@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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@intel.com> 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.