diff mbox series

[tip:,x86/urgent] x86/tdx: Prepare for using "INFO" call for a second purpose

Message ID 166734513630.7716.12952231613533508782.tip-bot2@tip-bot2
State New
Headers
Series [tip:,x86/urgent] x86/tdx: Prepare for using "INFO" call for a second purpose |

Commit Message

tip-bot2 for Thomas Gleixner Nov. 1, 2022, 11:25 p.m. UTC 
  The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     a6dd6f39008bb3ef7c73ef0a2acc2a4209555bd8
Gitweb:        https://git.kernel.org/tip/a6dd6f39008bb3ef7c73ef0a2acc2a4209555bd8
Author:        Dave Hansen <dave.hansen@linux.intel.com>
AuthorDate:    Fri, 28 Oct 2022 17:12:19 +03:00
Committer:     Dave Hansen <dave.hansen@linux.intel.com>
CommitterDate: Tue, 01 Nov 2022 10:07:15 -07:00

x86/tdx: Prepare for using "INFO" call for a second purpose

The TDG.VP.INFO TDCALL provides the guest with various details about
the TDX system that the guest needs to run.  Only one field is currently
used: 'gpa_width' which tells the guest which PTE bits mark pages shared
or private.

A second field is now needed: the guest "TD attributes" to tell if
virtualization exceptions are configured in a way that can harm the guest.

Make the naming and calling convention more generic and discrete from the
mask-centric one.

Thanks to Sathya for the inspiration here, but there's no code, comments
or changelogs left from where he started.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: stable@vger.kernel.org
---
 arch/x86/coco/tdx/tdx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Borislav Petkov Nov. 6, 2022, 12:45 p.m. UTC | #1 
On Tue, Nov 01, 2022 at 11:25:36PM -0000, tip-bot2 for Dave Hansen wrote:
> @@ -121,7 +121,7 @@ static u64 get_cc_mask(void)
>  	 * The highest bit of a guest physical address is the "sharing" bit.
>  	 * Set it for shared pages and clear it for private pages.
>  	 */
> -	return BIT_ULL(gpa_width - 1);
> +	*cc_mask = BIT_ULL(gpa_width - 1);
>  }

I'm looking at the next patch too and I still don't see what the point
is of making it a void?

IOW, what's wrong with doing this?

---
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index b8998cf0508a..0421cb7f3b86 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -100,11 +100,11 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
 }
 
-static void tdx_parse_tdinfo(u64 *cc_mask)
+static u64 tdx_parse_tdinfo(void)
 {
 	struct tdx_module_output out;
 	unsigned int gpa_width;
-	u64 td_attr;
+	u64 td_attr, ret;
 
 	/*
 	 * TDINFO TDX module call is used to get the TD execution environment
@@ -123,7 +123,7 @@ static void tdx_parse_tdinfo(u64 *cc_mask)
 	 * can not meaningfully run without it.
 	 */
 	gpa_width = out.rcx & GENMASK(5, 0);
-	*cc_mask = BIT_ULL(gpa_width - 1);
+	ret = BIT_ULL(gpa_width - 1);
 
 	/*
 	 * The kernel can not handle #VE's when accessing normal kernel
@@ -133,6 +133,8 @@ static void tdx_parse_tdinfo(u64 *cc_mask)
 	td_attr = out.rdx;
 	if (!(td_attr & ATTR_SEPT_VE_DISABLE))
 		panic("TD misconfiguration: SEPT_VE_DISABLE attibute must be set.\n");
+
+	return ret;
 }
 
 /*
@@ -769,7 +771,7 @@ void __init tdx_early_init(void)
 	setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
 
 	cc_set_vendor(CC_VENDOR_INTEL);
-	tdx_parse_tdinfo(&cc_mask);
+	cc_mask = tdx_parse_tdinfo();
 	cc_set_mask(cc_mask);
 
 	/*
Dave Hansen Nov. 6, 2022, 5:02 p.m. UTC | #2 
On 11/6/22 04:45, Borislav Petkov wrote:
> On Tue, Nov 01, 2022 at 11:25:36PM -0000, tip-bot2 for Dave Hansen wrote:
>> @@ -121,7 +121,7 @@ static u64 get_cc_mask(void)
>>  	 * The highest bit of a guest physical address is the "sharing" bit.
>>  	 * Set it for shared pages and clear it for private pages.
>>  	 */
>> -	return BIT_ULL(gpa_width - 1);
>> +	*cc_mask = BIT_ULL(gpa_width - 1);
>>  }
> I'm looking at the next patch too and I still don't see what the point
> is of making it a void?
> 
> IOW, what's wrong with doing this?

It's fine for now, except that the naming on this:

-	tdx_parse_tdinfo(&cc_mask);
+	cc_mask = tdx_parse_tdinfo();

is a bit funky since tdx_parse_tdinfo() is doing a couple of things and
will need to return a second item shortly.

But, zero objections if you want to make it that way for now.
Borislav Petkov Nov. 6, 2022, 7:50 p.m. UTC | #3 
On Sun, Nov 06, 2022 at 09:02:27AM -0800, Dave Hansen wrote:
> It's fine for now, except that the naming on this:
> 
> -	tdx_parse_tdinfo(&cc_mask);
> +	cc_mask = tdx_parse_tdinfo();
> 
> is a bit funky since tdx_parse_tdinfo() is doing a couple of things

Yeah, that was the next thing that was bothering me.

> and will need to return a second item shortly.

Well, then rename this one back to get_cc_mask() and have a new function
return the second item?
Dave Hansen Nov. 7, 2022, 1:26 p.m. UTC | #4 
On 11/6/22 11:50, Borislav Petkov wrote:
> On Sun, Nov 06, 2022 at 09:02:27AM -0800, Dave Hansen wrote:
>> It's fine for now, except that the naming on this:
>>
>> -	tdx_parse_tdinfo(&cc_mask);
>> +	cc_mask = tdx_parse_tdinfo();
>>
>> is a bit funky since tdx_parse_tdinfo() is doing a couple of things
> Yeah, that was the next thing that was bothering me.
> 
>> and will need to return a second item shortly.
> Well, then rename this one back to get_cc_mask() and have a new function
> return the second item?

That's doable.  It would look something like what I've attached for now.
 The only downside to this is making two tdx_module_call(TDX_GET_INFO...)
calls.  That seems a bit wasteful, but it's not the end of the world.
It would look something like the attached patch.

I kinda like the idea of making one tdx_module_call() and parsing it all
in one place.  The calls are kinda slow, but two of them versus one
isn't going to hurt anybody.

The other thing I considered was keeping a temporary 'struct
tdx_guest_info' structure or something, filling it one, and parsing it
in get_cc_mask() and attribute checking functions.  But, that seemed
like overkill.
diff mbox series

Patch

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 928dcf7..3fee969 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -98,7 +98,7 @@  static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
 }
 
-static u64 get_cc_mask(void)
+static void tdx_parse_tdinfo(u64 *cc_mask)
 {
 	struct tdx_module_output out;
 	unsigned int gpa_width;
@@ -121,7 +121,7 @@  static u64 get_cc_mask(void)
 	 * The highest bit of a guest physical address is the "sharing" bit.
 	 * Set it for shared pages and clear it for private pages.
 	 */
-	return BIT_ULL(gpa_width - 1);
+	*cc_mask = BIT_ULL(gpa_width - 1);
 }
 
 /*
@@ -758,7 +758,7 @@  void __init tdx_early_init(void)
 	setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
 
 	cc_set_vendor(CC_VENDOR_INTEL);
-	cc_mask = get_cc_mask();
+	tdx_parse_tdinfo(&cc_mask);
 	cc_set_mask(cc_mask);
 
 	/*