| Message ID | 1d6fe6bd392b604091b57842c15cc5460aa92593.1613221549.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM SGX virtualization support | expand |
On Sun, Feb 14, 2021 at 02:29:10AM +1300, Kai Huang wrote: > From: Sean Christopherson <sean.j.christopherson@intel.com> > > Expose SGX architectural structures, as KVM will use many of the > architectural constants and structs to virtualize SGX. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Acked-by: Dave Hansen <dave.hansen@intel.com> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/{kernel/cpu/sgx/arch.h => include/asm/sgx_arch.h} | 0 > arch/x86/kernel/cpu/sgx/encl.c | 2 +- > arch/x86/kernel/cpu/sgx/sgx.h | 2 +- > tools/testing/selftests/sgx/defines.h | 2 +- > 4 files changed, 3 insertions(+), 3 deletions(-) > rename arch/x86/{kernel/cpu/sgx/arch.h => include/asm/sgx_arch.h} (100%) > > diff --git a/arch/x86/kernel/cpu/sgx/arch.h b/arch/x86/include/asm/sgx_arch.h > similarity index 100% > rename from arch/x86/kernel/cpu/sgx/arch.h > rename to arch/x86/include/asm/sgx_arch.h Why not just sgx.h? The postfix is useless. /Jarkko
> > On Sun, Feb 14, 2021 at 02:29:10AM +1300, Kai Huang wrote: > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Expose SGX architectural structures, as KVM will use many of the > > architectural constants and structs to virtualize SGX. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Acked-by: Dave Hansen <dave.hansen@intel.com> > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > arch/x86/{kernel/cpu/sgx/arch.h => include/asm/sgx_arch.h} | 0 > > arch/x86/kernel/cpu/sgx/encl.c | 2 +- > > arch/x86/kernel/cpu/sgx/sgx.h | 2 +- > > tools/testing/selftests/sgx/defines.h | 2 +- > > 4 files changed, 3 insertions(+), 3 deletions(-) rename > > arch/x86/{kernel/cpu/sgx/arch.h => include/asm/sgx_arch.h} (100%) > > > > diff --git a/arch/x86/kernel/cpu/sgx/arch.h > > b/arch/x86/include/asm/sgx_arch.h similarity index 100% rename from > > arch/x86/kernel/cpu/sgx/arch.h rename to > > arch/x86/include/asm/sgx_arch.h > > Why not just sgx.h? The postfix is useless. Because those contents are *architectural*. They are defined in SDM. And patch 13 (x86/sgx: Add helpers to expose ECREATE and EINIT to KVM) will introduce arch/x86/include/asm/sgx.h, where non-architectural functions will be declared.
On Tue, Feb 16, 2021 at 10:30:03AM +0000, Huang, Kai wrote: > Because those contents are *architectural*. They are defined in SDM. > > And patch 13 (x86/sgx: Add helpers to expose ECREATE and EINIT to KVM) > will introduce arch/x86/include/asm/sgx.h, where non-architectural > functions will be declared. Who cares about the SDM?
> > On Tue, Feb 16, 2021 at 10:30:03AM +0000, Huang, Kai wrote: > > Because those contents are *architectural*. They are defined in SDM. > > > > And patch 13 (x86/sgx: Add helpers to expose ECREATE and EINIT to KVM) > > will introduce arch/x86/include/asm/sgx.h, where non-architectural > > functions will be declared. > > Who cares about the SDM? Sorry I am not sure I understand your question. Could you elaborate? IMHO it's better to put architectural staff (such as data structures defined in SDM and used by hardware) into one header, and other non-architectural staff into another header, so that the user can include the one that is actually required, but doesn't have to include one big header which includes all SGX related data structures and functions.
On Tue, Feb 16, 2021 at 11:15:35AM +0000, Huang, Kai wrote: > Sorry I am not sure I understand your question. Could you elaborate? > > IMHO it's better to put architectural staff (such as data structures > defined in SDM and used by hardware) into one header, and other > non-architectural staff into another header, so that the user can > include the one that is actually required, but doesn't have to include > one big header which includes all SGX related data structures and > functions. And including one big - (not sure about "big" - we have a lot bigger) - header is an actual problem because? 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. If you don't have such reasons, then it all is just unnecessary and not needed churn. And in that case, keeping it simple is the proper approach. Those headers can always be split later, when really needed. HTH.
> > On Tue, Feb 16, 2021 at 11:15:35AM +0000, Huang, Kai wrote: > > Sorry I am not sure I understand your question. Could you elaborate? > > > > IMHO it's better to put architectural staff (such as data structures > > defined in SDM and used by hardware) into one header, and other > > non-architectural staff into another header, so that the user can > > include the one that is actually required, but doesn't have to include > > one big header which includes all SGX related data structures and > > functions. > > And including one big - (not sure about "big" - we have a lot bigger) - header is > an actual problem because? > > 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. > > If you don't have such reasons, then it all is just unnecessary and not needed > churn. And in that case, keeping it simple is the proper approach. > > Those headers can always be split later, when really needed. > > HTH. Thanks for feedback! I'll take a deeper look at the code and give feedback (too late for me today). > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
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.
On Tue, Feb 16, 2021 at 11:32:18AM +0100, Borislav Petkov wrote: > On Tue, Feb 16, 2021 at 10:30:03AM +0000, Huang, Kai wrote: > > Because those contents are *architectural*. They are defined in SDM. > > > > And patch 13 (x86/sgx: Add helpers to expose ECREATE and EINIT to KVM) > > will introduce arch/x86/include/asm/sgx.h, where non-architectural > > functions will be declared. > > Who cares about the SDM? I don't. > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko
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.
On 2/16/21 10:47 AM, Borislav Petkov wrote: > 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? Fine with me. It's just as easy to miss this comment as is would be to miss the comment at the top of sgx_arch.h. :)
On Tue, Feb 16, 2021 at 10:53:03AM -0800, Dave Hansen wrote: > Fine with me. It's just as easy to miss this comment as is would be to > miss the comment at the top of sgx_arch.h. :) Sure but SGX should try to keep it together and not plant headers everywhere. :-)
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. Let's worry about split later on when we add "big" SGX specific features like EDMM, and consider this more like "move and rename". /Jarkko
> > 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. > > Let's worry about split later on when we add "big" SGX specific features like > EDMM, and consider this more like "move and rename". If we need to worry about split when we add EDMM, why we are merging to one single asm/sgx.h in this KVM SGX series? EDMM is a feature we definitely need to support, right?
diff --git a/arch/x86/kernel/cpu/sgx/arch.h b/arch/x86/include/asm/sgx_arch.h similarity index 100% rename from arch/x86/kernel/cpu/sgx/arch.h rename to arch/x86/include/asm/sgx_arch.h diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 584fceab6c76..0884b9a4e356 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -7,7 +7,7 @@ #include <linux/shmem_fs.h> #include <linux/suspend.h> #include <linux/sched/mm.h> -#include "arch.h" +#include <asm/sgx_arch.h> #include "encl.h" #include "encls.h" #include "sgx.h" diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index 1bff93be7bf4..161d2d8ac3b6 100644 --- a/arch/x86/kernel/cpu/sgx/sgx.h +++ b/arch/x86/kernel/cpu/sgx/sgx.h @@ -8,7 +8,7 @@ #include <linux/rwsem.h> #include <linux/types.h> #include <asm/asm.h> -#include "arch.h" +#include <asm/sgx_arch.h> #undef pr_fmt #define pr_fmt(fmt) "sgx: " fmt diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h index 592c1ccf4576..4dd39a003f40 100644 --- a/tools/testing/selftests/sgx/defines.h +++ b/tools/testing/selftests/sgx/defines.h @@ -14,7 +14,7 @@ #define __aligned(x) __attribute__((__aligned__(x))) #define __packed __attribute__((packed)) -#include "../../../../arch/x86/kernel/cpu/sgx/arch.h" +#include "../../../../arch/x86/include/asm/sgx_arch.h" #include "../../../../arch/x86/include/asm/enclu.h" #include "../../../../arch/x86/include/uapi/asm/sgx.h"