linux-kernel - Re: [PATCH v19 030/130] KVM: TDX: Add helper functions to print TDX SEAMCALL error

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: <315bfb1b-7735-4e26-b881-fcaa25e0d3f3@intel.com>
Date: Wed, 20 Mar 2024 13:29:07 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: <isaku.yamahata@...el.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
CC: <isaku.yamahata@...il.com>, Paolo Bonzini <pbonzini@...hat.com>,
	<erdemaktas@...gle.com>, Sean Christopherson <seanjc@...gle.com>, Sagi Shahar
	<sagis@...gle.com>, <chen.bo@...el.com>, <hang.yuan@...el.com>,
	<tina.zhang@...el.com>, Binbin Wu <binbin.wu@...ux.intel.com>, Yuan Yao
	<yuan.yao@...el.com>
Subject: Re: [PATCH v19 030/130] KVM: TDX: Add helper functions to print TDX
 SEAMCALL error



On 26/02/2024 9:25 pm, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> Add helper functions to print out errors from the TDX module in a uniform
> manner.

Likely we need more information here.  See below.

> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> Reviewed-by: Binbin Wu <binbin.wu@...ux.intel.com>
> Reviewed-by: Yuan Yao <yuan.yao@...el.com>
> ---
> v19:
> - dropped unnecessary include <asm/tdx.h>
> 
> v18:
> - Added Reviewed-by Binbin.

The tag doesn't show in the SoB chain.

> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---

[...]

> +void pr_tdx_error(u64 op, u64 error_code, const struct tdx_module_args *out)
> +{
> +	if (!out) {
> +		pr_err_ratelimited("SEAMCALL (0x%016llx) failed: 0x%016llx\n",
> +				   op, error_code);
> +		return;
> +	}

I think this is the reason you still want the @out in tdx_seamcall()?

But I am not sure either -- even if you want to have @out *here* -- why 
cannot you pass a NULL explicitly when you *know* the concerned SEAMCALL 
doesn't have a valid output?

> +
> +#define MSG	\
> +	"SEAMCALL (0x%016llx) failed: 0x%016llx RCX 0x%016llx RDX 0x%016llx R8 0x%016llx R9 0x%016llx R10 0x%016llx R11 0x%016llx\n"
> +	pr_err_ratelimited(MSG, op, error_code, out->rcx, out->rdx, out->r8,
> +			   out->r9, out->r10, out->r11);
> +}

Besides the regs that you are printing, there are more regs (R12-R15, 
RDI, RSI) in the structure.

It's not clear why you only print some, but not all.

AFAICT the VP.ENTER SEAMCALL can have all regs as valid output?

Anyway, that being said, you might need to put more text in 
changelog/comment to make this patch (at least more) reviewable.