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

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

On Wed, 2022-04-27 at 07:22 -0700, Dave Hansen wrote:
> On 4/26/22 16:49, Kai Huang wrote:
> > On Tue, 2022-04-26 at 16:28 -0700, Dave Hansen wrote:
> > > 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?
> 
> You're stuck in some really weird fantasy world.  Sure, we can dream up
> more than one user of the TDX module.  But, in the real world, there's
> only one.  Plus, code can have multiple dependencies!
> 
> 	depends on FOO || BAR
> 
> This TDX cruft is dead code in today's real-world kernel without KVM.
> You should add a dependency.

Will add a dependency on CONFIG_KVM_INTEL.

> 
> > > > > > +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.
> 
> You're confusing two different things:
>  * The state of the variable
>  * The actual correct hardware state
> 
> The *VARIABLE* can't be non-zero and also denote that SEAMRR is enabled.
>  Does this *CODE* ever set ENABLED or LOCKED without each other?

OK.  Will just use !!seamrr_mask.  I thought explicitly checking
SEAMRR_ENABLED_BITS would be clearer.

> 
> > > > > > +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.
> 
> Safety?  Safety of what?

I'll ask TDX architect people and get back to you.

I'll also ask what will happen if TDX KeyID isn't configured consistently across
packages.  Currently TDX KeyID is also detected on all cpus (existing
detect_tme() also detect MKTME KeyID bits on all cpus).