| Message ID | 72b528863d14b322df553efa285ef4637ee67581.1708933498.git.isaku.yamahata@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v19,001/130] x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro | expand |
On 2/26/2024 4:25 PM, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > To read meta data in series, use table. > Instead of metadata_read(fid0, &data0); metadata_read(...); ... > table = { {fid0, &data0}, ...}; metadata-read(tables). > TODO: Once the TDX host code introduces its framework to read TDX metadata, > drop this patch and convert the code that uses this. Do you mean the patch 1-5 included in this patch set. I think the patch 1-5 of this patch set is doing this thing, right? Since they are already there, I think you can use them directly in this patch set instead of introducing these temp code? > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > v18: > - newly added > --- > arch/x86/kvm/vmx/tdx.c | 45 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > index cde971122c1e..dce21f675155 100644 > --- a/arch/x86/kvm/vmx/tdx.c > +++ b/arch/x86/kvm/vmx/tdx.c > @@ -6,6 +6,7 @@ > #include "capabilities.h" > #include "x86_ops.h" > #include "x86.h" > +#include "tdx_arch.h" > #include "tdx.h" > > #undef pr_fmt > @@ -39,6 +40,50 @@ static void __used tdx_guest_keyid_free(int keyid) > ida_free(&tdx_guest_keyid_pool, keyid); > } > > +#define TDX_MD_MAP(_fid, _ptr) \ > + { .fid = MD_FIELD_ID_##_fid, \ > + .ptr = (_ptr), } > + > +struct tdx_md_map { > + u64 fid; > + void *ptr; > +}; > + > +static size_t tdx_md_element_size(u64 fid) > +{ > + switch (TDX_MD_ELEMENT_SIZE_CODE(fid)) { > + case TDX_MD_ELEMENT_SIZE_8BITS: > + return 1; > + case TDX_MD_ELEMENT_SIZE_16BITS: > + return 2; > + case TDX_MD_ELEMENT_SIZE_32BITS: > + return 4; > + case TDX_MD_ELEMENT_SIZE_64BITS: > + return 8; > + default: > + WARN_ON_ONCE(1); > + return 0; > + } > +} > + > +static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps) > +{ > + struct tdx_md_map *m; > + int ret, i; > + u64 tmp; > + > + for (i = 0; i < nr_maps; i++) { > + m = &maps[i]; > + ret = tdx_sys_metadata_field_read(m->fid, &tmp); > + if (ret) > + return ret; > + > + memcpy(m->ptr, &tmp, tdx_md_element_size(m->fid)); > + } > + > + return 0; > +} > + > static int __init tdx_module_setup(void) > { > int ret;
On 3/14/2024 5:17 PM, Binbin Wu wrote: > > > On 2/26/2024 4:25 PM, isaku.yamahata@intel.com wrote: >> From: Isaku Yamahata <isaku.yamahata@intel.com> >> >> To read meta data in series, use table. >> Instead of metadata_read(fid0, &data0); metadata_read(...); ... >> table = { {fid0, &data0}, ...}; metadata-read(tables). >> TODO: Once the TDX host code introduces its framework to read TDX >> metadata, >> drop this patch and convert the code that uses this. > > Do you mean the patch 1-5 included in this patch set. > I think the patch 1-5 of this patch set is doing this thing, right? > > Since they are already there, I think you can use them directly in this > patch set instead of introducing these temp code? I may have some mis-understanding, but I think the TODO has been done, right? > >> >> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> >> --- >> v18: >> - newly added >> --- >> arch/x86/kvm/vmx/tdx.c | 45 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 45 insertions(+) >> >> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c >> index cde971122c1e..dce21f675155 100644 >> --- a/arch/x86/kvm/vmx/tdx.c >> +++ b/arch/x86/kvm/vmx/tdx.c >> @@ -6,6 +6,7 @@ >> #include "capabilities.h" >> #include "x86_ops.h" >> #include "x86.h" >> +#include "tdx_arch.h" >> #include "tdx.h" >> #undef pr_fmt >> @@ -39,6 +40,50 @@ static void __used tdx_guest_keyid_free(int keyid) >> ida_free(&tdx_guest_keyid_pool, keyid); >> } >> +#define TDX_MD_MAP(_fid, _ptr) \ >> + { .fid = MD_FIELD_ID_##_fid, \ >> + .ptr = (_ptr), } >> + >> +struct tdx_md_map { >> + u64 fid; >> + void *ptr; >> +}; >> + >> +static size_t tdx_md_element_size(u64 fid) >> +{ >> + switch (TDX_MD_ELEMENT_SIZE_CODE(fid)) { >> + case TDX_MD_ELEMENT_SIZE_8BITS: >> + return 1; >> + case TDX_MD_ELEMENT_SIZE_16BITS: >> + return 2; >> + case TDX_MD_ELEMENT_SIZE_32BITS: >> + return 4; >> + case TDX_MD_ELEMENT_SIZE_64BITS: >> + return 8; >> + default: >> + WARN_ON_ONCE(1); >> + return 0; >> + } >> +} >> + >> +static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps) >> +{ >> + struct tdx_md_map *m; >> + int ret, i; >> + u64 tmp; >> + >> + for (i = 0; i < nr_maps; i++) { >> + m = &maps[i]; >> + ret = tdx_sys_metadata_field_read(m->fid, &tmp); >> + if (ret) >> + return ret; >> + >> + memcpy(m->ptr, &tmp, tdx_md_element_size(m->fid)); >> + } >> + >> + return 0; >> +} >> + >> static int __init tdx_module_setup(void) >> { >> int ret; >
On Thu, Mar 14, 2024 at 10:35:47PM +0800, Binbin Wu <binbin.wu@linux.intel.com> wrote: > > > On 3/14/2024 5:17 PM, Binbin Wu wrote: > > > > > > On 2/26/2024 4:25 PM, isaku.yamahata@intel.com wrote: > > > From: Isaku Yamahata <isaku.yamahata@intel.com> > > > > > > To read meta data in series, use table. > > > Instead of metadata_read(fid0, &data0); metadata_read(...); ... > > > table = { {fid0, &data0}, ...}; metadata-read(tables). > > > TODO: Once the TDX host code introduces its framework to read TDX > > > metadata, > > > drop this patch and convert the code that uses this. > > > > Do you mean the patch 1-5 included in this patch set. > > I think the patch 1-5 of this patch set is doing this thing, right? > > > > Since they are already there, I think you can use them directly in this > > patch set instead of introducing these temp code? > I may have some mis-understanding, but I think the TODO has been done, > right? I meant the following patch series. https://lore.kernel.org/kvm/cover.1709288433.git.kai.huang@intel.com/ If (the future version of) the patch series doesn't provide a way to read one metadata with size, we need to keep this patch.
On 26/02/2024 9:25 pm, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > To read meta data in series, use table. > Instead of metadata_read(fid0, &data0); metadata_read(...); ... > table = { {fid0, &data0}, ...}; metadata-read(tables). This explains nothing why the code introduced in patch 5 cannot be used. > TODO: Once the TDX host code introduces its framework to read TDX metadata, > drop this patch and convert the code that uses this. Seriously, what is this?? Please treat your patches as "official" patches. > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > v18: > - newly added > --- > arch/x86/kvm/vmx/tdx.c | 45 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > index cde971122c1e..dce21f675155 100644 > --- a/arch/x86/kvm/vmx/tdx.c > +++ b/arch/x86/kvm/vmx/tdx.c > @@ -6,6 +6,7 @@ > #include "capabilities.h" > #include "x86_ops.h" > #include "x86.h" > +#include "tdx_arch.h" > #include "tdx.h" > > #undef pr_fmt > @@ -39,6 +40,50 @@ static void __used tdx_guest_keyid_free(int keyid) > ida_free(&tdx_guest_keyid_pool, keyid); > } > > +#define TDX_MD_MAP(_fid, _ptr) \ > + { .fid = MD_FIELD_ID_##_fid, \ > + .ptr = (_ptr), } > + > +struct tdx_md_map { > + u64 fid; > + void *ptr; > +}; > + > +static size_t tdx_md_element_size(u64 fid) > +{ > + switch (TDX_MD_ELEMENT_SIZE_CODE(fid)) { > + case TDX_MD_ELEMENT_SIZE_8BITS: > + return 1; > + case TDX_MD_ELEMENT_SIZE_16BITS: > + return 2; > + case TDX_MD_ELEMENT_SIZE_32BITS: > + return 4; > + case TDX_MD_ELEMENT_SIZE_64BITS: > + return 8; > + default: > + WARN_ON_ONCE(1); > + return 0; > + } > +} > + > +static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps) > +{ > + struct tdx_md_map *m; > + int ret, i; > + u64 tmp; > + > + for (i = 0; i < nr_maps; i++) { > + m = &maps[i]; > + ret = tdx_sys_metadata_field_read(m->fid, &tmp); > + if (ret) > + return ret; > + > + memcpy(m->ptr, &tmp, tdx_md_element_size(m->fid)); > + } > + > + return 0; > +} > + It's just insane to have two duplicated mechanism for metadata reading. This will only confuse people. If there's anything missing in patch 1-5, we can enhance them.
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index cde971122c1e..dce21f675155 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -6,6 +6,7 @@ #include "capabilities.h" #include "x86_ops.h" #include "x86.h" +#include "tdx_arch.h" #include "tdx.h" #undef pr_fmt @@ -39,6 +40,50 @@ static void __used tdx_guest_keyid_free(int keyid) ida_free(&tdx_guest_keyid_pool, keyid); } +#define TDX_MD_MAP(_fid, _ptr) \ + { .fid = MD_FIELD_ID_##_fid, \ + .ptr = (_ptr), } + +struct tdx_md_map { + u64 fid; + void *ptr; +}; + +static size_t tdx_md_element_size(u64 fid) +{ + switch (TDX_MD_ELEMENT_SIZE_CODE(fid)) { + case TDX_MD_ELEMENT_SIZE_8BITS: + return 1; + case TDX_MD_ELEMENT_SIZE_16BITS: + return 2; + case TDX_MD_ELEMENT_SIZE_32BITS: + return 4; + case TDX_MD_ELEMENT_SIZE_64BITS: + return 8; + default: + WARN_ON_ONCE(1); + return 0; + } +} + +static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps) +{ + struct tdx_md_map *m; + int ret, i; + u64 tmp; + + for (i = 0; i < nr_maps; i++) { + m = &maps[i]; + ret = tdx_sys_metadata_field_read(m->fid, &tmp); + if (ret) + return ret; + + memcpy(m->ptr, &tmp, tdx_md_element_size(m->fid)); + } + + return 0; +} + static int __init tdx_module_setup(void) { int ret;