linux-kernel - Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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



On 27/09/2024 3:47 am, Hansen, Dave wrote:
> On 9/24/24 04:28, Kai Huang wrote:
>> +#define build_sysmd_read(_size)							\
>> +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
>> +{										\
>> +	u64 tmp;								\
>> +	int ret;								\
>> +										\
>> +	ret = tdh_sys_rd(field_id, &tmp);					\
>> +	if (ret)								\
>> +		return ret;							\
>> +										\
>> +	*val = tmp;								\
>> +										\
>> +	return 0;								\
>>   }
> 
> Why?  What's so important about having the compiler do the copy?
> 
> 
> #define read_sys_metadata_field(id, val) 	\
> 	__read_sys_metadata_field(id, val, sizeof (*(val)))
> 
> static int __read_sys_metadata_field(u64 field_id, void *ptr, int size)
> {
> 	...
> 	memcpy(ptr, &tmp, size);
> 
> 	return 0;
> }
> 
> There's one simple #define there so that users don't have to do the
> sizeof and can't screw it up.

Yes we can do this.  This is basically what I did in the previous version:

https://lore.kernel.org/kvm/0403cdb142b40b9838feeb222eb75a4831f6b46d.1724741926.git.kai.huang@intel.com/

But Dan commented using typeless 'void *' and 'size' is kinda a step 
backwards and we should do something similar to build_mmio_read():

https://lore.kernel.org/kvm/66db75497a213_22a2294b@dwillia2-xfh.jf.intel.com.notmuch/

Hi Dan,

I think what Dave suggested makes sense.  If the concern is using 
__read_sys_metadata_field() directly isn't typesafe, we can add a 
comment to it saying callers should not use it directly and use 
read_sys_metadata_field() instead.

Dave's approach also makes the LoC slightly shorter, and cleaner from 
the perspective that we don't need to explicitly specify the '16/32/64' 
in the READ_SYS_INFO() macro anymore as shown in here:

https://lore.kernel.org/kvm/79c256b8978310803bb4de48cd81dd373330cbc2.1727173372.git.kai.huang@intel.com/

Please let me know your comment?