diff mbox series

[1/5] x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro

Message ID a55e86430c81274af86d2d1c23cdce2f53fef7d6.1709288433.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series TDX host: Provide TDX module metadata reading APIs | expand

Commit Message

Huang, Kai March 1, 2024, 11:20 a.m. UTC 
TD_SYSINFO_MAP() macro actually takes the member of the 'struct
tdx_tdmr_sysinfo' as the second argument and uses the offsetof() to
calculate the offset for that member.

Rename the macro argument _offset to _member to reflect this.

Signed-off-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/virt/vmx/tdx/tdx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Edgecombe, Rick P May 3, 2024, 12:07 a.m. UTC | #1 
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> TD_SYSINFO_MAP() macro actually takes the member of the 'struct
> tdx_tdmr_sysinfo' as the second argument and uses the offsetof() to
> calculate the offset for that member.
> 
> Rename the macro argument _offset to _member to reflect this.

The KVM patches will want to use this macro. The fact that it is misnamed will
percolate into the KVM code if it is not updated before it gets wider callers.
(This is a reason why this is good change from KVM's perspective).

See the KVM code below:

#define TDX_INFO_MAP(_field_id, _member)		\
	TD_SYSINFO_MAP(_field_id, struct st, _member)

	struct tdx_metadata_field_mapping st_fields[] = {
		TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
		TDX_INFO_MAP(TDCS_BASE_SIZE, tdcs_base_size),
		TDX_INFO_MAP(TDVPS_BASE_SIZE, tdvps_base_size),
	};
#undef TDX_INFO_MAP

#define TDX_INFO_MAP(_field_id, _member)			\
	TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)

	struct tdx_metadata_field_mapping fields[] = {
		TDX_INFO_MAP(FEATURES0, features0),
		TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
		TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
		TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
		TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
	};
#undef TDX_INFO_MAP
Huang, Kai May 6, 2024, 11:34 a.m. UTC | #2 
On Fri, 2024-05-03 at 00:07 +0000, Edgecombe, Rick P wrote:
> On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
> > TD_SYSINFO_MAP() macro actually takes the member of the 'struct
> > tdx_tdmr_sysinfo' as the second argument and uses the offsetof() to
> > calculate the offset for that member.
> > 
> > Rename the macro argument _offset to _member to reflect this.
> 
> The KVM patches will want to use this macro. The fact that it is misnamed will
> percolate into the KVM code if it is not updated before it gets wider callers.
> (This is a reason why this is good change from KVM's perspective).
> 
> See the KVM code below:
> 
> #define TDX_INFO_MAP(_field_id, _member)		\
> 	TD_SYSINFO_MAP(_field_id, struct st, _member)
> 
> 	struct tdx_metadata_field_mapping st_fields[] = {
> 		TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
> 		TDX_INFO_MAP(TDCS_BASE_SIZE, tdcs_base_size),
> 		TDX_INFO_MAP(TDVPS_BASE_SIZE, tdvps_base_size),
> 	};
> #undef TDX_INFO_MAP
> 
> #define TDX_INFO_MAP(_field_id, _member)			\
> 	TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
> 
> 	struct tdx_metadata_field_mapping fields[] = {
> 		TDX_INFO_MAP(FEATURES0, features0),
> 		TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
> 		TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
> 		TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
> 		TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
> 	};
> #undef TDX_INFO_MAP

I was thinking how to respond.  I guess your point is we can also mention
KVM will need to use this too so it's better to change it before it gets
wider callers.  But I don't think it is needed because if it is misnamed
now then we already have a justification to do it.

And technically, I don't think the argument name used in KVM actually has
anything to do with the argument name used in the TD_SYSINFO_MAP() macro
definition here.  What really matters is when they get used, we need to
pass the "real struct member": 

	struct whatever {
		u64 a;
		u16 b;
	};

	#define TDX_INFO_MAP_WHATEVER(_field_id, _xyz)	\
		TD_SYSINFO_MAP(_field_id, struct whatever, _xyz)

	const struct tdx_metadata_field_mapping fields[] = {
		TDX_INFO_MAP_WHATEVER(_FIELD_A, a),
		TDX_INFO_MAP_WHATEVER(_FIELD_B, b),
	};

No?
diff mbox series

Patch

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4d6826a76f78..2aee64d2f27f 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -297,9 +297,9 @@  struct field_mapping {
 	int offset;
 };
 
-#define TD_SYSINFO_MAP(_field_id, _offset) \
+#define TD_SYSINFO_MAP(_field_id, _member) \
 	{ .field_id = MD_FIELD_ID_##_field_id,	   \
-	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _offset) }
+	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _member) }
 
 /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
 static const struct field_mapping fields[] = {