linux-kernel - Re: [PATCH v2 08/25] x86/virt/tdx: Add SEAMCALL wrappers for TDX page cache management

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: <aff59a1a-c8e7-4784-b950-595875bf6304@intel.com>
Date: Tue, 12 Nov 2024 16:20:19 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>, pbonzini@...hat.com,
 seanjc@...gle.com
Cc: yan.y.zhao@...el.com, isaku.yamahata@...il.com, kai.huang@...el.com,
 kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 tony.lindgren@...ux.intel.com, xiaoyao.li@...el.com,
 reinette.chatre@...el.com, Isaku Yamahata <isaku.yamahata@...el.com>,
 Sean Christopherson <sean.j.christopherson@...el.com>,
 Binbin Wu <binbin.wu@...ux.intel.com>, Yuan Yao <yuan.yao@...el.com>
Subject: Re: [PATCH v2 08/25] x86/virt/tdx: Add SEAMCALL wrappers for TDX page
 cache management

On 10/30/24 12:00, Rick Edgecombe wrote:
> +u64 tdh_phymem_page_reclaim(u64 page, u64 *rcx, u64 *rdx, u64 *r8)
> +{
> +	struct tdx_module_args args = {
> +		.rcx = page,
> +	};
> +	u64 ret;

This isn't quite what I'm looking for in these wrappers.

For instance:

> +	/*
> +	 * Additional error information:
> +	 *
> +	 *  - RCX: page type
> +	 *  - RDX: owner
> +	 *  - R8:  page size (4K, 2M or 1G)
> +	 */
> +	*rcx = args.rcx;
> +	*rdx = args.rdx;
> +	*r8 = args.r8;

If this were, instead:

u64 tdh_phymem_page_reclaim(u64 page, u64 *type, u64 *owner, u64 *size)
{
	...
	*type = args.rcx;
	*owner = args.rdx;
	*size = args.r8;

Then you wouldn't need the comment in the first place.  Then you could
also be thinking about adding _some_ kind of type safety to the
arguments.  The 'size' or the 'type' could totally be enums.

There's really zero value in having wrappers like these.  They don't
have any type safety or add any readability or make the seamcall easier
to use.  There's almost no value in having these versus just exporting
seamcall_ret() itself.