Re: [PATCH v11 07/13] x86, sgx: detect Intel SGX

[Dave Hansen]

> +config INTEL_SGX_CORE
> +	prompt "Intel SGX core functionality
> +	depends on X86_64 && CPU_SUP_INTEL
> +	help
> +	Intel Software Guard eXtensions (SGX) is a set of CPU instructions
> +	that allows ring 3 applications to create enclaves; private regions
> +	of memory that are protected, by hardware, from unauthorized access
> +	and/or modification.

That semicolon needs to be a colon.  The second half of that sentence is
not a stand-alone statement.

> +	This option enables kernel recognition of SGX, high-level management
> +	of the Enclave Page Cache (EPC), tracking and writing of SGX Launch
> +	Enclave Hash MSRs, and allows for virtualization of SGX via KVM. By
> +	iteslf, this option does not provide SGX support to userspace.
> +
> +	For details, see Documentation/x86/intel_sgx.rst
> +
> +	If unsure, say N.
> +
>  config EFI
>  	bool "EFI runtime service support"
>  	depends on ACPI
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> new file mode 100644
> index 000000000000..fa3e6e0eb8af
> --- /dev/null
> +++ b/arch/x86/include/asm/sgx.h
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +//
> +// Authors:
> +//
> +// Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> +// Suresh Siddha <suresh.b.siddha@xxxxxxxxx>
> +// Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> +
> +#ifndef _ASM_X86_SGX_H
> +#define _ASM_X86_SGX_H
> +
> +#include <linux/types.h>
> +
> +#define SGX_CPUID 0x12

Hey, I just saw 0x12 as a magic, hard-coded number earlier in these
patches.  It seems cruel to hard-code it, and then also have a #define
that isn't used.

> +enum sgx_cpuid {
> +	SGX_CPUID_CAPABILITIES	= 0,
> +	SGX_CPUID_ATTRIBUTES	= 1,
> +	SGX_CPUID_EPC_BANKS	= 2,
> +};

These are cpuid *leaves*, right?  Please make this clear that these are
hardware-defined values and not some kind of software construct.

> +bool sgx_enabled __ro_after_init = false;
> +EXPORT_SYMBOL(sgx_enabled);
> +
> +static __init bool sgx_is_enabled(void)
> +{
> +	unsigned long fc;
> +
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> +		return false;

Not necessary. CPUID does this part for you.

> +	if (!boot_cpu_has(X86_FEATURE_SGX))
> +		return false;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SGX1))
> +		return false;
> +
> +	rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
> +	if (!(fc & FEATURE_CONTROL_LOCKED))
> +		return false;
> +
> +	if (!(fc & FEATURE_CONTROL_SGX_ENABLE))
> +		return false;

Comments, please.  Why would this happen?  What would it mean?