LinuxLists.cc - [PATCHv2 00/29] TDX Guest: TDX core support

[permalink] [raw]

Subject: [PATCHv2 10/29] x86: Consolidate port I/O helpers

There are two implementations of port I/O helpers: one in the kernel and
one in the boot stub.

Move the helpers required for both to <asm/shared/io.h> and use the one
implementation everywhere.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/boot/boot.h | 35 +-------------------------------
arch/x86/boot/compressed/misc.h | 2 +-
arch/x86/include/asm/io.h | 22 ++------------------
arch/x86/include/asm/shared/io.h | 32 +++++++++++++++++++++++++++++
4 files changed, 36 insertions(+), 55 deletions(-)
create mode 100644 arch/x86/include/asm/shared/io.h

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 34c9dbb6a47d..22a474c5b3e8 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -23,6 +23,7 @@
#include <linux/edd.h>
#include <asm/setup.h>
#include <asm/asm.h>
+#include <asm/shared/io.h>
#include "bitops.h"
#include "ctype.h"
#include "cpuflags.h"
@@ -35,40 +36,6 @@ extern struct boot_params boot_params;

#define cpu_relax() asm volatile("rep; nop")

-/* Basic port I/O */
-static inline void outb(u8 v, u16 port)
-{
- asm volatile("outb %0,%1" : : "a" (v), "dN" (port));
-}
-static inline u8 inb(u16 port)
-{
- u8 v;
- asm volatile("inb %1,%0" : "=a" (v) : "dN" (port));
- return v;
-}
-
-static inline void outw(u16 v, u16 port)
-{
- asm volatile("outw %0,%1" : : "a" (v), "dN" (port));
-}
-static inline u16 inw(u16 port)
-{
- u16 v;
- asm volatile("inw %1,%0" : "=a" (v) : "dN" (port));
- return v;
-}
-
-static inline void outl(u32 v, u16 port)
-{
- asm volatile("outl %0,%1" : : "a" (v), "dN" (port));
-}
-static inline u32 inl(u16 port)
-{
- u32 v;
- asm volatile("inl %1,%0" : "=a" (v) : "dN" (port));
- return v;
-}
-
static inline void io_delay(void)
{
const u16 DELAY_PORT = 0x80;
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 0d8e275a9d96..8a253e85f990 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -22,11 +22,11 @@
#include <linux/linkage.h>
#include <linux/screen_info.h>
#include <linux/elf.h>
-#include <linux/io.h>
#include <asm/page.h>
#include <asm/boot.h>
#include <asm/bootparam.h>
#include <asm/desc_defs.h>
+#include <asm/shared/io.h>

#include "tdx.h"

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index f6d91ecb8026..8ce0a40379de 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -44,6 +44,7 @@
#include <asm/page.h>
#include <asm/early_ioremap.h>
#include <asm/pgtable_types.h>
+#include <asm/shared/io.h>

#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -258,20 +259,6 @@ static inline void slow_down_io(void)
#endif

#define BUILDIO(bwl, bw, type) \
-static inline void out##bwl(unsigned type value, int port) \
-{ \
- asm volatile("out" #bwl " %" #bw "0, %w1" \
- : : "a"(value), "Nd"(port)); \
-} \
- \
-static inline unsigned type in##bwl(int port) \
-{ \
- unsigned type value; \
- asm volatile("in" #bwl " %w1, %" #bw "0" \
- : "=a"(value) : "Nd"(port)); \
- return value; \
-} \
- \
static inline void out##bwl##_p(unsigned type value, int port) \
{ \
out##bwl(value, port); \
@@ -320,10 +307,8 @@ static inline void ins##bwl(int port, void *addr, unsigned long count) \
BUILDIO(b, b, char)
BUILDIO(w, w, short)
BUILDIO(l, , int)
+#undef BUILDIO

-#define inb inb
-#define inw inw
-#define inl inl
#define inb_p inb_p
#define inw_p inw_p
#define inl_p inl_p
@@ -331,9 +316,6 @@ BUILDIO(l, , int)
#define insw insw
#define insl insl

-#define outb outb
-#define outw outw
-#define outl outl
#define outb_p outb_p
#define outw_p outw_p
#define outl_p outl_p
diff --git a/arch/x86/include/asm/shared/io.h b/arch/x86/include/asm/shared/io.h
new file mode 100644
index 000000000000..f17247f6c471
--- /dev/null
+++ b/arch/x86/include/asm/shared/io.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_SHARED_IO_H
+#define _ASM_X86_SHARED_IO_H
+
+#define BUILDIO(bwl, bw, type) \
+static inline void out##bwl(unsigned type value, int port) \
+{ \
+ asm volatile("out" #bwl " %" #bw "0, %w1" \
+ : : "a"(value), "Nd"(port)); \
+} \
+ \
+static inline unsigned type in##bwl(int port) \
+{ \
+ unsigned type value; \
+ asm volatile("in" #bwl " %w1, %" #bw "0" \
+ : "=a"(value) : "Nd"(port)); \
+ return value; \
+}
+
+BUILDIO(b, b, char)
+BUILDIO(w, w, short)
+BUILDIO(l, , int)
+#undef BUILDIO
+
+#define inb inb
+#define inw inw
+#define inl inl
+#define outb outb
+#define outw outw
+#define outl outl
+
+#endif
--
2.34.1

2022-01-24 19:28:14

[permalink] [raw]

Subject: [PATCHv2 20/29] x86/tdx: Get page shared bit info from the TDX module

Intel TDX doesn't allow VMM to access guest private memory. Any memory
that is required for communication with the VMM must be shared
explicitly by setting a bit in the page table entry. Details about
which bit in the page table entry to be used to indicate shared/private
state can be determined by using the TDINFO TDCALL (call to TDX
module).

Fetch and save the guest TD execution environment information at
initialization time. The next patch will use the information.

More details about the TDINFO TDCALL can be found in
Guest-Host-Communication Interface (GHCI) for Intel Trust Domain
Extensions (Intel TDX) specification, sec titled "TDCALL[TDINFO]".

Co-developed-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/kernel/tdx.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index a4e696f12666..b27c4261bfd2 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -11,6 +11,7 @@
#include <asm/insn-eval.h>

/* TDX module Call Leaf IDs */
+#define TDX_GET_INFO 1
#define TDX_GET_VEINFO 3

/* See Exit Qualification for I/O Instructions in VMX documentation */
@@ -19,6 +20,12 @@
#define VE_GET_PORT_NUM(exit_qual) ((exit_qual) >> 16)
#define VE_IS_IO_STRING(exit_qual) ((exit_qual) & 16 ? 1 : 0)

+/* Guest TD execution environment information */
+static struct {
+ unsigned int gpa_width;
+ unsigned long attributes;
+} td_info __ro_after_init;
+
static bool tdx_guest_detected __ro_after_init;

/*
@@ -59,6 +66,28 @@ long tdx_kvm_hypercall(unsigned int nr, unsigned long p1, unsigned long p2,
EXPORT_SYMBOL_GPL(tdx_kvm_hypercall);
#endif

+static void tdx_get_info(void)
+{
+ struct tdx_module_output out;
+ u64 ret;
+
+ /*
+ * TDINFO TDX module call is used to get the TD execution environment
+ * information like GPA width, number of available vcpus, debug mode
+ * information, etc. More details about the ABI can be found in TDX
+ * Guest-Host-Communication Interface (GHCI), sec 2.4.2 TDCALL
+ * [TDG.VP.INFO].
+ */
+ ret = __tdx_module_call(TDX_GET_INFO, 0, 0, 0, 0, &out);
+
+ /* Non zero return value indicates buggy TDX module, so panic */
+ if (ret)
+ panic("TDINFO TDCALL failed (Buggy TDX module!)\n");
+
+ td_info.gpa_width = out.rcx & GENMASK(5, 0);
+ td_info.attributes = out.rdx;
+}
+
static u64 __cpuidle _tdx_halt(const bool irq_disabled, const bool do_sti)
{
/*
@@ -455,5 +484,7 @@ void __init tdx_early_init(void)

setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);

+ tdx_get_info();
+
pr_info("Guest detected\n");
}
--
2.34.1

2022-02-02 10:43:34

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCHv2 20/29] x86/tdx: Get page shared bit info from the TDX module

On Mon, Jan 24 2022 at 18:02, Kirill A. Shutemov wrote:
> +static void tdx_get_info(void)
> +{
> + struct tdx_module_output out;
> + u64 ret;
> +
> + /*
> + * TDINFO TDX module call is used to get the TD execution environment
> + * information like GPA width, number of available vcpus, debug mode
> + * information, etc. More details about the ABI can be found in TDX
> + * Guest-Host-Communication Interface (GHCI), sec 2.4.2 TDCALL
> + * [TDG.VP.INFO].
> + */
> + ret = __tdx_module_call(TDX_GET_INFO, 0, 0, 0, 0, &out);
> +
> + /* Non zero return value indicates buggy TDX module, so panic */

Can you please get rid of these useless comments all over the place. The
panic() message tells the same story. Please document the non-obvious
things.

Thanks,

tglx

2022-02-03 14:47:41

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCHv2 10/29] x86: Consolidate port I/O helpers

On Mon, Jan 24 2022 at 18:01, Kirill A. Shutemov wrote:
> There are two implementations of port I/O helpers: one in the kernel and
> one in the boot stub.
>
> Move the helpers required for both to <asm/shared/io.h> and use the one
> implementation everywhere.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>

Reviewed-by: Thomas Gleixner <[email protected]>

2022-02-08 13:18:05

[permalink] [raw]

Subject: Re: [PATCHv2 20/29] x86/tdx: Get page shared bit info from the TDX module

On Mon, Jan 24, 2022 at 06:02:06PM +0300, Kirill A. Shutemov wrote:
> @@ -59,6 +66,28 @@ long tdx_kvm_hypercall(unsigned int nr, unsigned long p1, unsigned long p2,
> EXPORT_SYMBOL_GPL(tdx_kvm_hypercall);
> #endif
>
> +static void tdx_get_info(void)

Btw, can we strip the "tdx_" prefix off from all those static
functions... there's an overload of "tdx" prefixes when looking at
the code and it would be easier on the eyes if they're only with the
external interfaces... it also helps differentiate which is which.

Thx.

--
Regards/Gruss,
Boris.

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

2022-02-09 11:47:31

[permalink] [raw]

Subject: Re: [PATCHv2 20/29] x86/tdx: Get page shared bit info from the TDX module

On Wed, Feb 02, 2022, Thomas Gleixner wrote:
> On Mon, Jan 24 2022 at 18:02, Kirill A. Shutemov wrote:
> > +static void tdx_get_info(void)
> > +{
> > + struct tdx_module_output out;
> > + u64 ret;
> > +
> > + /*
> > + * TDINFO TDX module call is used to get the TD execution environment
> > + * information like GPA width, number of available vcpus, debug mode
> > + * information, etc. More details about the ABI can be found in TDX
> > + * Guest-Host-Communication Interface (GHCI), sec 2.4.2 TDCALL
> > + * [TDG.VP.INFO].
> > + */
> > + ret = __tdx_module_call(TDX_GET_INFO, 0, 0, 0, 0, &out);
> > +
> > + /* Non zero return value indicates buggy TDX module, so panic */
>
> Can you please get rid of these useless comments all over the place. The
> panic() message tells the same story. Please document the non-obvious
> things.

And why isn't there a tdx_module_call() wrapper to panic() on failure? IIRC,
that's why the asm routines had the double underscore, but that detail appears
to have been lost. E.g. __tdx_module_call(TDX_GET_VEINFO, ...) in patch 04 should
also panic, but it currently morphs the #VE into a #GP if it can't retrieve the
info, which will lead to weird "#GPs" on things like vanilla MOV instructions if
something does go wrong. TDX_ACCEPT_PAGE is the only call into the TDX Module
for which failure is not fatal.

2022-02-09 12:24:07

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

> 60 files changed, 2079 insertions(+), 142 deletions(-)
> create mode 100644 Documentation/x86/tdx.rst
> create mode 100644 arch/x86/boot/compressed/tdcall.S
> create mode 100644 arch/x86/boot/compressed/tdx.c
> create mode 100644 arch/x86/boot/compressed/tdx.h
> create mode 100644 arch/x86/boot/io.h
> create mode 100644 arch/x86/include/asm/shared/io.h
> create mode 100644 arch/x86/include/asm/shared/tdx.h
> create mode 100644 arch/x86/include/asm/tdx.h
> create mode 100644 arch/x86/kernel/tdcall.S
> create mode 100644 arch/x86/kernel/tdx.c
>

Hi,

Is it better to change the file name(s) to reflect they are for TDX guest
support, for instance, especially the last one arch/x86/kernel/tdx.c?

TDX host support basically does detection of SEAM, TDX KeyIDs, P-SEAMLDR and
initialize the TDX module, so likely TDX host support will introduce couple of
new files to do above things respectively, and the majority of the code could be
self-contained under some directory (currently under arch/x86/kernel/cpu/tdx/,
but can be changed of course). Could we have some suggestions on how to
organize?

Thanks,
-Kai

2022-02-09 12:30:02

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Thu, Feb 10, 2022 at 12:48:31AM +1300, Kai Huang wrote:
> Are you suggesting even for now we can start to put TDX host support to
> arch/x86/coco/tdx/ ?

That's exactly what I'm suggesting. The TDX stuff is not upstream so
nothing's cast in stone yet. This way there won't be any unpleasant code
movements later.

But let's wait to see what the bikeshed discussion will bring first and
then start moving files.

Thx.

--
Regards/Gruss,
Boris.

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

2022-02-09 12:33:43

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Thu, Feb 10, 2022 at 12:30:33AM +1300, Kai Huang wrote:
> Because SEAM, P-SEAMLDR can logically be independent, so I feel it's better to
> have separate C files for them.

Most of those look like small files. I don't see the point of having it
all in separate files - you can just as well put them in tdx.c and carve
out only then when the file becomes too unwieldy to handle.

> Thanks for the information. However, for now does it make sense to also put
> TDX host files under arch/x86/kernel/, or maybe arch/x86/kernel/tdx_host/?

Didn't you just read what I wrote about "kernel"?

> As suggested by Thomas, host SEAMCALL can share TDX guest's __tdx_module_call()
> implementation. Kirill will have a arch/x86/kernel/tdxcall.S which implements
> the core body of __tdx_module_call() and is supposed to be included by the new
> assembly file to implement the host SEAMCALL function. From this perspective,
> it seems more reasonable to just put all TDX host files under arch/x86/kernel/?

It would be a lot harder to move them to a different location later,
when they're upstream already. I'm talking from past experience here.

But let's see what the others think first.

--
Regards/Gruss,
Boris.

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

2022-02-09 13:53:40

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, Feb 09, 2022 at 11:56:13PM +1300, Kai Huang wrote:
> TDX host support basically does detection of SEAM, TDX KeyIDs, P-SEAMLDR and
> initialize the TDX module, so likely TDX host support will introduce couple of
> new files to do above things respectively,

Why a couple of new files? How much code is that?

> and the majority of the code could be self-contained under some
> directory (currently under arch/x86/kernel/cpu/tdx/, but can be
> changed of course). Could we have some suggestions on how to organize?

So we slowly try to move stuff away from arch/x86/kernel/ as that is a
dumping ground for everything and everything there is "kernel" so that
part of the path is kinda redundant.

That's why, for example, we stuck the entry code under arch/x86/entry/.

I'm thinking long term we probably should stick all confidentail
computing stuff under its own folder:

arch/x86/coco/

for example. The "coco" being COnfidential COmputing, for lack of a
better idea.

And there you'll have

arch/x86/coco/tdx and
arch/x86/coco/sev

where to we'll start migrating the AMD stuff eventually too.

Methinks.

--
Regards/Gruss,
Boris.

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

2022-02-09 14:42:45

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, 9 Feb 2022 12:56:26 +0100 Borislav Petkov wrote:
> On Thu, Feb 10, 2022 at 12:48:31AM +1300, Kai Huang wrote:
> > Are you suggesting even for now we can start to put TDX host support to
> > arch/x86/coco/tdx/ ?
>
> That's exactly what I'm suggesting. The TDX stuff is not upstream so
> nothing's cast in stone yet. This way there won't be any unpleasant code
> movements later.
>
> But let's wait to see what the bikeshed discussion will bring first and
> then start moving files.
>

Sure. Thanks.

2022-02-09 17:05:02

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, Feb 09, 2022, Borislav Petkov wrote:
> On Thu, Feb 10, 2022 at 12:48:31AM +1300, Kai Huang wrote:
> > Are you suggesting even for now we can start to put TDX host support to
> > arch/x86/coco/tdx/ ?
>
> That's exactly what I'm suggesting. The TDX stuff is not upstream so
> nothing's cast in stone yet. This way there won't be any unpleasant code
> movements later.

I strongly prefer we put the guest and host code in separate directories. Both
TDX and SEV are big enough that they'll benefit from splitting up files, having
to fight over file names or tag all files with guest/host will get annoying.

I do like the idea of arch/x86/coco though. The most straightforward approach
would be:

arch/x86/coco/guest/
arch/x86/coco/host/

but that doesn't provide any extensibility on the host virtualization side, e.g.
to land non-coco, non-KVM-specific host virtualization code (we have a potential
use case for this). If that happens, we'd end up with x86 KVM having code and
dependencies split across:

arch/x86/coco/host
arch/x86/kvm/
arch/x86/???/

An alternative idea would be to mirror what generic KVM does (virt/kvm/), and do:

arch/x86/coco/<guest stuff>
arch/x86/virt/<"generic" x86 host virtualization stuff>
arch/x86/virt/coco/<host coco stuff>
arch/x86/virt/kvm/

Though I can already hear the stable trees and downstream kernels crying out in
horror at moving arch/x86/kvm :-)

2022-02-09 18:29:39

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, 9 Feb 2022 12:08:18 +0100 Borislav Petkov wrote:
> On Wed, Feb 09, 2022 at 11:56:13PM +1300, Kai Huang wrote:
> > TDX host support basically does detection of SEAM, TDX KeyIDs, P-SEAMLDR and
> > initialize the TDX module, so likely TDX host support will introduce couple of
> > new files to do above things respectively,
>
> Why a couple of new files? How much code is that?

This is the fine names and code size of current internal version that I have:

.../admin-guide/kernel-parameters.txt | 6 +
Documentation/x86/index.rst | 1 +
Documentation/x86/intel-tdx.rst | 259 +++++++
arch/x86/Kconfig | 14 +
arch/x86/include/asm/seam.h | 213 ++++++
arch/x86/include/asm/tdx_host.h | 20 +
arch/x86/kernel/asm-offsets_64.c | 18 +
arch/x86/kernel/cpu/Makefile | 1 +
arch/x86/kernel/cpu/intel.c | 6 +
arch/x86/kernel/cpu/tdx/Makefile | 1 +
arch/x86/kernel/cpu/tdx/p-seamldr.c | 109 +++
arch/x86/kernel/cpu/tdx/p-seamldr.h | 14 +
arch/x86/kernel/cpu/tdx/seam.c | 105 +++
arch/x86/kernel/cpu/tdx/seamcall.S | 80 ++
arch/x86/kernel/cpu/tdx/tdmr.c | 581 ++++++++++++++
arch/x86/kernel/cpu/tdx/tdmr.h | 28 +
arch/x86/kernel/cpu/tdx/tdx.c | 707 ++++++++++++++++++
arch/x86/kernel/cpu/tdx/tdx_arch.h | 88 +++
arch/x86/kernel/cpu/tdx/tdx_seamcall.h | 138 ++++
19 files changed, 2389 insertions(+)
create mode 100644 Documentation/x86/intel-tdx.rst
create mode 100644 arch/x86/include/asm/seam.h
create mode 100644 arch/x86/include/asm/tdx_host.h
create mode 100644 arch/x86/kernel/cpu/tdx/Makefile
create mode 100644 arch/x86/kernel/cpu/tdx/p-seamldr.c
create mode 100644 arch/x86/kernel/cpu/tdx/p-seamldr.h
create mode 100644 arch/x86/kernel/cpu/tdx/seam.c
create mode 100644 arch/x86/kernel/cpu/tdx/seamcall.S
create mode 100644 arch/x86/kernel/cpu/tdx/tdmr.c
create mode 100644 arch/x86/kernel/cpu/tdx/tdmr.h
create mode 100644 arch/x86/kernel/cpu/tdx/tdx.c
create mode 100644 arch/x86/kernel/cpu/tdx/tdx_arch.h
create mode 100644 arch/x86/kernel/cpu/tdx/tdx_seamcall.h

Because SEAM, P-SEAMLDR can logically be independent, so I feel it's better to
have separate C files for them. TDMR (TD Memory Region, which is the structure
defined by TDX architecture to manage usable TDX memory) is split out as a
separate file as the logic to deal with it requires non-trival LOC too.

>
> > and the majority of the code could be self-contained under some
> > directory (currently under arch/x86/kernel/cpu/tdx/, but can be
> > changed of course). Could we have some suggestions on how to organize?
>
> So we slowly try to move stuff away from arch/x86/kernel/ as that is a
> dumping ground for everything and everything there is "kernel" so that
> part of the path is kinda redundant.
>
> That's why, for example, we stuck the entry code under arch/x86/entry/.
>
> I'm thinking long term we probably should stick all confidentail
> computing stuff under its own folder:
>
> arch/x86/coco/
>
> for example. The "coco" being COnfidential COmputing, for lack of a
> better idea.
>
> And there you'll have
>
> arch/x86/coco/tdx and
> arch/x86/coco/sev
>
> where to we'll start migrating the AMD stuff eventually too.

Thanks for the information. However, for now does it make sense to also put
TDX host files under arch/x86/kernel/, or maybe arch/x86/kernel/tdx_host/?

As suggested by Thomas, host SEAMCALL can share TDX guest's __tdx_module_call()
implementation. Kirill will have a arch/x86/kernel/tdxcall.S which implements
the core body of __tdx_module_call() and is supposed to be included by the new
assembly file to implement the host SEAMCALL function. From this perspective,
it seems more reasonable to just put all TDX host files under arch/x86/kernel/?

Thanks in advance.

2022-02-09 19:37:38

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, 9 Feb 2022 12:40:17 +0100 Borislav Petkov wrote:
> On Thu, Feb 10, 2022 at 12:30:33AM +1300, Kai Huang wrote:
> > Because SEAM, P-SEAMLDR can logically be independent, so I feel it's better to
> > have separate C files for them.
>
> Most of those look like small files. I don't see the point of having it
> all in separate files - you can just as well put them in tdx.c and carve
> out only then when the file becomes too unwieldy to handle.

arch/x86/kernel/tdx.c is already taken by this series. This is the reason that
I think perhaps it's better to rename it to reflect it is for TDX guest support.

>
> > Thanks for the information. However, for now does it make sense to also put
> > TDX host files under arch/x86/kernel/, or maybe arch/x86/kernel/tdx_host/?
>
> Didn't you just read what I wrote about "kernel"?
>
> > As suggested by Thomas, host SEAMCALL can share TDX guest's __tdx_module_call()
> > implementation. Kirill will have a arch/x86/kernel/tdxcall.S which implements
> > the core body of __tdx_module_call() and is supposed to be included by the new
> > assembly file to implement the host SEAMCALL function. From this perspective,
> > it seems more reasonable to just put all TDX host files under arch/x86/kernel/?
>
> It would be a lot harder to move them to a different location later,
> when they're upstream already. I'm talking from past experience here.

Are you suggesting even for now we can start to put TDX host support to
arch/x86/coco/tdx/ ?

>
> But let's see what the others think first.

Sure thanks for comments.

>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

2022-02-09 20:01:42

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, Feb 09, 2022 at 04:50:08PM +0000, Sean Christopherson wrote:
> On Wed, Feb 09, 2022, Borislav Petkov wrote:
> > On Thu, Feb 10, 2022 at 12:48:31AM +1300, Kai Huang wrote:
> > > Are you suggesting even for now we can start to put TDX host support to
> > > arch/x86/coco/tdx/ ?
> >
> > That's exactly what I'm suggesting. The TDX stuff is not upstream so
> > nothing's cast in stone yet. This way there won't be any unpleasant code
> > movements later.
>
> I strongly prefer we put the guest and host code in separate directories. Both
> TDX and SEV are big enough that they'll benefit from splitting up files, having
> to fight over file names or tag all files with guest/host will get annoying.
>
> I do like the idea of arch/x86/coco though. The most straightforward approach
> would be:
>
> arch/x86/coco/guest/
> arch/x86/coco/host/
>
> but that doesn't provide any extensibility on the host virtualization side, e.g.
> to land non-coco, non-KVM-specific host virtualization code (we have a potential
> use case for this). If that happens, we'd end up with x86 KVM having code and
> dependencies split across:
>
> arch/x86/coco/host
> arch/x86/kvm/
> arch/x86/???/
>
> An alternative idea would be to mirror what generic KVM does (virt/kvm/), and do:
>
> arch/x86/coco/<guest stuff>
> arch/x86/virt/<"generic" x86 host virtualization stuff>
> arch/x86/virt/coco/<host coco stuff>
> arch/x86/virt/kvm/
>
> Though I can already hear the stable trees and downstream kernels crying out in
> horror at moving arch/x86/kvm :-)

Hmmm, so I am still thinking about guest-only when we're talking about
arch/x86/coco/.

Lemme look at the other virt things:

the kvm host virt stuff is in:

arch/x86/kvm/

(btw, this is where the SEV host stuff is: arch/x86/kvm/svm/sev.c)

arch/x86/hyperv/ - looks like hyperv guest stuff

arch/x86/xen/ - xen guest stuff

arch/x86/kernel/cpu/vmware.c - vmware guest stuff

arch/x86/kernel/cpu/acrn.c - Acorn guest stuff

So we have a real mess. :-(

Not surprised though. So that last thing you're suggesting kinda makes
sense but lemme tweak it a bit:

arch/x86/coco/<guest stuff>
arch/x86/virt/<"generic" x86 host virtualization stuff>
arch/x86/virt/tdx/ - no need for the "coco" thing - TDX is nothing but coco. TDX host
stuff

arch/x86/virt/sev/ - ditto

and we'll keep arch/x86/kvm because of previous precedents with other
things I've enumerated above.

Hmmm?

--
Regards/Gruss,
Boris.

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

2022-02-09 20:50:20

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, Feb 09, 2022, Borislav Petkov wrote:
> On Wed, Feb 09, 2022 at 04:50:08PM +0000, Sean Christopherson wrote:
> > An alternative idea would be to mirror what generic KVM does (virt/kvm/), and do:
> >
> > arch/x86/coco/<guest stuff>
> > arch/x86/virt/<"generic" x86 host virtualization stuff>
> > arch/x86/virt/coco/<host coco stuff>
> > arch/x86/virt/kvm/
> >
> > Though I can already hear the stable trees and downstream kernels crying out in
> > horror at moving arch/x86/kvm :-)
>
> Hmmm, so I am still thinking about guest-only when we're talking about
> arch/x86/coco/.
>
> Lemme look at the other virt things:
>
> the kvm host virt stuff is in:
>
> arch/x86/kvm/
>
> (btw, this is where the SEV host stuff is: arch/x86/kvm/svm/sev.c)
>
> arch/x86/hyperv/ - looks like hyperv guest stuff
>
> arch/x86/xen/ - xen guest stuff
>
> arch/x86/kernel/cpu/vmware.c - vmware guest stuff
>
> arch/x86/kernel/cpu/acrn.c - Acorn guest stuff
>
> So we have a real mess. :-(

Don't forget :-)

arch/x86/kernel/kvm.c - KVM guest stuff

> Not surprised though. So that last thing you're suggesting kinda makes
> sense but lemme tweak it a bit:
>
> arch/x86/coco/<guest stuff>
> arch/x86/virt/<"generic" x86 host virtualization stuff>
> arch/x86/virt/tdx/ - no need for the "coco" thing - TDX is nothing but coco. TDX host
> stuff
>
> arch/x86/virt/sev/ - ditto
>
> and we'll keep arch/x86/kvm because of previous precedents with other
> things I've enumerated above.
>
> Hmmm?

No objection to omitting "coco". Though what about using "vmx" and "svm" instead
of "tdx" and "sev". We lose the more explicit tie to coco, but it would mirror the
sub-directories in arch/x86/kvm/ and would avoid a mess in the scenario where tdx
or sev needs to share code with the non-coco side, e.g. I'm guessing TDX will need
to do VMXON.

arch/x86/virt/vmx/
tdx.c
vmx.c

arch/x86/virt/svm/
sev.c
sev-es.c
sev-snp.c
svm.c

2022-02-09 23:56:14

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

+ SEV guys. You can scroll upthread to read up on the context.

On Wed, Feb 09, 2022 at 08:07:52PM +0000, Sean Christopherson wrote:
> Don't forget :-)
>
> arch/x86/kernel/kvm.c - KVM guest stuff

I knew I'd miss something, ofc.

> No objection to omitting "coco". Though what about using "vmx" and "svm" instead
> of "tdx" and "sev".

I'm not dead-set on this but ...

> We lose the more explicit tie to coco, but it would mirror the
> sub-directories in arch/x86/kvm/

... having them too close in naming to the non-coco stuff, might cause
confusion when looking at:

arch/x86/kvm/vmx/vmx.c

vs

arch/x86/virt/vmx/vmx.c

Instead of having

arch/x86/kvm/vmx/vmx.c

and

arch/x86/virt/tdx/vmx.c

That second version differs just the right amount. :-)

> and would avoid a mess in the scenario where tdx
> or sev needs to share code with the non-coco side, e.g. I'm guessing TDX will need
> to do VMXON.
>
> arch/x86/virt/vmx/
> tdx.c
> vmx.c
>
> arch/x86/virt/svm/
> sev.c
> sev-es.c
> sev-snp.c
> svm.c

That will probably be two files too: sev.c and svm.c

But let's see what the other folks think first...

--
Regards/Gruss,
Boris.

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

2022-02-10 02:11:14

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

> > No objection to omitting "coco". Though what about using "vmx" and "svm" instead
> > of "tdx" and "sev".
>
> I'm not dead-set on this but ...
>
> > We lose the more explicit tie to coco, but it would mirror the
> > sub-directories in arch/x86/kvm/
>
> ... having them too close in naming to the non-coco stuff, might cause
> confusion when looking at:
>
> arch/x86/kvm/vmx/vmx.c
>
> vs
>
> arch/x86/virt/vmx/vmx.c
>
> Instead of having
>
> arch/x86/kvm/vmx/vmx.c
>
> and
>
> arch/x86/virt/tdx/vmx.c
>
> That second version differs just the right amount. :-)

Having vmx.c under tdx/ directory looks a little bit strange.

vmx.c seems more like "generic non-KVM host virtualization staff".

>
> > and would avoid a mess in the scenario where tdx
> > or sev needs to share code with the non-coco side, e.g. I'm guessing TDX will need
> > to do VMXON.
> >
> > arch/x86/virt/vmx/
> > tdx.c
> > vmx.c
> >
> > arch/x86/virt/svm/
> > sev.c
> > sev-es.c
> > sev-snp.c
> > svm.c
>
> That will probably be two files too: sev.c and svm.c
>
> But let's see what the other folks think first...
>

So if I catch you guys correctly, so far I am heading towards to:

arch/x86/virt/vmx/
tdx.c

("vmx/" can be changed if you guys prefers others later).

And I am targeting to use single tdx.c to hold ~2k LoC since looks like single
file is preferred.

2022-02-16 16:40:21

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Thu, Feb 10, 2022, Kai Huang wrote:
>
> > > No objection to omitting "coco". Though what about using "vmx" and "svm" instead
> > > of "tdx" and "sev".
> >
> > I'm not dead-set on this but ...
> >
> > > We lose the more explicit tie to coco, but it would mirror the
> > > sub-directories in arch/x86/kvm/
> >
> > ... having them too close in naming to the non-coco stuff, might cause
> > confusion when looking at:
> >
> > arch/x86/kvm/vmx/vmx.c
> >
> > vs
> >
> > arch/x86/virt/vmx/vmx.c
> >
> > Instead of having
> >
> > arch/x86/kvm/vmx/vmx.c
> >
> > and
> >
> > arch/x86/virt/tdx/vmx.c
> >
> > That second version differs just the right amount. :-)
>
> Having vmx.c under tdx/ directory looks a little bit strange.

Yeah, it's inverted. TDX is a built on top of VMX. If/when we end up with stuff
that is relevant to VMX but not TDX, then we'll be referencing tdx/ for things
that that don't care at all about TDX.

2022-02-16 17:41:59

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Wed, Feb 09, 2022 at 09:36:57PM +0100, Borislav Petkov wrote:
> + SEV guys. You can scroll upthread to read up on the context.
>
> On Wed, Feb 09, 2022 at 08:07:52PM +0000, Sean Christopherson wrote:
> > Don't forget :-)
> >
> > arch/x86/kernel/kvm.c - KVM guest stuff
>
> I knew I'd miss something, ofc.
>
> > No objection to omitting "coco". Though what about using "vmx" and "svm" instead
> > of "tdx" and "sev".
>
> I'm not dead-set on this but ...
>
> > We lose the more explicit tie to coco, but it would mirror the
> > sub-directories in arch/x86/kvm/
>
> ... having them too close in naming to the non-coco stuff, might cause
> confusion when looking at:
>
> arch/x86/kvm/vmx/vmx.c
>
> vs
>
> arch/x86/virt/vmx/vmx.c
>
> Instead of having
>
> arch/x86/kvm/vmx/vmx.c
>
> and
>
> arch/x86/virt/tdx/vmx.c
>
> That second version differs just the right amount. :-)
>
> > and would avoid a mess in the scenario where tdx
> > or sev needs to share code with the non-coco side, e.g. I'm guessing TDX will need
> > to do VMXON.
> >
> > arch/x86/virt/vmx/
> > tdx.c
> > vmx.c
> >
> > arch/x86/virt/svm/
> > sev.c
> > sev-es.c
> > sev-snp.c
> > svm.c
>
> That will probably be two files too: sev.c and svm.c
>
> But let's see what the other folks think first...

So, any conclusion?

I want to understand where to land TDX guest code and host-guest shared TDX code.
Host-guest shared code doesn't seem to fit anywhere nicely.

Or should I leave it under arch/x86/kernel until decision is made?

--
Kirill A. Shutemov

2022-02-17 16:57:01

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Thu, Feb 17, 2022 at 06:26:13PM +0300, Kirill A. Shutemov wrote:
> Okay, so on TDX guest side I would have
>
> arch/x86/kernel/tdx.c => arch/x86/coco/tdx.c

Right, and to answer a previous question: if that file starts becoming
too huge and unwieldy then we should split it, by all means. What I
don't like is getting a bunch of small files with no good reason why.

> arch/x86/kernel/tdcall.S => arch/x86/coco/tdcall.S
> arch/x86/kernel/tdxcall.S => arch/x86/virt/tdxcall.S
>
> The last one going to be used by TDX host as well to define SEMACALL
> helper.
>
> Looks good?

Right, that looks neat.

Thx.

--
Regards/Gruss,
Boris.

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

2022-02-17 23:40:00

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Thu, Feb 17, 2022, Borislav Petkov wrote:
> On Wed, Feb 16, 2022 at 06:48:09PM +0300, Kirill A. Shutemov wrote:
> > So, any conclusion?
>
> Lemme type the whole thing here again so that we have it all summed up
> in one place - I think we all agree by now:
>
> - confidential computing guest stuff: arch/x86/coco/{sev,tdx}
> - generic host virtualization stuff: arch/x86/virt/
> - coco host stuff: arch/x86/virt/vmx/{tdx,vmx}.c and arch/x86/virt/svm/sev*.c

LGTM

> New stuff goes to the new paths - i.e., TDX guest, host, etc - old stuff
> - AMD SEV/SNP will get moved gradually so that development doesn't get
> disrupted. Or we can do a flag day, right before -rc1 or so, and move it
> all so in one go. We'll see.

FWIW, I don't think there's much existing SEV host virtualization stuff that can
be moved without first extracting and decoupling it from KVM, which will be
non-trivial. I do want to do that some day, but it definitely shouldn't hold up
merging SNP.

2022-02-17 23:59:55

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Thu, Feb 17, 2022 at 03:29:00PM +0000, Sean Christopherson wrote:
> FWIW, I don't think there's much existing SEV host virtualization stuff that can
> be moved without first extracting and decoupling it from KVM, which will be
> non-trivial. I do want to do that some day, but it definitely shouldn't hold up
> merging SNP.

Oh sure, code movement is lowest prio so whenever you feel like it.

--
Regards/Gruss,
Boris.

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

2022-02-18 00:16:08

[permalink] [raw]

Subject: Re: [PATCHv2 00/29] TDX Guest: TDX core support

On Thu, Feb 17, 2022 at 04:19:35PM +0100, Borislav Petkov wrote:
> On Wed, Feb 16, 2022 at 06:48:09PM +0300, Kirill A. Shutemov wrote:
> > So, any conclusion?
>
> Lemme type the whole thing here again so that we have it all summed up
> in one place - I think we all agree by now:
>
> - confidential computing guest stuff: arch/x86/coco/{sev,tdx}
> - generic host virtualization stuff: arch/x86/virt/
> - coco host stuff: arch/x86/virt/vmx/{tdx,vmx}.c and arch/x86/virt/svm/sev*.c
>
> New stuff goes to the new paths - i.e., TDX guest, host, etc - old stuff
> - AMD SEV/SNP will get moved gradually so that development doesn't get
> disrupted. Or we can do a flag day, right before -rc1 or so, and move it
> all so in one go. We'll see.

Okay, so on TDX guest side I would have

arch/x86/kernel/tdx.c => arch/x86/coco/tdx.c
arch/x86/kernel/tdcall.S => arch/x86/coco/tdcall.S
arch/x86/kernel/tdxcall.S => arch/x86/virt/tdxcall.S

The last one going to be used by TDX host as well to define SEMACALL
helper.

Looks good?

--
Kirill A. Shutemov

2022-02-18 00:19:32