| Message ID | 41cd371d8a9caadf183e3ab464c57f9f715184d3.1709288433.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host: Provide TDX module metadata reading APIs | expand |
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote: > The kernel reads all TDMR related global metadata fields based on a > table which maps the metadata fields to the corresponding members of > 'struct tdx_tdmr_sysinfo'. > > Currently this table is a static variable. But this table is only used > by the function which reads these metadata fields and becomes useless > after reading is done. > > Change the table to function local variable. This also saves the > storage of the table from the kernel image. It seems like a reasonable change, but I don't see how it helps the purpose of this series. It seems more like generic cleanup. Can you explain?
On 3/05/2024 12:09 pm, Edgecombe, Rick P wrote: > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote: >> The kernel reads all TDMR related global metadata fields based on a >> table which maps the metadata fields to the corresponding members of >> 'struct tdx_tdmr_sysinfo'. >> >> Currently this table is a static variable. But this table is only used >> by the function which reads these metadata fields and becomes useless >> after reading is done. >> >> Change the table to function local variable. This also saves the >> storage of the table from the kernel image. > > It seems like a reasonable change, but I don't see how it helps the purpose of > this series. It seems more like generic cleanup. Can you explain? It doesn't help KVM from exporting API's perspective. I just uses this series for some small improvement (that I believe) of the current code too. I can certainly drop this if you don't want it, but it's just a small change and I don't see the benefit of sending it out separately.
On Fri, 2024-05-03 at 12:18 +1200, Huang, Kai wrote: > > > On 3/05/2024 12:09 pm, Edgecombe, Rick P wrote: > > On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote: > > > The kernel reads all TDMR related global metadata fields based on a > > > table which maps the metadata fields to the corresponding members of > > > 'struct tdx_tdmr_sysinfo'. > > > > > > Currently this table is a static variable. But this table is only used > > > by the function which reads these metadata fields and becomes useless > > > after reading is done. > > > > > > Change the table to function local variable. This also saves the > > > storage of the table from the kernel image. > > > > It seems like a reasonable change, but I don't see how it helps the purpose > > of > > this series. It seems more like generic cleanup. Can you explain? > > It doesn't help KVM from exporting API's perspective. > > I just uses this series for some small improvement (that I believe) of > the current code too. > > I can certainly drop this if you don't want it, but it's just a small > change and I don't see the benefit of sending it out separately. The change makes sense to me by itself, but it needs to be called out as unrelated cleanup. Otherwise it will be confusing to anyone reviewing this from the perspective of something KVM TDX needs. Don't have a super strong opinion. But given the choice, I would prefer it gets separated, because to me it's a lower priority then the rest (which is high).
On 3/05/2024 12:29 pm, Edgecombe, Rick P wrote: > On Fri, 2024-05-03 at 12:18 +1200, Huang, Kai wrote: >> >> >> On 3/05/2024 12:09 pm, Edgecombe, Rick P wrote: >>> On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote: >>>> The kernel reads all TDMR related global metadata fields based on a >>>> table which maps the metadata fields to the corresponding members of >>>> 'struct tdx_tdmr_sysinfo'. >>>> >>>> Currently this table is a static variable. But this table is only used >>>> by the function which reads these metadata fields and becomes useless >>>> after reading is done. >>>> >>>> Change the table to function local variable. This also saves the >>>> storage of the table from the kernel image. >>> >>> It seems like a reasonable change, but I don't see how it helps the purpose >>> of >>> this series. It seems more like generic cleanup. Can you explain? >> >> It doesn't help KVM from exporting API's perspective. >> >> I just uses this series for some small improvement (that I believe) of >> the current code too. >> >> I can certainly drop this if you don't want it, but it's just a small >> change and I don't see the benefit of sending it out separately. > > The change makes sense to me by itself, but it needs to be called out as > unrelated cleanup. Otherwise it will be confusing to anyone reviewing this from > the perspective of something KVM TDX needs. > > Don't have a super strong opinion. But given the choice, I would prefer it gets > separated, because to me it's a lower priority then the rest (which is high). OK I'll drop this one.
On 3/1/24 03:20, Kai Huang wrote: > The kernel reads all TDMR related global metadata fields based on a > table which maps the metadata fields to the corresponding members of > 'struct tdx_tdmr_sysinfo'. > > Currently this table is a static variable. But this table is only used > by the function which reads these metadata fields and becomes useless > after reading is done. Is this intended to be a problem statement? _How_ is this a problem? > Change the table to function local variable. This also saves the > storage of the table from the kernel image. I'm confused how this would happen. Could you please explain your logic a bit here?
On Fri, 2024-05-03 at 09:01 -0700, Dave Hansen wrote: > On 3/1/24 03:20, Kai Huang wrote: > > The kernel reads all TDMR related global metadata fields based on a > > table which maps the metadata fields to the corresponding members of > > 'struct tdx_tdmr_sysinfo'. > > > > Currently this table is a static variable. But this table is only used > > by the function which reads these metadata fields and becomes useless > > after reading is done. > > Is this intended to be a problem statement? _How_ is this a problem? > > > Change the table to function local variable. This also saves the > > storage of the table from the kernel image. > > I'm confused how this would happen. Could you please explain your logic > a bit here? I think I failed to notice one thing, that although this patch can eliminate the (static) @fields[] array in the data section, it generates more code in the function get_tdx_tdmr_sysinfo() in order to build the same array in the stack. I did experiment and compared the generated code with or without the code change in this patch: before: fields: .quad -7998392933915033592 /* metadata field ID */ .long 0 .zero 4 .quad -7998392933915033591 .long 2 .zero 4 .quad -7998392933915033584 .long 4 .zero 4 .quad -7998392933915033583 .long 6 .zero 4 .quad -7998392933915033582 .long 8 .zero 4 get_tdx_tdmr_sysinfo: pushq %rbp movq %rsp, %rbp subq $24, %rsp movq %rdi, -24(%rbp) movl $0, -4(%rbp) jmp .L8 ...... after: get_tdx_tdmr_sysinfo: pushq %rbp movq %rsp, %rbp subq $112, %rsp movq %rdi, -104(%rbp) movabsq $-7998392933915033592, %rax movq %rax, -96(%rbp) movl $0, -88(%rbp) movabsq $-7998392933915033591, %rax movq %rax, -80(%rbp) movl $2, -72(%rbp) movabsq $-7998392933915033584, %rax movq %rax, -64(%rbp) movl $4, -56(%rbp) movabsq $-7998392933915033583, %rax movq %rax, -48(%rbp) movl $6, -40(%rbp) movabsq $-7998392933915033582, %rax movq %rax, -32(%rbp) movl $8, -24(%rbp) movl $0, -4(%rbp) jmp .L8 ...... So looks we cannot assume moving the static array to function local variable can always save the storage. I think the point is the compiler has to keep those constants (metadata field ID and offset) somewhere in the object file, no matter they are in the data section or in the code in text section, and no matter how does the compiler generate the code. The more reasonable benefit of this patch is to make the name scope of the @fields[] array be only visible in the get_tdx_tdmr_sysinfo() but not the entire file. Thanks for the insight. I hope the above is all I missed, or am I still missing anything? Anyway as replied to Rick I'll drop this patch from this series.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 2aee64d2f27f..cdcb3332bc5d 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -301,17 +301,16 @@ struct field_mapping { { .field_id = MD_FIELD_ID_##_field_id, \ .offset = offsetof(struct tdx_tdmr_sysinfo, _member) } -/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ -static const struct field_mapping fields[] = { - TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs), - TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr), - TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]), - TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]), - TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]), -}; - static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo) { + /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */ + const struct field_mapping fields[] = { + TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs), + TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr), + TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]), + TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]), + TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]), + }; int ret; int i;