Re: [RFC PATCH 04/23] x86/cpufeatures: Add SGX1 and SGX2 sub-features

From: Borislav Petkov <bp@alien8.de>
To: Sean Christopherson <seanjc@google.com>
Cc: Kai Huang <kai.huang@intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	linux-sgx@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org,
	jarkko@kernel.org, luto@kernel.org, haitao.huang@intel.com,
	pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com,
	hpa@zytor.com
Subject: Re: [RFC PATCH 04/23] x86/cpufeatures: Add SGX1 and SGX2 sub-features
Date: Sat, 9 Jan 2021 02:19:39 +0100	[thread overview]
Message-ID: <20210109011939.GL4042@zn.tnic> (raw)
In-Reply-To: <X/jxCOLG+HUO4QlZ@google.com>

On Fri, Jan 08, 2021 at 03:55:52PM -0800, Sean Christopherson wrote:
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dc921d76e42e..21f92d81d5a5 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -7,7 +7,25 @@
>  #include <asm/processor.h>
>  #include <uapi/asm/kvm_para.h>
> 
> -extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly;
> +/*
> + * Hardware-defined CPUID leafs that are scattered in the kernel, but need to
> + * be directly by KVM.  Note, these word values conflict with the kernel's
> + * "bug" caps, but KVM doesn't use those.

This feels like another conflict waiting to happen if KVM decides to use
them at some point...

So let me get this straight: KVM wants to use X86_FEATURE_* which
means, those numbers must map to the respective words in its CPUID caps
representation kvm_cpu_caps, AFAICT.

Then, it wants the leafs to correspond to the hardware leafs layout so
that it can do:

	kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg);

which comes straight from CPUID.

So lemme look at one word:

        kvm_cpu_cap_mask(CPUID_1_EDX,
                F(FPU) | F(VME) | F(DE) | F(PSE) |
                F(TSC) | F(MSR) | F(PAE) | F(MCE) |
		...

it would build the bitmask of the CPUID leaf using X86_FEATURE_* bits
and then mask it out with the hardware leaf read from CPUID.

But why?

Why doesn't it simply build those leafs in kvm_cpu_caps from the leafs
we've already queried?

Oh it does so a bit earlier:

        memcpy(&kvm_cpu_caps, &boot_cpu_data.x86_capability,
               sizeof(kvm_cpu_caps));

and that kvm_cpu_cap_mask() call is to clear some bits in kvm_cpu_caps
which is kvm-specific thing (not supported stuff etc).

But then why does kvm_cpu_cap_mask() does cpuid_count()? Didn't it just
read the bits from boot_cpu_data.x86_capability? And those bits we do
query and massage extensively during boot. So why does KVM needs to
query CPUID again instead of using what we've already queried?

Maybe I'm missing something kvm-specific.

In any case, this feels somewhat weird: you have *_cpu_has() on
baremetal abstracting almost completely from CPUID by collecting all
feature bits it needs into its own structure - x86_capability[] along
with accessors for it - and then you want to "abstract back" to CPUID
leafs from that interface. I wonder why.

Anyway, more questions tomorrow.

Gnight and good luck. :)

-- 
Regards/Gruss,
    Boris.

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