Re: [PATCH v19 027/130] KVM: TDX: Define TDX architectural definitions

Huang, Kai
2024-03-21T21:58:09Z

> +/*
> + * 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 */