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@xxxxxxxxx/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.
