[PATCH] arch/x86: Set XSS while handling #VC intercept for CPUID

[PATCH] arch/x86: Set XSS while handling #VC intercept for CPUID

[Jinank Jain]

According to [1], while handling the #VC intercept for CPUID leaf
0x0000_000D, we need to supply the value of XSS in the GHCB page. If
this value is not provided then a spec compliant hypervisor can fail the
GHCB request and kill the guest.

[1] https://www.amd.com/system/files/TechDocs/56421-guest-hypervisor-communication-block-standardization.pdf

Signed-off-by: Jinank Jain <jinankjain@linux.microsoft.com>
---
 arch/x86/include/asm/svm.h   | 1 +
 arch/x86/kernel/sev-shared.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 19bf955b67e0..c2f670f7cb47 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -678,5 +678,6 @@ DEFINE_GHCB_ACCESSORS(sw_exit_info_1)
 DEFINE_GHCB_ACCESSORS(sw_exit_info_2)
 DEFINE_GHCB_ACCESSORS(sw_scratch)
 DEFINE_GHCB_ACCESSORS(xcr0)
+DEFINE_GHCB_ACCESSORS(xss)
 
 #endif
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 2eabccde94fb..92350a24848c 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -880,6 +880,9 @@ static enum es_result vc_handle_cpuid(struct ghcb *ghcb,
 	if (snp_cpuid_ret != -EOPNOTSUPP)
 		return ES_VMM_ERROR;
 
+	if (regs->ax == 0xD && regs->cx == 0x1)
+		ghcb_set_xss(ghcb, 0);
+
 	ghcb_set_rax(ghcb, regs->ax);
 	ghcb_set_rcx(ghcb, regs->cx);
 
-- 
2.34.1

Re: [PATCH] arch/x86: Set XSS while handling #VC intercept for CPUID

[Ingo Molnar]

* Jinank Jain <jinankjain@linux.microsoft.com> wrote:

> According to [1], while handling the #VC intercept for CPUID leaf
> 0x0000_000D, we need to supply the value of XSS in the GHCB page. If
> this value is not provided then a spec compliant hypervisor can fail the
> GHCB request and kill the guest.
> 
> [1] https://www.amd.com/system/files/TechDocs/56421-guest-hypervisor-communication-block-standardization.pdf

URL doesn't seem to exist, I get redirected to AMD's 404 page.

Thanks,

	Ingo

Re: [PATCH] arch/x86: Set XSS while handling #VC intercept for CPUID

[Dave Hansen]

On 10/3/23 02:28, Jinank Jain wrote:
...
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 2eabccde94fb..92350a24848c 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -880,6 +880,9 @@ static enum es_result vc_handle_cpuid(struct ghcb *ghcb,
>  	if (snp_cpuid_ret != -EOPNOTSUPP)
>  		return ES_VMM_ERROR;
>  
> +	if (regs->ax == 0xD && regs->cx == 0x1)
> +		ghcb_set_xss(ghcb, 0);

The spec talks about leaf 0xD, but not the subleaf:

> XSS is only required to besupplied when a request forCPUID 0000_000D
> is made andthe guest supports the XSS MSR(0x0000_0DA0).
Why restrict this to subleaf (regx->cx) 1?

Second, XCR0 is being supplied regardless of the CPUID leaf.  Why should
XSS be restricted to 0xD while XCR0 is universally supplied?

Third, why is it OK to supply a garbage (0) value?  If the GHCB field is
required it's surely because the host *NEEDS* the value to do something.
 Won't a garbage value potentially confuse the host?

Re: [PATCH] arch/x86: Set XSS while handling #VC intercept for CPUID

[Tom Lendacky]

On 10/3/23 11:07, Dave Hansen wrote:
> On 10/3/23 02:28, Jinank Jain wrote:
> ...
>> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
>> index 2eabccde94fb..92350a24848c 100644
>> --- a/arch/x86/kernel/sev-shared.c
>> +++ b/arch/x86/kernel/sev-shared.c
>> @@ -880,6 +880,9 @@ static enum es_result vc_handle_cpuid(struct ghcb *ghcb,
>>   	if (snp_cpuid_ret != -EOPNOTSUPP)
>>   		return ES_VMM_ERROR;
>>   
>> +	if (regs->ax == 0xD && regs->cx == 0x1)
>> +		ghcb_set_xss(ghcb, 0);
> 
> The spec talks about leaf 0xD, but not the subleaf:
> 
>> XSS is only required to besupplied when a request forCPUID 0000_000D
>> is made andthe guest supports the XSS MSR(0x0000_0DA0).
> Why restrict this to subleaf (regx->cx) 1?

Today, only subleaf 1 deals with XSS, but we could do just what you say 
and set it for any 0xD subleaf to be safe.

> 
> Second, XCR0 is being supplied regardless of the CPUID leaf.  Why should
> XSS be restricted to 0xD while XCR0 is universally supplied?

XCR0 is really only required for 0xD, I'm not sure why it is being setting 
all the time (unless similar to above, it becomes required for some other 
CPUID leaf in the future?)

> 
> Third, why is it OK to supply a garbage (0) value?  If the GHCB field is
> required it's surely because the host *NEEDS* the value to do something.
>   Won't a garbage value potentially confuse the host?

Ideally, the guest should be checking if XSAVES is enabled, which requires 
checking CPUID leaf 0xD, subleaf 1. So a bit of a chicken and egg thing 
going on the very first time. And then the guest should read MSR_IA32_XSS 
to get the actual value. This MSR is virtualized, so the hypervisor needs 
to not intercept access in order for the guest to actually set/get a 
value. Today, KVM/SVM doesn't support that since XSS is used (mainly/only) 
for shadow stack and KVM shadow stack support is only getting looked at now.

So the guest support for XSS and ES/SNP guests needs to be thought out a 
bit more.

Thanks,
Tom

>