Re: [RFC PATCH v5 08/26] x86/sgx: Expose SGX architectural definitions to the kernel

From: Borislav Petkov <bp@alien8.de>
To: Dave Hansen <dave.hansen@intel.com>
Cc: "Huang, Kai" <kai.huang@intel.com>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"seanjc@google.com" <seanjc@google.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"Edgecombe, Rick P" <rick.p.edgecombe@intel.com>,
	"Huang, Haitao" <haitao.huang@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"hpa@zytor.com" <hpa@zytor.com>
Subject: Re: [RFC PATCH v5 08/26] x86/sgx: Expose SGX architectural definitions to the kernel
Date: Tue, 16 Feb 2021 19:47:18 +0100	[thread overview]
Message-ID: <20210216184718.GE10592@zn.tnic> (raw)
In-Reply-To: <9dca76b9-a0f9-a7aa-5d85-f8b43f89a3d2@intel.com>

On Tue, Feb 16, 2021 at 07:18:27AM -0800, Dave Hansen wrote:
> On 2/16/21 3:48 AM, Borislav Petkov wrote:
> > What I'm trying to point you at is, to not give some artificial reasons
> > why the headers should be separate - artificial as the SDM says it
> > is architectural and so on - but give a reason from software design
> > perspective why the separation is needed: better build times, less
> > symbols exposed to modules, blabla and so on.
> 
> I think I actually suggested this sgx_arch.h split for SGX in the first
> place.
> 
> I was reading the patches and I had a really hard time separating the
> hardware and software structures.  There would be a 'struct sgx_foo {}'
> and some chit chat about what it did...  and I still had no idea if it
> was an architectural structure or not.
> 
> This way, it's 100% crystal clear what Linux is defining and what the
> hardware defines from the diff context.

Yap, that's a valid reason in my book. And arch.h has at the top:

 * Contains data structures defined by the SGX architecture.  Data structures
 * defined by the Linux software stack should not be placed here.

and by now we already have:

$ git ls-files | grep \/sgx.h
arch/x86/include/uapi/asm/sgx.h
arch/x86/kernel/cpu/sgx/sgx.h

two sgx.h headers.

So how about we have a single

  arch/x86/include/asm/sgx.h

header which contains the architectural definitions at the *top* and at
their end, there'll be a:

/* Do not put any hardware-defined SGX structure representations below this line! */

and after that line begin the other, software definitions?

Then that arch/x86/kernel/cpu/sgx/sgx.h can be renamed to private.h
because it is included only there so you'll have:

arch/x86/include/uapi/asm/sgx.h	- user API crap
arch/x86/include/asm/sgx.h - shared with other kernel facilities
arch/x86/kernel/cpu/sgx/internal.h - SGX-internal definitions

How does that look?

And we do have similar header structure with other features...

Thx.

-- 
Regards/Gruss,
    Boris.

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