On Fri, Mar 22, 2024 at 10:57:53AM +1300,
"Huang, Kai" <kai.huang@xxxxxxxxx> wrote:
>
> > +/*
> > + * 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.
Ok, let's break this patch into other patches that uses the constants first.
> > +/*
> > + * 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?
This is architectural because it the field width is 16 bits. Each version
of TDX module may have their own limitation with metadata, MAX_VCPUS_PER_TD.
> Anyway, looks you can safely move this to the patch to enable CAP_MAX_CPU?
Yes.
> > +
> > +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?
I'm trying to make it 1024 because the spec defines the struct size is 1024.
Maybe I can add BUILD_BUG_ON(sizeof(struct td_params) != 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.
Now your TDX host patch has the definition in arch/x86/include/asm/tdx.h,
I'll eliminate this one here and use your definition.
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
