Messages in this thread | | | Date | Fri, 22 Mar 2024 10:57:53 +1300 | Subject | Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural definitions | From | "Huang, Kai" <> |
| |
> +/* > + * TDX SEAMCALL API function leaves > + */ > +#define TDH_VP_ENTER 0 > +#define TDH_MNG_ADDCX 1 > +#define TDH_MEM_PAGE_ADD 2 > +#define TDH_MEM_SEPT_ADD 3 > +#define TDH_VP_ADDCX 4 > +#define TDH_MEM_PAGE_RELOCATE 5
I don't think the "RELOCATE" is needed in this patchset?
> +#define TDH_MEM_PAGE_AUG 6 > +#define TDH_MEM_RANGE_BLOCK 7 > +#define TDH_MNG_KEY_CONFIG 8 > +#define TDH_MNG_CREATE 9 > +#define TDH_VP_CREATE 10 > +#define TDH_MNG_RD 11 > +#define TDH_MR_EXTEND 16 > +#define TDH_MR_FINALIZE 17 > +#define TDH_VP_FLUSH 18 > +#define TDH_MNG_VPFLUSHDONE 19 > +#define TDH_MNG_KEY_FREEID 20 > +#define TDH_MNG_INIT 21 > +#define TDH_VP_INIT 22 > +#define TDH_MEM_SEPT_RD 25 > +#define TDH_VP_RD 26 > +#define TDH_MNG_KEY_RECLAIMID 27 > +#define TDH_PHYMEM_PAGE_RECLAIM 28 > +#define TDH_MEM_PAGE_REMOVE 29 > +#define TDH_MEM_SEPT_REMOVE 30 > +#define TDH_SYS_RD 34 > +#define TDH_MEM_TRACK 38 > +#define TDH_MEM_RANGE_UNBLOCK 39 > +#define TDH_PHYMEM_CACHE_WB 40 > +#define TDH_PHYMEM_PAGE_WBINVD 41 > +#define TDH_VP_WR 43 > +#define TDH_SYS_LP_SHUTDOWN 44
And LP_SHUTDOWN is certainly not needed.
Could you check whether there are others that are not needed?
Perhaps we should just include macros that got used, but anyway.
[...]
> + > +/* > + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is 1024B. > + */
Why is this comment applied to TDX_MAX_VCPUS?
> +#define TDX_MAX_VCPUS (~(u16)0)
And is (~(16)0) an architectural value defined by TDX spec, or just SW value that you just put here for convenience?
I mean, is it possible that different version of TDX module have different implementation of MAX_CPU, e.g., module 1.0 only supports X but module 1.5 increases to Y where Y > X?
Anyway, looks you can safely move this to the patch to enable CAP_MAX_CPU?
> + > +struct td_params { > + u64 attributes; > + u64 xfam; > + u16 max_vcpus; > + u8 reserved0[6]; > + > + u64 eptp_controls; > + u64 exec_controls; > + u16 tsc_frequency; > + u8 reserved1[38]; > + > + u64 mrconfigid[6]; > + u64 mrowner[6]; > + u64 mrownerconfig[6]; > + u64 reserved2[4]; > + > + union { > + DECLARE_FLEX_ARRAY(struct tdx_cpuid_value, cpuid_values); > + u8 reserved3[768];
I am not sure you need the 'reseved3[768]', unless you need to make sieof(struct td_params) return 1024?
> + }; > +} __packed __aligned(1024); > +
[...]
> + > +#define TDX_MD_ELEMENT_SIZE_8BITS 0 > +#define TDX_MD_ELEMENT_SIZE_16BITS 1 > +#define TDX_MD_ELEMENT_SIZE_32BITS 2 > +#define TDX_MD_ELEMENT_SIZE_64BITS 3 > + > +union tdx_md_field_id { > + struct { > + u64 field : 24; > + u64 reserved0 : 8; > + u64 element_size_code : 2; > + u64 last_element_in_field : 4; > + u64 reserved1 : 3; > + u64 inc_size : 1; > + u64 write_mask_valid : 1; > + u64 context : 3; > + u64 reserved2 : 1; > + u64 class : 6; > + u64 reserved3 : 1; > + u64 non_arch : 1; > + }; > + u64 raw; > +};
Could you clarify why we need such detailed definition? For metadata element size you can use simple '&' and '<<' to get the result.
> + > +#define TDX_MD_ELEMENT_SIZE_CODE(_field_id) \ > + ({ union tdx_md_field_id _fid = { .raw = (_field_id)}; \ > + _fid.element_size_code; }) > + > +#endif /* __KVM_X86_TDX_ARCH_H */
| |