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?
