Re: [PATCH v3 01/21] x86/virt/tdx: Detect SEAM

[Kai Huang <kai.huang@intel.com>]

On Tue, 2022-04-26 at 16:28 -0700, Dave Hansen wrote:
> On 4/26/22 16:12, Kai Huang wrote:
> > Hi Dave,
> > 
> > Thanks for review!
> > 
> > On Tue, 2022-04-26 at 13:21 -0700, Dave Hansen wrote:
> > > > +config INTEL_TDX_HOST
> > > > +	bool "Intel Trust Domain Extensions (TDX) host support"
> > > > +	default n
> > > > +	depends on CPU_SUP_INTEL
> > > > +	depends on X86_64
> > > > +	help
> > > > +	  Intel Trust Domain Extensions (TDX) protects guest VMs from
> > > > malicious
> > > > +	  host and certain physical attacks.  This option enables necessary
> > > > TDX
> > > > +	  support in host kernel to run protected VMs.
> > > > +
> > > > +	  If unsure, say N.
> > > 
> > > Nothing about KVM?
> > 
> > I'll add KVM into the context. How about below?
> > 
> > "Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
> > host and certain physical attacks.  This option enables necessary TDX
> > support in host kernel to allow KVM to run protected VMs called Trust
> > Domains (TD)."
> 
> What about a dependency?  Isn't this dead code without CONFIG_KVM=y/m?

Conceptually, KVM is one user of the TDX module, so it doesn't seem correct to
make CONFIG_INTEL_TDX_HOST depend on CONFIG_KVM.  But so far KVM is the only
user of TDX, so in practice the code is dead w/o KVM.

What's your opinion?

> 
> > > > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > > > new file mode 100644
> > > > index 000000000000..03f35c75f439
> > > > --- /dev/null
> > > > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > > > @@ -0,0 +1,102 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright(c) 2022 Intel Corporation.
> > > > + *
> > > > + * Intel Trusted Domain Extensions (TDX) support
> > > > + */
> > > > +
> > > > +#define pr_fmt(fmt)	"tdx: " fmt
> > > > +
> > > > +#include <linux/types.h>
> > > > +#include <linux/cpumask.h>
> > > > +#include <asm/msr-index.h>
> > > > +#include <asm/msr.h>
> > > > +#include <asm/cpufeature.h>
> > > > +#include <asm/cpufeatures.h>
> > > > +#include <asm/tdx.h>
> > > > +
> > > > +/* Support Intel Secure Arbitration Mode Range Registers (SEAMRR) */
> > > > +#define MTRR_CAP_SEAMRR			BIT(15)
> > > > +
> > > > +/* Core-scope Intel SEAMRR base and mask registers. */
> > > > +#define MSR_IA32_SEAMRR_PHYS_BASE	0x00001400
> > > > +#define MSR_IA32_SEAMRR_PHYS_MASK	0x00001401
> > > > +
> > > > +#define SEAMRR_PHYS_BASE_CONFIGURED	BIT_ULL(3)
> > > > +#define SEAMRR_PHYS_MASK_ENABLED	BIT_ULL(11)
> > > > +#define SEAMRR_PHYS_MASK_LOCKED		BIT_ULL(10)
> > > > +
> > > > +#define SEAMRR_ENABLED_BITS	\
> > > > +	(SEAMRR_PHYS_MASK_ENABLED | SEAMRR_PHYS_MASK_LOCKED)
> > > > +
> > > > +/* BIOS must configure SEAMRR registers for all cores consistently */
> > > > +static u64 seamrr_base, seamrr_mask;
> > > > +
> > > > +static bool __seamrr_enabled(void)
> > > > +{
> > > > +	return (seamrr_mask & SEAMRR_ENABLED_BITS) == SEAMRR_ENABLED_BITS;
> > > > +}
> > > 
> > > But there's no case where seamrr_mask is non-zero and where
> > > _seamrr_enabled().  Why bother checking the SEAMRR_ENABLED_BITS?
> > 
> > seamrr_mask will only be non-zero when SEAMRR is enabled by BIOS, otherwise it
> > is 0.  It will also be cleared when BIOS mis-configuration is detected on any
> > AP.  SEAMRR_ENABLED_BITS is used to check whether SEAMRR is enabled.
> 
> The point is that this could be:
> 
> 	return !!seamrr_mask;

The definition of this SEAMRR_MASK MSR defines "ENABLED" and "LOCKED" bits. 
Explicitly checking the two bits, instead of !!seamrr_mask roles out other
incorrect configurations.  For instance, we should not treat SEAMRR being
enabled if we only have "ENABLED" bit set or "LOCKED" bit set.

> 
> 
> > > > +static void detect_seam_ap(struct cpuinfo_x86 *c)
> > > > +{
> > > > +	u64 base, mask;
> > > > +
> > > > +	/*
> > > > +	 * Don't bother to detect this AP if SEAMRR is not
> > > > +	 * enabled after earlier detections.
> > > > +	 */
> > > > +	if (!__seamrr_enabled())
> > > > +		return;
> > > > +
> > > > +	rdmsrl(MSR_IA32_SEAMRR_PHYS_BASE, base);
> > > > +	rdmsrl(MSR_IA32_SEAMRR_PHYS_MASK, mask);
> > > > +
> > > 
> > > This is the place for a comment about why the values have to be equal.
> > 
> > I'll add below:
> > 
> > /* BIOS must configure SEAMRR consistently across all cores */
> 
> What happens if the BIOS doesn't do this?  What actually breaks?  In
> other words, do we *NEED* error checking here?

AFAICT the spec doesn't explicitly mention what will happen if BIOS doesn't
configure them consistently among cores.  But for safety I think it's better to
detect.

> 
> > > > +	if (base == seamrr_base && mask == seamrr_mask)
> > > > +		return;
> > > > +
> > > > +	pr_err("Inconsistent SEAMRR configuration by BIOS\n");
> > > > +	/* Mark SEAMRR as disabled. */
> > > > +	seamrr_base = 0;
> > > > +	seamrr_mask = 0;
> > > > +}
> > > > +
> > > > +static void detect_seam(struct cpuinfo_x86 *c)
> > > > +{
> > > > +	if (c == &boot_cpu_data)
> > > > +		detect_seam_bsp(c);
> > > > +	else
> > > > +		detect_seam_ap(c);
> > > > +}
> > > > +
> > > > +void tdx_detect_cpu(struct cpuinfo_x86 *c)
> > > > +{
> > > > +	detect_seam(c);
> > > > +}
> > > 
> > > The extra function looks a bit silly here now.  Maybe this gets filled
> > > out later, but it's goofy-looking here.
> > 
> > Thomas suggested to put all TDX detection related in one function call, so I
> > added tdx_detect_cpu().  I'll move this to the next patch when detecting TDX
> > KeyIDs.
> 
> That's fine, or just add a comment or a changelog sentence about this
> being filled out later.

There's already one sentence in the changelog:

"......Add a function to detect all TDX preliminaries (SEAMRR, TDX private
KeyIDs) for a given cpu when it is brought up.  As the first step, detect the
validity of SEAMRR."

Does this look good to you?