On Mon, 2023-01-09 at 11:52 -0800, Hansen, Dave wrote: > 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 ^ should be: "about the TDX module"? > only use the fields necessary to build the PAMT and TDMRs and > pr_info() some basics about the module. Above looks great! Thanks. > > The rest of the fields will get used... (by kvm? never??) The current KVM TDX support series uses majority of the rest fields: https://lore.kernel.org/lkml/99e5fcf2a7127347816982355fd4141ee1038a54.1667110240.git.isaku.yamahata@xxxxxxxxx/ Only one field isn't used, but I don't want to assume it won't be used forever, so I think "The rest of the fields will get used by KVM." is good enough. > > ... > > > > + 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. > Thanks will do. I think this applies to construct_tdmrs() too (patch 09 - 11). I'll check that part too based on your above idea.
