diff mbox series

[kernel,3/3] x86/sev: Do not handle #VC for DR7 read/write

Message ID 20221201021948.9259-4-aik@amd.com
State New
Headers
Series KVM: SEV: Enable AMD SEV-ES DebugSwap |

Commit Message

Alexey Kardashevskiy Dec. 1, 2022, 2:19 a.m. UTC 
  With SVM_SEV_FEAT_DEBUG_SWAP enabled, the VM should not get #VC events
for DR7 read/write which it rather avoided.

Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---
 arch/x86/kernel/sev.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Sean Christopherson Dec. 1, 2022, 5:38 p.m. UTC | #1 
On Thu, Dec 01, 2022, Alexey Kardashevskiy wrote:
> With SVM_SEV_FEAT_DEBUG_SWAP enabled, the VM should not get #VC events
> for DR7 read/write which it rather avoided.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> ---
>  arch/x86/kernel/sev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index a428c62330d3..4e91b9f8742c 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -1618,6 +1618,9 @@ static enum es_result vc_handle_dr7_write(struct ghcb *ghcb,
>  	long val, *reg = vc_insn_get_rm(ctxt);
>  	enum es_result ret;
>  
> +	if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)

Probably high time to add a helper/macro to convert the SEV_STATUS to the SEV_FEATURES
field.

> +		return ES_VMM_ERROR;
> +
>  	if (!reg)
>  		return ES_DECODE_FAILED;
>  
> @@ -1655,6 +1658,9 @@ static enum es_result vc_handle_dr7_read(struct ghcb *ghcb,
>  	struct sev_es_runtime_data *data = this_cpu_read(runtime_data);
>  	long *reg = vc_insn_get_rm(ctxt);
>  
> +	if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)
> +		return ES_VMM_ERROR;
> +
>  	if (!reg)
>  		return ES_DECODE_FAILED;
>  
> -- 
> 2.38.1
>
Borislav Petkov Dec. 7, 2022, 7:01 p.m. UTC | #2 
On Thu, Dec 01, 2022 at 05:38:33PM +0000, Sean Christopherson wrote:
> Probably high time to add a helper/macro to convert the SEV_STATUS to
> the SEV_FEATURES field.

Nah, there's a couple of

MSR_AMD64_SEV*

defines in arch/x86/include/asm/msr-index.h.

Bit 5 should simply be added there.

Thx.
Sean Christopherson Dec. 7, 2022, 7:07 p.m. UTC | #3 
On Wed, Dec 07, 2022, Borislav Petkov wrote:
> On Thu, Dec 01, 2022 at 05:38:33PM +0000, Sean Christopherson wrote:
> > Probably high time to add a helper/macro to convert the SEV_STATUS to
> > the SEV_FEATURES field.
> 
> Nah, there's a couple of
> 
> MSR_AMD64_SEV*
> 
> defines in arch/x86/include/asm/msr-index.h.
> 
> Bit 5 should simply be added there.

Ah, yeah, that's much better.
Alexey Kardashevskiy Dec. 8, 2022, 7:14 a.m. UTC | #4 
On 8/12/22 06:07, Sean Christopherson wrote:
> On Wed, Dec 07, 2022, Borislav Petkov wrote:
>> On Thu, Dec 01, 2022 at 05:38:33PM +0000, Sean Christopherson wrote:
>>> Probably high time to add a helper/macro to convert the SEV_STATUS to
>>> the SEV_FEATURES field.
>>
>> Nah, there's a couple of
>>
>> MSR_AMD64_SEV*
>>
>> defines in arch/x86/include/asm/msr-index.h.
>>
>> Bit 5 should simply be added there.
> 
> Ah, yeah, that's much better.

Sorry, I am not following. How is moving the bit makes 
SEV_STATUS_TO_FEATURES() not needed?

When I am setting it in VMSA SEV_FEATURES - it is a bit 5.

Inside a SEV VM, it is SEV_STATUS MSR and there it is bit 7. Mentioned 
MSR_AMD64_SEV* are SEV_STATUS MSR bits.

Since the current patch is bad, I'd rather define the bit twice then:

arch/x86/include/asm/msr-index.h:
#define MSR_AMD64_SEV_FEAT_DEBUG_SWAP    BIT_ULL(7)

arch/x86/include/asm/svm.h
#define SVM_SEV_FEAT_DEBUG_SWAP         BIT(5)

as nothing really says that SEV_FEATURES is always going to be 
SEV_STATUS>>2, even though it is now.

Soooo what is acceptable solution here? Thanks,
Borislav Petkov Dec. 8, 2022, 11:01 a.m. UTC | #5 
On Thu, Dec 08, 2022 at 06:14:01PM +1100, Alexey Kardashevskiy wrote:
> Sorry, I am not following. How is moving the bit makes
> SEV_STATUS_TO_FEATURES() not needed?
> 
> When I am setting it in VMSA SEV_FEATURES - it is a bit 5.
> 
> Inside a SEV VM, it is SEV_STATUS MSR and there it is bit 7. Mentioned
> MSR_AMD64_SEV* are SEV_STATUS MSR bits.
> 
> Since the current patch is bad, I'd rather define the bit twice then:

Yes.

> arch/x86/include/asm/msr-index.h:
> #define MSR_AMD64_SEV_FEAT_DEBUG_SWAP    BIT_ULL(7)
> 
> arch/x86/include/asm/svm.h
> #define SVM_SEV_FEAT_DEBUG_SWAP         BIT(5)
> 
> as nothing really says that SEV_FEATURES is always going to be
> SEV_STATUS>>2, even though it is now.
> 
> Soooo what is acceptable solution here? Thanks,

Right, so since you're testing against sev_status which is a copy of
MSR_AMD64_SEV, then you use bit definitions which are for that MSR as
documented in the respective PPR section for "MSRC001_0131 [SEV Status]
(Core::X86::Msr::SEV_Status)"

When you're setting the VMSA's SEV_FEATURES field, then you need a
different define, ofc.

This also automatically takes care of SEV_FEATURES not being tied to
SEV_STATUS >> 2 forever, as you say.

So yes, do the twice thing.

Thx.
diff mbox series

Patch

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index a428c62330d3..4e91b9f8742c 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1618,6 +1618,9 @@  static enum es_result vc_handle_dr7_write(struct ghcb *ghcb,
 	long val, *reg = vc_insn_get_rm(ctxt);
 	enum es_result ret;
 
+	if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)
+		return ES_VMM_ERROR;
+
 	if (!reg)
 		return ES_DECODE_FAILED;
 
@@ -1655,6 +1658,9 @@  static enum es_result vc_handle_dr7_read(struct ghcb *ghcb,
 	struct sev_es_runtime_data *data = this_cpu_read(runtime_data);
 	long *reg = vc_insn_get_rm(ctxt);
 
+	if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)
+		return ES_VMM_ERROR;
+
 	if (!reg)
 		return ES_DECODE_FAILED;