Re: [PATCH v9 31/43] x86/compressed/64: Add support for SEV-SNP CPUID table in #VC handlers

[Borislav Petkov]

On Fri, Jan 28, 2022 at 11:17:52AM -0600, Brijesh Singh wrote:
> +/*
> + * Individual entries of the SEV-SNP CPUID table, as defined by the SEV-SNP
> + * Firmware ABI, Revision 0.9, Section 7.1, Table 14.
> + */
> +struct snp_cpuid_fn {
> +	u32 eax_in;
> +	u32 ecx_in;
> +	u64 xcr0_in;
> +	u64 xss_in;

So what's the end result here:

-+	u64 __unused;
-+	u64 __unused2;
++	u64 xcr0_in;
++	u64 xss_in;

those are not unused fields anymore but xcr0 and xss input values?

Looking at the FW abi doc, they're only mentioned in "Table 14.
CPUID_FUNCTION Structure" that they're XCR0 and XSS at the time of the
CPUID execution.

But those values are input values to what exactly, guest or firmware?

There's a typo in the FW doc, btw:

"The guest constructs an MSG_CPUID_REQ message as defined in Table 13.
This message contains an array of CPUID function structures as defined
in Table 13."

That second "Table" is 14 not 13.

So, if an array CPUID_FUNCTION[] is passed as part of an MSG_CPUID_REQ
command, then, the two _IN variables contain what the guest received
from the HV for XCR0 and XSS values. Which means, this is the guest
asking the FW whether those values the HV gave the guest are kosher.

Am I close?

> +static const struct snp_cpuid_info *snp_cpuid_info_get_ptr(void)
> +{
> +	void *ptr;
> +
> +	asm ("lea cpuid_info_copy(%%rip), %0"
> +	     : "=r" (ptr)

Same question as the last time:

Why not "=g" and let the compiler decide?

> +	     : "p" (&cpuid_info_copy));
> +
> +	return ptr;
> +}

...

> +static bool snp_cpuid_check_range(u32 func)
> +{
> +	if (func <= cpuid_std_range_max ||
> +	    (func >= 0x40000000 && func <= cpuid_hyp_range_max) ||
> +	    (func >= 0x80000000 && func <= cpuid_ext_range_max))
> +		return true;
> +
> +	return false;
> +}
> +
> +static int snp_cpuid_postprocess(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> +				 u32 *ecx, u32 *edx)

And again, same question as the last time:

I'm wondering if you could make everything a lot easier by doing

static int snp_cpuid_postprocess(struct cpuid_leaf *leaf)

and marshall around that struct cpuid_leaf which contains func, subfunc,
e[abcd]x instead of dealing with 6 parameters.

Callers of snp_cpuid() can simply allocate it on their stack and hand it
in and it is all in sev-shared.c so nicely self-contained...

Ok I'm ignoring this patch for now and I'll review it only after you've
worked in all comments from the previous review.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette