Re: [PATCH v8 06/16] x86/virt/tdx: Get information about TDX module and TDX-capable memory

From: Dave Hansen <dave.hansen@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "Luck, Tony" <tony.luck@intel.com>,
	"bagasdotme@gmail.com" <bagasdotme@gmail.com>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"Shahar, Sagi" <sagis@google.com>,
	"imammedo@redhat.com" <imammedo@redhat.com>,
	"Gao, Chao" <chao.gao@intel.com>,
	"Brown, Len" <len.brown@intel.com>,
	"sathyanarayanan.kuppuswamy@linux.intel.com" 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: Re: [PATCH v8 06/16] x86/virt/tdx: Get information about TDX module and TDX-capable memory
Date: Mon, 9 Jan 2023 11:52:16 -0800	[thread overview]
Message-ID: <bea7dbd0-f6ec-5103-9d00-9617154d6be9@intel.com> (raw)
In-Reply-To: <e605ce95f1b92fae630bf6abb801774bc28d8072.camel@intel.com>

On 1/9/23 02:25, Huang, Kai wrote:
> On Fri, 2023-01-06 at 09:46 -0800, Dave Hansen wrote:
...
>>> Note not all members in the 1024 bytes TDX module information are used
>>> (even by the KVM).
>>
>> I'm not sure what this has to do with anything.
> 
> You mentioned in v7 that:
>>>> This is also a great place to mention that the tdsysinfo_struct
contains
>>> a *lot* of gunk which will not be used for a bit or that may never get
>>> used.
> 
> https://lore.kernel.org/linux-mm/cc195eb6499cf021b4ce2e937200571915bfe66f.camel@intel.com/T/#m168e619aac945fa418ccb1d6652113003243d895
> 
> Perhaps I misunderstood something but I was trying to address this.
> 
> Should I remove this sentence?

If someone goes looking at this patch, the see tdsysinfo_struct with
something like two dozen defined fields.  But, very few of them get used
in this patch.  Why?  Just saying that they are unused is a bit silly.

	The 'tdsysinfo_struct' is fairly large (1k) and contains a lot
	of info about the TD.  Fully define the entire structure, but
	only use the fields necessary to build the PAMT and TDMRs and
	pr_info() some basics about the module.

	The rest of the fields will get used... (by kvm?  never??)

...
>>> +	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
>>> +	int ret;
>>> +
>>> +	ret = tdx_get_sysinfo(sysinfo, cmr_array);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>>  	/*
>>>  	 * TODO:
>>>  	 *
>>> -	 *  - Get TDX module information and TDX-capable memory regions.
>>>  	 *  - Build the list of TDX-usable memory regions.
>>>  	 *  - Construct a list of TDMRs to cover all TDX-usable memory
>>>  	 *    regions.
>>> @@ -166,7 +239,9 @@ static int init_tdx_module(void)
>>>  	 *
>>>  	 *  Return error before all steps are done.
>>>  	 */
>>> -	return -EINVAL;
>>> +	ret = -EINVAL;
>>> +out:
>>> +	return ret;
>>>  }
>>
>> I'm going to be lazy and not look into the future.  But, you don't need
>> the "out:" label here, yet.  It doesn'serve any purpose like this, so
>> why introduce it here?
> 
> The 'out' label is here because of below code:
> 
> 	ret = tdx_get_sysinfo(...);
> 	if (ret)
> 		goto out;
> 
> If I don't have 'out' label here in this patch, do you mean something below?
> 
> 	ret = tdx_get_sysinfo(...);
> 	if (ret)
> 		return ret;
> 
> 	/*
> 	 * TODO:
> 	 * ...
> 	 * Return error before all steps are done.
> 	 */
> 	return -EINVAL;

Yes, if you remove the 'out:' label like you've shown in your reply,
it's actually _less_ code.  The labels are really only necessary when
you have common work to "undo" something before returning from the
function.  Here, you can just return.