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

[Huang, Kai]

On 26/02/2024 9:25 pm, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
> Reviewed-by: Yuan Yao <yuan.yao@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> ---

[...]

> +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.