Message-ID: <d3fa4260c50c8d4101f8476c1cc4d6474b5800ce.camel@intel.com>
Date: Tue, 1 Oct 2024 10:44:31 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Hansen, Dave" <dave.hansen@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "bp@...en8.de" <bp@...en8.de>, "peterz@...radead.org"
	<peterz@...radead.org>, "hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "Williams, Dan J" <dan.j.williams@...el.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "nik.borisov@...e.com"
	<nik.borisov@...e.com>, "Hunter, Adrian" <adrian.hunter@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Edgecombe,
 Rick P" <rick.p.edgecombe@...el.com>, "x86@...nel.org" <x86@...nel.org>,
	"Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other
 global metadata fields

On Tue, 2024-10-01 at 00:56 -0700, Dan Williams wrote:
> Huang, Kai wrote:
> > 
> > 
> > On 27/09/2024 10:26 am, Hansen, Dave wrote:
> > > On 9/26/24 15:22, Huang, Kai wrote:
> > > > But Dan commented using typeless 'void *' and 'size' is kinda a step
> > > > backwards and we should do something similar to build_mmio_read():
> > > 
> > > Well, void* is typeless, but at least it knows the size in this case.
> > > It's not completely aimless.  I was thinking of how things like
> > > get_user() work.
> > 
> > get_user(x,ptr) only works with simple types:
> > 
> >   * @ptr must have pointer-to-simple-variable type, and the result of
> >   * dereferencing @ptr must be assignable to @x without a cast.
> > 
> > The compiler knows the type of both @x and @(*ptr), so it knows 
> > type-safety and size to copy.
> > 
> > I think we can eliminate the __read_sys_metadata_field() by implementing 
> > it as a macro directly and get rid of 'void *' and 'size':
> > 
> > static int tdh_sys_rd(u64 field_id, u64 *val) {}
> > 
> > /* @_valptr must be pointer to u8/u16/u32/u64 */
> > #define read_sys_metadata_field(_field_id, _valptr)	\
> > ({							\
> > 	u64 ___tmp;					\
> > 	int ___ret;					\
> > 							\
> > 	BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=	\
> > 		sizeof(*_valptr));			\
> > 							\
> > 	___ret = tdh_sys_rd(_field_id, &___tmp);	\
> > 							\
> > 	*_valptr = ___tmp;				\
> > 	___ret;
> > })
> > 
> > It sets *_valptr unconditionally but we can also only do it when ___ret 
> > is 0.
> > 
> > The caller will need to do:
> > 
> > static int get_tdx_metadata_X_which_is_32bit(...)
> > {
> > 	u32 metadata_X;
> > 	int ret;
> > 
> > 	ret = read_sys_metadata_field(MD_FIELD_ID_X, &metadata_X);
> > 
> > 	return ret;
> > }
> > 
> > I haven't compiled and tested but it seems feasible.
> > 
> > Any comments?
> 
> If it works this approach addresses all the concerns I had with getting
> the compiler to validate field sizes.

Yes I just quickly tested on my box and it worked -- TDX module can be
initialized successfully and all metadata fields (module version, CMRs etc) seem
to be correct.

Hi Dave,

Please let me know if you have any concern?  Otherwise I will go with this
route.

> 
> Should be straightforward to put this in a shared location so that it
> can optionally use tdg_sys_rd internally.

Yeah it's doable.  As you also noticed guest and host use different calls: guest
uses tdg_vm_rd() (which is in Kirill's not-yet-merged series[*]), and host uses
tdh_sys_rd().  

[*]
https://lore.kernel.org/all/20240828093505.2359947-2-kirill.shutemov@linux.intel.com/

This can be resolved by adding a new argument to the read_sys_metadata_field()
macro, e.g.,:

/* @_valptr must be pointer to u8/u16/u32/u64 */
#define read_sys_metadata_field(_read_func, _field_id, _valptr)		\
({									\
	u64 ___tmp;							\
	int ___ret;							\
									\
	BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) !=			\
		sizeof(*_valptr));					\
									\
	___ret = _read_func(_field_id, &___tmp);			\
									\
	*_valptr = ___tmp;						\
	___ret;							
	\
})

We can put it in <asm/tdx.h> (together with the MD_FIELD_ELE_SIZE() macro) for
guest and host to share.

And in guest code (arch/x86/coco/tdx/tdx.c), we can have a wrapper:

#define tdg_read_sys_metadata_field(_field_id, _valptr)		\
	read_sys_metadata_field(tdg_vm_rd, _field_id, _valptr)

Similarly, in the host code (arch/x86/virt/vmx/tdx/tdx.c), we can have:

#define tdh_read_sys_metadata_field(_field_id, _valptr)		\
	read_sys_metadata_field(tdh_sys_rd, _field_id, _valptr)

We can start with this if you think it's better.

But I would like to discuss this more:

Once we start to share, it feels a little bit odd to share only the
read_sys_metadata_field() macro, because we can probably share others too:

1) The metadata field ID definitions and bit definitions (this is obvious).
2) tdh_sys_rd() and tdg_vm_rd() are similar and can be shared too:

/* Read TD-scoped metadata */
static inline u64 __maybe_unused tdg_vm_rd(u64 field, u64 *value)
{
	struct tdx_module_args args = {
		.rdx = field,
	};
	u64 ret;

	ret = __tdcall_ret(TDG_VM_RD, &args);
	*value = args.r8;

	return ret;
}

static int tdh_sys_rd(u64 field_id, u64 *data)    
{                                    
        struct tdx_module_args args = {};                        
        int ret;                                                           
                                                                           
        /*                                                 
         * TDH.SYS.RD -- reads one global metadata field               
         *  - RDX (in): the field to read                               
         *  - R8 (out): the field data                                       
         */                                           
        args.rdx = field_id;                                                  
        ret = seamcall_prerr_ret(TDH_SYS_RD, &args);
        if (ret)                                                    
                return ret;                             
                             
        *data = args.r8;                            
                                                                 
        return 0;                               
}

There are minor differences, e.g., tdh_sys_rd() only sets *data when TDH_SYS_RD
succeeded but tdg_vm_rd() unconditionally sets *value (spec says R8 is set to 0
if TDG_VM_RD or TDH_SYS_RD fails, and guest code kinda depends on this).

Putting aside which is better, those differences are resolvable.  And if we
start to share, it appears we should share this too.

And then the TDH_SYS_RD definition (and probably all TDH_SYS_xx SEAMCALL leaf
definitions) should be moved to <asm/tdx.h> too.

And then the header will grow bigger and bigger, which brings us a question on
how to better organize all those definitions: 1) TDCALL/SEAMCALL leaf function
definitions (e.g., TDH_SYS_RD); 2) TDCALL/SEAMCALL low level functions
(__tdcall(), __seamcall()); 3) TDCALL/SEAMCALL leaf function wrappers; 4) sys
metadata field ID definitions; 5) sys metadata read helper macros;

So to me we can have a separate series to address how to better organize TDX
code and how to share code between guest and host (I can start to work on it if
it's better to be done sooner rather than later), thus I am not sure we want to
start sharing read_sys_metadata_field() macro in _this_ series.

But of course, if it is worth to share read_sys_metadata_field() in this series,
I can add a patch as the last patch of this series to only share
read_sys_metadata_field() in <asm/tdx.h> as mentioned above.

Powered by blists - more mailing lists