diff mbox series

[kernel,v3] x86/compressed/64: reduce #VC nesting for intercepted CPUID for SEV-SNP guest

Message ID 20231003073123.1763564-1-aik@amd.com
State New
Headers
Series [kernel,v3] x86/compressed/64: reduce #VC nesting for intercepted CPUID for SEV-SNP guest |

Commit Message

Alexey Kardashevskiy Oct. 3, 2023, 7:31 a.m. UTC 
  For certain intercepts an SNP guest uses the GHCB protocol to talk to
the hypervisor from the #VC handler. The protocol requires a shared page so
there is one per vCPU. In case NMI arrives in a middle of #VC or the NMI
handler triggers a #VC, there is another "backup" GHCB page which stores
the content of the first one while SVM_VMGEXIT_NMI_COMPLETE is sent.
The vc_raw_handle_exception() handler manages main and backup GHCB pages
via __sev_get_ghcb/__sev_put_ghcb.

This works fine for #VC and occasional NMIs but not so fine when the #VC
handler causes intercept + another #VC. If NMI arrives during
the second #VC, there are no more pages for SVM_VMGEXIT_NMI_COMPLETE.
The problem place is the #VC CPUID handler which reads an MSR which
triggers another #VC and if "perf" was running, panic happens:

Kernel panic - not syncing: Unable to handle #VC exception! GHCB and Backup GHCB are already in use

Add a helper similar to native_read_msr_safe() for making a direct hypercall
in the SEV-ES environment. Use the new helper instead of the raw "rdmsr" to
avoid the extra #VC event.

Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP CPUID table in #VC handlers")
Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---

Based on:
https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-urgent
which top at the time was:
62d5e970d022 "x86/sev: Change npages to unsigned long in snp_accept_memory()"

---
Changes:
v3:
* made it a function, mimic native_read_msr_safe() which 1) returns value 2) returns an error
* removed debug backtraces the commit log as these were added for debugging and never
appear with actual kernels


v2:
* de-uglify by defining rdmsr_safe_GHCB()
---
 arch/x86/kernel/sev-shared.c | 27 +++++++++++++++++---
 1 file changed, 23 insertions(+), 4 deletions(-)

Comments

Ingo Molnar Oct. 3, 2023, 8:07 a.m. UTC | #1 
* Alexey Kardashevskiy <aik@amd.com> wrote:

> For certain intercepts an SNP guest uses the GHCB protocol to talk to
> the hypervisor from the #VC handler. The protocol requires a shared page so

s/requires a shared page so there is
 /requires a shared page, so there is

> there is one per vCPU. In case NMI arrives in a middle of #VC or the NMI
> handler triggers a #VC, there is another "backup" GHCB page which stores
> the content of the first one while SVM_VMGEXIT_NMI_COMPLETE is sent.
> The vc_raw_handle_exception() handler manages main and backup GHCB pages
> via __sev_get_ghcb/__sev_put_ghcb.
> 
> This works fine for #VC and occasional NMIs but not so fine when the #VC
> handler causes intercept + another #VC. If NMI arrives during
> the second #VC, there are no more pages for SVM_VMGEXIT_NMI_COMPLETE.
> The problem place is the #VC CPUID handler which reads an MSR which
> triggers another #VC and if "perf" was running, panic happens:
> 
> Kernel panic - not syncing: Unable to handle #VC exception! GHCB and Backup GHCB are already in use
> 
> Add a helper similar to native_read_msr_safe() for making a direct hypercall
> in the SEV-ES environment. Use the new helper instead of the raw "rdmsr" to

s/"rdmsr"
 /RDMSR

> avoid the extra #VC event.
> 
> Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP CPUID table in #VC handlers")
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>

LGTM.

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo
Tom Lendacky Oct. 3, 2023, 5:21 p.m. UTC | #2 
On 10/3/23 02:31, Alexey Kardashevskiy wrote:
> For certain intercepts an SNP guest uses the GHCB protocol to talk to
> the hypervisor from the #VC handler. The protocol requires a shared page so
> there is one per vCPU. In case NMI arrives in a middle of #VC or the NMI
> handler triggers a #VC, there is another "backup" GHCB page which stores
> the content of the first one while SVM_VMGEXIT_NMI_COMPLETE is sent.
> The vc_raw_handle_exception() handler manages main and backup GHCB pages
> via __sev_get_ghcb/__sev_put_ghcb.
> 
> This works fine for #VC and occasional NMIs but not so fine when the #VC
> handler causes intercept + another #VC. If NMI arrives during
> the second #VC, there are no more pages for SVM_VMGEXIT_NMI_COMPLETE.
> The problem place is the #VC CPUID handler which reads an MSR which
> triggers another #VC and if "perf" was running, panic happens:
> 
> Kernel panic - not syncing: Unable to handle #VC exception! GHCB and Backup GHCB are already in use
> 
> Add a helper similar to native_read_msr_safe() for making a direct hypercall
> in the SEV-ES environment. Use the new helper instead of the raw "rdmsr" to
> avoid the extra #VC event.
> 
> Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP CPUID table in #VC handlers")
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> ---
> 
> Based on:
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-urgent
> which top at the time was:
> 62d5e970d022 "x86/sev: Change npages to unsigned long in snp_accept_memory()"
> 
> ---
> Changes:
> v3:
> * made it a function, mimic native_read_msr_safe() which 1) returns value 2) returns an error
> * removed debug backtraces the commit log as these were added for debugging and never
> appear with actual kernels
> 
> 
> v2:
> * de-uglify by defining rdmsr_safe_GHCB()
> ---
>   arch/x86/kernel/sev-shared.c | 27 +++++++++++++++++---
>   1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index dcf325b7b022..494d92a71986 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -241,6 +241,25 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
>   	return verify_exception_info(ghcb, ctxt);
>   }
>   
> +
> +/* Paravirt SEV-ES rdmsr which avoids extra #VC event */
> +static unsigned long long ghcb_prot_read_msr(unsigned int msr, struct ghcb *ghcb,
> +					     struct es_em_ctxt *ctxt, int *err)

Alternatively you could return enum es_result and take xss as a 
parameter... six of one, half dozen of another I guess.

> +{
> +	unsigned long long ret = 0;
> +
> +	ghcb_set_rcx(ghcb, msr);
> +
> +	*err = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, 0, 0);
> +	if (*err == ES_OK)
> +		ret = (ghcb->save.rdx << 32) | ghcb->save.rax;

You should check ghcb_rax_is_valid(ghcb) and ghcb_rdx_is_valid(ghcb) 
before using the values.

> +
> +	/* Invalidate qwords for likely another following GHCB call */
> +	vc_ghcb_invalidate(ghcb);

We should probably call this on entry to the function, too, right? Not 
sure it really matters though.

Thanks,
Tom

> +
> +	return ret;
> +}
> +
>   static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
>   {
>   	u64 val;
> @@ -477,11 +496,11 @@ static int snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
>   		if (leaf->subfn == 1) {
>   			/* Get XSS value if XSAVES is enabled. */
>   			if (leaf->eax & BIT(3)) {
> -				unsigned long lo, hi;
> +				int err = 0;
>   
> -				asm volatile("rdmsr" : "=a" (lo), "=d" (hi)
> -						     : "c" (MSR_IA32_XSS));
> -				xss = (hi << 32) | lo;
> +				xss = ghcb_prot_read_msr(MSR_IA32_XSS, ghcb, ctxt, &err);
> +				if (err != ES_OK)
> +					return -EINVAL;
>   			}
>   
>   			/*
Alexey Kardashevskiy Oct. 3, 2023, 11:22 p.m. UTC | #3 
On 4/10/23 04:21, Tom Lendacky wrote:
> On 10/3/23 02:31, Alexey Kardashevskiy wrote:
>> For certain intercepts an SNP guest uses the GHCB protocol to talk to
>> the hypervisor from the #VC handler. The protocol requires a shared 
>> page so
>> there is one per vCPU. In case NMI arrives in a middle of #VC or the NMI
>> handler triggers a #VC, there is another "backup" GHCB page which stores
>> the content of the first one while SVM_VMGEXIT_NMI_COMPLETE is sent.
>> The vc_raw_handle_exception() handler manages main and backup GHCB pages
>> via __sev_get_ghcb/__sev_put_ghcb.
>>
>> This works fine for #VC and occasional NMIs but not so fine when the #VC
>> handler causes intercept + another #VC. If NMI arrives during
>> the second #VC, there are no more pages for SVM_VMGEXIT_NMI_COMPLETE.
>> The problem place is the #VC CPUID handler which reads an MSR which
>> triggers another #VC and if "perf" was running, panic happens:
>>
>> Kernel panic - not syncing: Unable to handle #VC exception! GHCB and 
>> Backup GHCB are already in use
>>
>> Add a helper similar to native_read_msr_safe() for making a direct 
>> hypercall
>> in the SEV-ES environment. Use the new helper instead of the raw 
>> "rdmsr" to
>> avoid the extra #VC event.
>>
>> Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP CPUID 
>> table in #VC handlers")
>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>> ---
>>
>> Based on:
>> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-urgent
>> which top at the time was:
>> 62d5e970d022 "x86/sev: Change npages to unsigned long in 
>> snp_accept_memory()"
>>
>> ---
>> Changes:
>> v3:
>> * made it a function, mimic native_read_msr_safe() which 1) returns 
>> value 2) returns an error
>> * removed debug backtraces the commit log as these were added for 
>> debugging and never
>> appear with actual kernels
>>
>>
>> v2:
>> * de-uglify by defining rdmsr_safe_GHCB()
>> ---
>>   arch/x86/kernel/sev-shared.c | 27 +++++++++++++++++---
>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
>> index dcf325b7b022..494d92a71986 100644
>> --- a/arch/x86/kernel/sev-shared.c
>> +++ b/arch/x86/kernel/sev-shared.c
>> @@ -241,6 +241,25 @@ static enum es_result sev_es_ghcb_hv_call(struct 
>> ghcb *ghcb,
>>       return verify_exception_info(ghcb, ctxt);
>>   }
>> +
>> +/* Paravirt SEV-ES rdmsr which avoids extra #VC event */
>> +static unsigned long long ghcb_prot_read_msr(unsigned int msr, struct 
>> ghcb *ghcb,
>> +                         struct es_em_ctxt *ctxt, int *err)
> 
> Alternatively you could return enum es_result and take xss as a 
> parameter... six of one, half dozen of another I guess.

How do we decide on this? :)

and yeah, I need to s/int/enum es_result/

>> +{
>> +    unsigned long long ret = 0;
>> +
>> +    ghcb_set_rcx(ghcb, msr);
>> +
>> +    *err = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, 0, 0);
>> +    if (*err == ES_OK)
>> +        ret = (ghcb->save.rdx << 32) | ghcb->save.rax;
> 
> You should check ghcb_rax_is_valid(ghcb) and ghcb_rdx_is_valid(ghcb) 
> before using the values.

Huh. v4 is coming then. Although what are the chances of *err == ES_OK 
and !ghcb_rax_is_valid() at the same time? What if *err == ES_OK and 
ghcb_rdx_is_valid()==true but ghcb_rax_is_valid()==false?

return ((ghcb_rdx_is_valid(ghcb)?(ghcb->save.rdx << 32):0) |
	(ghcb_rax_is_valid(ghcb)?ghcb->save.rax:0;

Or I can just drop *err, invalidate ghcb before sev_es_ghcb_hv_call() 
and only rely on (ghcb_rdx_is_valid() && ghcb_rax_is_valid)?

Where should I stop with this? :)

>> +
>> +    /* Invalidate qwords for likely another following GHCB call */
>> +    vc_ghcb_invalidate(ghcb);
> 
> We should probably call this on entry to the function, too, right? Not 
> sure it really matters though.

The SVM_EXIT_MSR's handler in SVM/KVM only cares if RCX is valid in 
sev_es_validate_vmgexit() and the guest's ghcb_set_rcx() does that. 
Nothing in SVM enforces that other (unused) registers are not valid 
though. Thanks,


> 
> Thanks,
> Tom
> 
>> +
>> +    return ret;
>> +}
>> +
>>   static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
>>   {
>>       u64 val;
>> @@ -477,11 +496,11 @@ static int snp_cpuid_postprocess(struct ghcb 
>> *ghcb, struct es_em_ctxt *ctxt,
>>           if (leaf->subfn == 1) {
>>               /* Get XSS value if XSAVES is enabled. */
>>               if (leaf->eax & BIT(3)) {
>> -                unsigned long lo, hi;
>> +                int err = 0;
>> -                asm volatile("rdmsr" : "=a" (lo), "=d" (hi)
>> -                             : "c" (MSR_IA32_XSS));
>> -                xss = (hi << 32) | lo;
>> +                xss = ghcb_prot_read_msr(MSR_IA32_XSS, ghcb, ctxt, 
>> &err);
>> +                if (err != ES_OK)
>> +                    return -EINVAL;
>>               }
>>               /*
Tom Lendacky Oct. 4, 2023, 1:53 p.m. UTC | #4 
On 10/3/23 18:22, Alexey Kardashevskiy wrote:
> 
> On 4/10/23 04:21, Tom Lendacky wrote:
>> On 10/3/23 02:31, Alexey Kardashevskiy wrote:
>>> For certain intercepts an SNP guest uses the GHCB protocol to talk to
>>> the hypervisor from the #VC handler. The protocol requires a shared 
>>> page so
>>> there is one per vCPU. In case NMI arrives in a middle of #VC or the NMI
>>> handler triggers a #VC, there is another "backup" GHCB page which stores
>>> the content of the first one while SVM_VMGEXIT_NMI_COMPLETE is sent.
>>> The vc_raw_handle_exception() handler manages main and backup GHCB pages
>>> via __sev_get_ghcb/__sev_put_ghcb.
>>>
>>> This works fine for #VC and occasional NMIs but not so fine when the #VC
>>> handler causes intercept + another #VC. If NMI arrives during
>>> the second #VC, there are no more pages for SVM_VMGEXIT_NMI_COMPLETE.
>>> The problem place is the #VC CPUID handler which reads an MSR which
>>> triggers another #VC and if "perf" was running, panic happens:
>>>
>>> Kernel panic - not syncing: Unable to handle #VC exception! GHCB and 
>>> Backup GHCB are already in use
>>>
>>> Add a helper similar to native_read_msr_safe() for making a direct 
>>> hypercall
>>> in the SEV-ES environment. Use the new helper instead of the raw 
>>> "rdmsr" to
>>> avoid the extra #VC event.
>>>
>>> Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP CPUID 
>>> table in #VC handlers")
>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>> ---
>>>
>>> Based on:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-urgent
>>> which top at the time was:
>>> 62d5e970d022 "x86/sev: Change npages to unsigned long in 
>>> snp_accept_memory()"
>>>
>>> ---
>>> Changes:
>>> v3:
>>> * made it a function, mimic native_read_msr_safe() which 1) returns 
>>> value 2) returns an error
>>> * removed debug backtraces the commit log as these were added for 
>>> debugging and never
>>> appear with actual kernels
>>>
>>>
>>> v2:
>>> * de-uglify by defining rdmsr_safe_GHCB()
>>> ---
>>>   arch/x86/kernel/sev-shared.c | 27 +++++++++++++++++---
>>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
>>> index dcf325b7b022..494d92a71986 100644
>>> --- a/arch/x86/kernel/sev-shared.c
>>> +++ b/arch/x86/kernel/sev-shared.c
>>> @@ -241,6 +241,25 @@ static enum es_result sev_es_ghcb_hv_call(struct 
>>> ghcb *ghcb,
>>>       return verify_exception_info(ghcb, ctxt);
>>>   }
>>> +
>>> +/* Paravirt SEV-ES rdmsr which avoids extra #VC event */
>>> +static unsigned long long ghcb_prot_read_msr(unsigned int msr, struct 
>>> ghcb *ghcb,
>>> +                         struct es_em_ctxt *ctxt, int *err)
>>
>> Alternatively you could return enum es_result and take xss as a 
>> parameter... six of one, half dozen of another I guess.
> 
> How do we decide on this? :)
> 
> and yeah, I need to s/int/enum es_result/
> 
>>> +{
>>> +    unsigned long long ret = 0;
>>> +
>>> +    ghcb_set_rcx(ghcb, msr);
>>> +
>>> +    *err = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, 0, 0);
>>> +    if (*err == ES_OK)
>>> +        ret = (ghcb->save.rdx << 32) | ghcb->save.rax;
>>
>> You should check ghcb_rax_is_valid(ghcb) and ghcb_rdx_is_valid(ghcb) 
>> before using the values.
> 
> Huh. v4 is coming then. Although what are the chances of *err == ES_OK and 
> !ghcb_rax_is_valid() at the same time? What if *err == ES_OK and 
> ghcb_rdx_is_valid()==true but ghcb_rax_is_valid()==false?
> 
> return ((ghcb_rdx_is_valid(ghcb)?(ghcb->save.rdx << 32):0) |
>      (ghcb_rax_is_valid(ghcb)?ghcb->save.rax:0;
> 
> Or I can just drop *err, invalidate ghcb before sev_es_ghcb_hv_call() and 
> only rely on (ghcb_rdx_is_valid() && ghcb_rax_is_valid)?
> 
> Where should I stop with this? :)

No, you can't drop *err. The GHCB protocol specifically calls out how 
errors can be returned and how register state is returned.

In this case, sev_es_ghcb_hv_call() will check for general errors being 
returned from the hypervisor, e.g. non-zero SW_EXITINFO1[31:0] and that is 
why you need to check *err.

Then you need to validate that the hypervisor set the proper registers, 
hence the check for ghcb_rax/rdx_is_valid() (see __sev_cpuid_hv_ghcb() as 
an example).

Thanks,
Tom

> 
>>> +
>>> +    /* Invalidate qwords for likely another following GHCB call */
>>> +    vc_ghcb_invalidate(ghcb);
>>
>> We should probably call this on entry to the function, too, right? Not 
>> sure it really matters though.
> 
> The SVM_EXIT_MSR's handler in SVM/KVM only cares if RCX is valid in 
> sev_es_validate_vmgexit() and the guest's ghcb_set_rcx() does that. 
> Nothing in SVM enforces that other (unused) registers are not valid 
> though. Thanks,
> 
> 
>>
>> Thanks,
>> Tom
>>
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
>>>   {
>>>       u64 val;
>>> @@ -477,11 +496,11 @@ static int snp_cpuid_postprocess(struct ghcb 
>>> *ghcb, struct es_em_ctxt *ctxt,
>>>           if (leaf->subfn == 1) {
>>>               /* Get XSS value if XSAVES is enabled. */
>>>               if (leaf->eax & BIT(3)) {
>>> -                unsigned long lo, hi;
>>> +                int err = 0;
>>> -                asm volatile("rdmsr" : "=a" (lo), "=d" (hi)
>>> -                             : "c" (MSR_IA32_XSS));
>>> -                xss = (hi << 32) | lo;
>>> +                xss = ghcb_prot_read_msr(MSR_IA32_XSS, ghcb, ctxt, &err);
>>> +                if (err != ES_OK)
>>> +                    return -EINVAL;
>>>               }
>>>               /*
>
Alexey Kardashevskiy Oct. 5, 2023, 9:36 a.m. UTC | #5 
On 5/10/23 00:53, Tom Lendacky wrote:
> On 10/3/23 18:22, Alexey Kardashevskiy wrote:
>>
>> On 4/10/23 04:21, Tom Lendacky wrote:
>>> On 10/3/23 02:31, Alexey Kardashevskiy wrote:
>>>> For certain intercepts an SNP guest uses the GHCB protocol to talk to
>>>> the hypervisor from the #VC handler. The protocol requires a shared 
>>>> page so
>>>> there is one per vCPU. In case NMI arrives in a middle of #VC or the 
>>>> NMI
>>>> handler triggers a #VC, there is another "backup" GHCB page which 
>>>> stores
>>>> the content of the first one while SVM_VMGEXIT_NMI_COMPLETE is sent.
>>>> The vc_raw_handle_exception() handler manages main and backup GHCB 
>>>> pages
>>>> via __sev_get_ghcb/__sev_put_ghcb.
>>>>
>>>> This works fine for #VC and occasional NMIs but not so fine when the 
>>>> #VC
>>>> handler causes intercept + another #VC. If NMI arrives during
>>>> the second #VC, there are no more pages for SVM_VMGEXIT_NMI_COMPLETE.
>>>> The problem place is the #VC CPUID handler which reads an MSR which
>>>> triggers another #VC and if "perf" was running, panic happens:
>>>>
>>>> Kernel panic - not syncing: Unable to handle #VC exception! GHCB and 
>>>> Backup GHCB are already in use
>>>>
>>>> Add a helper similar to native_read_msr_safe() for making a direct 
>>>> hypercall
>>>> in the SEV-ES environment. Use the new helper instead of the raw 
>>>> "rdmsr" to
>>>> avoid the extra #VC event.
>>>>
>>>> Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP 
>>>> CPUID table in #VC handlers")
>>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>>> ---
>>>>
>>>> Based on:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-urgent
>>>> which top at the time was:
>>>> 62d5e970d022 "x86/sev: Change npages to unsigned long in 
>>>> snp_accept_memory()"
>>>>
>>>> ---
>>>> Changes:
>>>> v3:
>>>> * made it a function, mimic native_read_msr_safe() which 1) returns 
>>>> value 2) returns an error
>>>> * removed debug backtraces the commit log as these were added for 
>>>> debugging and never
>>>> appear with actual kernels
>>>>
>>>>
>>>> v2:
>>>> * de-uglify by defining rdmsr_safe_GHCB()
>>>> ---
>>>>   arch/x86/kernel/sev-shared.c | 27 +++++++++++++++++---
>>>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kernel/sev-shared.c 
>>>> b/arch/x86/kernel/sev-shared.c
>>>> index dcf325b7b022..494d92a71986 100644
>>>> --- a/arch/x86/kernel/sev-shared.c
>>>> +++ b/arch/x86/kernel/sev-shared.c
>>>> @@ -241,6 +241,25 @@ static enum es_result 
>>>> sev_es_ghcb_hv_call(struct ghcb *ghcb,
>>>>       return verify_exception_info(ghcb, ctxt);
>>>>   }
>>>> +
>>>> +/* Paravirt SEV-ES rdmsr which avoids extra #VC event */
>>>> +static unsigned long long ghcb_prot_read_msr(unsigned int msr, 
>>>> struct ghcb *ghcb,
>>>> +                         struct es_em_ctxt *ctxt, int *err)
>>>
>>> Alternatively you could return enum es_result and take xss as a 
>>> parameter... six of one, half dozen of another I guess.
>>
>> How do we decide on this? :)
>>
>> and yeah, I need to s/int/enum es_result/
>>
>>>> +{
>>>> +    unsigned long long ret = 0;
>>>> +
>>>> +    ghcb_set_rcx(ghcb, msr);
>>>> +
>>>> +    *err = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, 0, 0);
>>>> +    if (*err == ES_OK)
>>>> +        ret = (ghcb->save.rdx << 32) | ghcb->save.rax;
>>>
>>> You should check ghcb_rax_is_valid(ghcb) and ghcb_rdx_is_valid(ghcb) 
>>> before using the values.
>>
>> Huh. v4 is coming then. Although what are the chances of *err == ES_OK 
>> and !ghcb_rax_is_valid() at the same time? What if *err == ES_OK and 
>> ghcb_rdx_is_valid()==true but ghcb_rax_is_valid()==false?
>>
>> return ((ghcb_rdx_is_valid(ghcb)?(ghcb->save.rdx << 32):0) |
>>      (ghcb_rax_is_valid(ghcb)?ghcb->save.rax:0;
>>
>> Or I can just drop *err, invalidate ghcb before sev_es_ghcb_hv_call() 
>> and only rely on (ghcb_rdx_is_valid() && ghcb_rax_is_valid)?
>>
>> Where should I stop with this? :)
> 
> No, you can't drop *err. The GHCB protocol specifically calls out how 
> errors can be returned and how register state is returned.
> 
> In this case, sev_es_ghcb_hv_call() will check for general errors being 
> returned from the hypervisor, e.g. non-zero SW_EXITINFO1[31:0] and that 
> is why you need to check *err.
> 
> Then you need to validate that the hypervisor set the proper registers, 
> hence the check for ghcb_rax/rdx_is_valid() (see __sev_cpuid_hv_ghcb() 
> as an example).


After an offline discussion, it turns out this intercepted rdmsr of XSS 
in this particular place (postprocessing of CPUID 0xd:0x1 bit3 == 
"XSAVES, XRSTOR, and XSS are supported") in the guest should not have 
been intercepted in the first place as it is virtualized and swapped as 
typeB, but it is intercepted as this is the default.


This applied to KVM fixes the guest crashing problem:

--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4266,6 +4266,11 @@ static void svm_vcpu_after_set_cpuid(struct 
kvm_vcpu *vcpu)
         vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, 
X86_FEATURE_XSAVE) &&
                                     boot_cpu_has(X86_FEATURE_XSAVE) &&
                                     boot_cpu_has(X86_FEATURE_XSAVES);
+       if (vcpu->arch.xsaves_enabled)
+               set_msr_interception(vcpu, svm->msrpm, MSR_IA32_XSS, 1, 1);


Sooo. I guess we want to fix the KVM but at least for now the guest 
needs the fix too, does not it?

And adding Sean in cc.

Thanks,


> 
> Thanks,
> Tom
> 
>>
>>>> +
>>>> +    /* Invalidate qwords for likely another following GHCB call */
>>>> +    vc_ghcb_invalidate(ghcb);
>>>
>>> We should probably call this on entry to the function, too, right? 
>>> Not sure it really matters though.
>>
>> The SVM_EXIT_MSR's handler in SVM/KVM only cares if RCX is valid in 
>> sev_es_validate_vmgexit() and the guest's ghcb_set_rcx() does that. 
>> Nothing in SVM enforces that other (unused) registers are not valid 
>> though. Thanks,
>>
>>
>>>
>>> Thanks,
>>> Tom
>>>
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>   static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
>>>>   {
>>>>       u64 val;
>>>> @@ -477,11 +496,11 @@ static int snp_cpuid_postprocess(struct ghcb 
>>>> *ghcb, struct es_em_ctxt *ctxt,
>>>>           if (leaf->subfn == 1) {
>>>>               /* Get XSS value if XSAVES is enabled. */
>>>>               if (leaf->eax & BIT(3)) {
>>>> -                unsigned long lo, hi;
>>>> +                int err = 0;
>>>> -                asm volatile("rdmsr" : "=a" (lo), "=d" (hi)
>>>> -                             : "c" (MSR_IA32_XSS));
>>>> -                xss = (hi << 32) | lo;
>>>> +                xss = ghcb_prot_read_msr(MSR_IA32_XSS, ghcb, ctxt, 
>>>> &err);
>>>> +                if (err != ES_OK)
>>>> +                    return -EINVAL;
>>>>               }
>>>>               /*
>>
Tom Lendacky Oct. 5, 2023, 2:41 p.m. UTC | #6 
On 10/5/23 04:36, Alexey Kardashevskiy wrote:
> 
> On 5/10/23 00:53, Tom Lendacky wrote:
>> On 10/3/23 18:22, Alexey Kardashevskiy wrote:
>>>
>>> On 4/10/23 04:21, Tom Lendacky wrote:
>>>> On 10/3/23 02:31, Alexey Kardashevskiy wrote:
>>>>> For certain intercepts an SNP guest uses the GHCB protocol to talk to
>>>>> the hypervisor from the #VC handler. The protocol requires a shared 
>>>>> page so
>>>>> there is one per vCPU. In case NMI arrives in a middle of #VC or the NMI
>>>>> handler triggers a #VC, there is another "backup" GHCB page which stores
>>>>> the content of the first one while SVM_VMGEXIT_NMI_COMPLETE is sent.
>>>>> The vc_raw_handle_exception() handler manages main and backup GHCB pages
>>>>> via __sev_get_ghcb/__sev_put_ghcb.
>>>>>
>>>>> This works fine for #VC and occasional NMIs but not so fine when the #VC
>>>>> handler causes intercept + another #VC. If NMI arrives during
>>>>> the second #VC, there are no more pages for SVM_VMGEXIT_NMI_COMPLETE.
>>>>> The problem place is the #VC CPUID handler which reads an MSR which
>>>>> triggers another #VC and if "perf" was running, panic happens:
>>>>>
>>>>> Kernel panic - not syncing: Unable to handle #VC exception! GHCB and 
>>>>> Backup GHCB are already in use
>>>>>
>>>>> Add a helper similar to native_read_msr_safe() for making a direct 
>>>>> hypercall
>>>>> in the SEV-ES environment. Use the new helper instead of the raw 
>>>>> "rdmsr" to
>>>>> avoid the extra #VC event.
>>>>>
>>>>> Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP 
>>>>> CPUID table in #VC handlers")
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
>>>>> ---
>>>>>
>>>>> Based on:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-urgent
>>>>> which top at the time was:
>>>>> 62d5e970d022 "x86/sev: Change npages to unsigned long in 
>>>>> snp_accept_memory()"
>>>>>
>>>>> ---
>>>>> Changes:
>>>>> v3:
>>>>> * made it a function, mimic native_read_msr_safe() which 1) returns 
>>>>> value 2) returns an error
>>>>> * removed debug backtraces the commit log as these were added for 
>>>>> debugging and never
>>>>> appear with actual kernels
>>>>>
>>>>>
>>>>> v2:
>>>>> * de-uglify by defining rdmsr_safe_GHCB()
>>>>> ---
>>>>>   arch/x86/kernel/sev-shared.c | 27 +++++++++++++++++---
>>>>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
>>>>> index dcf325b7b022..494d92a71986 100644
>>>>> --- a/arch/x86/kernel/sev-shared.c
>>>>> +++ b/arch/x86/kernel/sev-shared.c
>>>>> @@ -241,6 +241,25 @@ static enum es_result sev_es_ghcb_hv_call(struct 
>>>>> ghcb *ghcb,
>>>>>       return verify_exception_info(ghcb, ctxt);
>>>>>   }
>>>>> +
>>>>> +/* Paravirt SEV-ES rdmsr which avoids extra #VC event */
>>>>> +static unsigned long long ghcb_prot_read_msr(unsigned int msr, 
>>>>> struct ghcb *ghcb,
>>>>> +                         struct es_em_ctxt *ctxt, int *err)
>>>>
>>>> Alternatively you could return enum es_result and take xss as a 
>>>> parameter... six of one, half dozen of another I guess.
>>>
>>> How do we decide on this? :)
>>>
>>> and yeah, I need to s/int/enum es_result/
>>>
>>>>> +{
>>>>> +    unsigned long long ret = 0;
>>>>> +
>>>>> +    ghcb_set_rcx(ghcb, msr);
>>>>> +
>>>>> +    *err = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, 0, 0);
>>>>> +    if (*err == ES_OK)
>>>>> +        ret = (ghcb->save.rdx << 32) | ghcb->save.rax;
>>>>
>>>> You should check ghcb_rax_is_valid(ghcb) and ghcb_rdx_is_valid(ghcb) 
>>>> before using the values.
>>>
>>> Huh. v4 is coming then. Although what are the chances of *err == ES_OK 
>>> and !ghcb_rax_is_valid() at the same time? What if *err == ES_OK and 
>>> ghcb_rdx_is_valid()==true but ghcb_rax_is_valid()==false?
>>>
>>> return ((ghcb_rdx_is_valid(ghcb)?(ghcb->save.rdx << 32):0) |
>>>      (ghcb_rax_is_valid(ghcb)?ghcb->save.rax:0;
>>>
>>> Or I can just drop *err, invalidate ghcb before sev_es_ghcb_hv_call() 
>>> and only rely on (ghcb_rdx_is_valid() && ghcb_rax_is_valid)?
>>>
>>> Where should I stop with this? :)
>>
>> No, you can't drop *err. The GHCB protocol specifically calls out how 
>> errors can be returned and how register state is returned.
>>
>> In this case, sev_es_ghcb_hv_call() will check for general errors being 
>> returned from the hypervisor, e.g. non-zero SW_EXITINFO1[31:0] and that 
>> is why you need to check *err.
>>
>> Then you need to validate that the hypervisor set the proper registers, 
>> hence the check for ghcb_rax/rdx_is_valid() (see __sev_cpuid_hv_ghcb() 
>> as an example).
> 
> 
> After an offline discussion, it turns out this intercepted rdmsr of XSS in 
> this particular place (postprocessing of CPUID 0xd:0x1 bit3 == "XSAVES, 
> XRSTOR, and XSS are supported") in the guest should not have been 
> intercepted in the first place as it is virtualized and swapped as typeB, 
> but it is intercepted as this is the default.
> 
> 
> This applied to KVM fixes the guest crashing problem:
> 
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4266,6 +4266,11 @@ static void svm_vcpu_after_set_cpuid(struct 
> kvm_vcpu *vcpu)
>          vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, 
> X86_FEATURE_XSAVE) &&
>                                      boot_cpu_has(X86_FEATURE_XSAVE) &&
>                                      boot_cpu_has(X86_FEATURE_XSAVES);
> +       if (vcpu->arch.xsaves_enabled)
> +               set_msr_interception(vcpu, svm->msrpm, MSR_IA32_XSS, 1, 1);

But this isn't valid for non ES/SNP guests, so it would need to be part of 
the (recently added) sev_es_vcpu_after_set_cpuid(). It would also need to 
(re)enable the intercept if xsaves_enabled isn't set.

> 
> 
> Sooo. I guess we want to fix the KVM but at least for now the guest needs 
> the fix too, does not it?

Since the SNP hypervisor patches aren't upstream, I don't think we do. If 
the SNP patches do the right thing from the start, everything is ok.

There isn't currently support for XSS in the ES #VC path for CPUID, so we 
do need to look at including that. But that is also the tricky part for ES 
since we don't have a CPUID table. The XSS MSR is valid if CPUID leaf 0xd, 
subleaf 1 has bit 3 set, but you need to be able to supply XSS on the 
CPUID call... so there is a bit of a chicken and egg issue where we might 
have to call that CPUID twice the very first time that CPUID request is 
encountered so that the proper XSS value can be supplied before the 
X86_FEATURE_XSAVES flag is set.

Is XSAVES being advertised for ES guests today? If not, we can add the 
interception updates today to KVM and be covered there, too.

Thanks,
Tom

> 
> And adding Sean in cc.
> 
> Thanks,
> 
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>>>> +
>>>>> +    /* Invalidate qwords for likely another following GHCB call */
>>>>> +    vc_ghcb_invalidate(ghcb);
>>>>
>>>> We should probably call this on entry to the function, too, right? Not 
>>>> sure it really matters though.
>>>
>>> The SVM_EXIT_MSR's handler in SVM/KVM only cares if RCX is valid in 
>>> sev_es_validate_vmgexit() and the guest's ghcb_set_rcx() does that. 
>>> Nothing in SVM enforces that other (unused) registers are not valid 
>>> though. Thanks,
>>>
>>>
>>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>   static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
>>>>>   {
>>>>>       u64 val;
>>>>> @@ -477,11 +496,11 @@ static int snp_cpuid_postprocess(struct ghcb 
>>>>> *ghcb, struct es_em_ctxt *ctxt,
>>>>>           if (leaf->subfn == 1) {
>>>>>               /* Get XSS value if XSAVES is enabled. */
>>>>>               if (leaf->eax & BIT(3)) {
>>>>> -                unsigned long lo, hi;
>>>>> +                int err = 0;
>>>>> -                asm volatile("rdmsr" : "=a" (lo), "=d" (hi)
>>>>> -                             : "c" (MSR_IA32_XSS));
>>>>> -                xss = (hi << 32) | lo;
>>>>> +                xss = ghcb_prot_read_msr(MSR_IA32_XSS, ghcb, ctxt, 
>>>>> &err);
>>>>> +                if (err != ES_OK)
>>>>> +                    return -EINVAL;
>>>>>               }
>>>>>               /*
>>>
>
diff mbox series

Patch

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index dcf325b7b022..494d92a71986 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -241,6 +241,25 @@  static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
 	return verify_exception_info(ghcb, ctxt);
 }
 
+
+/* Paravirt SEV-ES rdmsr which avoids extra #VC event */
+static unsigned long long ghcb_prot_read_msr(unsigned int msr, struct ghcb *ghcb,
+					     struct es_em_ctxt *ctxt, int *err)
+{
+	unsigned long long ret = 0;
+
+	ghcb_set_rcx(ghcb, msr);
+
+	*err = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, 0, 0);
+	if (*err == ES_OK)
+		ret = (ghcb->save.rdx << 32) | ghcb->save.rax;
+
+	/* Invalidate qwords for likely another following GHCB call */
+	vc_ghcb_invalidate(ghcb);
+
+	return ret;
+}
+
 static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
 {
 	u64 val;
@@ -477,11 +496,11 @@  static int snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
 		if (leaf->subfn == 1) {
 			/* Get XSS value if XSAVES is enabled. */
 			if (leaf->eax & BIT(3)) {
-				unsigned long lo, hi;
+				int err = 0;
 
-				asm volatile("rdmsr" : "=a" (lo), "=d" (hi)
-						     : "c" (MSR_IA32_XSS));
-				xss = (hi << 32) | lo;
+				xss = ghcb_prot_read_msr(MSR_IA32_XSS, ghcb, ctxt, &err);
+				if (err != ES_OK)
+					return -EINVAL;
 			}
 
 			/*