lkml.org 
[lkml]   [2023]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v8 06/16] x86/virt/tdx: Get information about TDX module and TDX-capable memory
Date
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@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
^
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@intel.com/

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.
\
 
 \ /
  Last update: 2023-03-26 23:34    [W:0.115 / U:0.648 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site