[PATCH 0/2] Fix AMD SEV guest boot issue with PCID feature

[PATCH 0/2] Fix AMD SEV guest boot issue with PCID feature

[Babu Moger]

SEV guests fail to boot on systems that support the PCID feature.

The problem is observed with SMM enabled OVMF build. The guest
crashes with the following messages on the console while loading.

----------------------------------------------------------------------
[    0.264224] tsc: Marking TSC unstable due to TSCs unsynchronized
[    0.264946] Calibrating delay loop (skipped) preset value.. 3194.00
                                                 BogoMIPS (lpj=1597000)
[    0.265946] pid_max: default: 65536 minimum: 512
KVM internal error. Suberror: 1
emulation failure
EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000000
ESI=00000000 EDI=7ffac000 EBP=00000000 ESP=7ffa1ff8
EIP=7ffb4280 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0020 00000000 ffffffff 00c09300 DPL=0 DS   [-WA]
CS =0000 00000000 00000fff 00009b00 DPL=0 CS16 [-RA]
SS =0020 00000000 ffffffff 00c09300 DPL=0 DS   [-WA]
DS =0020 00000000 ffffffff 00c09300 DPL=0 DS   [-WA]
FS =0020 00000000 ffffffff 00c09300 DPL=0 DS   [-WA]
GS =0020 00000000 ffffffff 00c09300 DPL=0 DS   [-WA]
LDT=0000 00000000 00000000 00000000
TR =0040 00003000 00004087 00008b00 DPL=0 TSS64-busy
GDT=     fffffe0000001000 0000007f
IDT=     fffffe0000000000 00000fff
CR0=80050033 CR2=ffff88817ffff000 CR3=0008000107e12000 CR4=000606b0
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000
DR3=0000000000000000 DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000d01
----------------------------------------------------------------------

The issue is root caused to the way kvm tries to validate the cr3
address in kvm_set_cr3(). The cr3 address in SEV guests have the encryption
bit set. KVM fails because the reserved bit check fails on this address.

This series fixes the problem by introducing a new kvm_x86_ops callback
function to detect the encryption bit and mask it during the check.
---

Babu Moger (2):
      KVM: x86: Introduce mask_cr3_rsvd_bits to mask memory encryption bit
      KVM:SVM: Mask SEV encryption bit from CR3 reserved bits


 arch/x86/include/asm/kvm_host.h |    2 ++
 arch/x86/kvm/svm/svm.c          |   15 +++++++++++++++
 arch/x86/kvm/svm/svm.h          |    3 +++
 arch/x86/kvm/vmx/vmx.c          |    6 ++++++
 arch/x86/kvm/x86.c              |    3 ++-
 5 files changed, 28 insertions(+), 1 deletion(-)

--

[PATCH 1/2] KVM: x86: Introduce mask_cr3_rsvd_bits to mask memory encryption bit

[Babu Moger]

SEV guests fail to boot on a system that supports the PCID feature.

While emulating the RSM instruction, KVM reads the guest CR3
and calls kvm_set_cr3(). If the vCPU is in the long mode,
kvm_set_cr3() does a sanity check for the CR3 value. In this case,
it validates whether the value has any reserved bits set.
The reserved bit range is 63:cpuid_maxphysaddr(). When AMD memory
encryption is enabled, the memory encryption bit is set in the CR3
value. The memory encryption bit may fall within the KVM reserved
bit range, causing the KVM emulation failure.

Introduce a generic callback function that can be used to mask bits
within the CR3 value before being checked by kvm_set_cr3().

Fixes: a780a3ea628268b2 ("KVM: X86: Fix reserved bits check for MOV to CR3")
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/include/asm/kvm_host.h |    2 ++
 arch/x86/kvm/svm/svm.c          |    6 ++++++
 arch/x86/kvm/vmx/vmx.c          |    6 ++++++
 arch/x86/kvm/x86.c              |    3 ++-
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d44858b69353..e791f841e0c2 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1265,6 +1265,8 @@ struct kvm_x86_ops {
 	int (*pre_enter_smm)(struct kvm_vcpu *vcpu, char *smstate);
 	int (*pre_leave_smm)(struct kvm_vcpu *vcpu, const char *smstate);
 	void (*enable_smi_window)(struct kvm_vcpu *vcpu);
+	unsigned long (*mask_cr3_rsvd_bits)(struct kvm_vcpu *vcpu,
+			unsigned long cr3);
 
 	int (*mem_enc_op)(struct kvm *kvm, void __user *argp);
 	int (*mem_enc_reg_region)(struct kvm *kvm, struct kvm_enc_region *argp);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 2f32fd09e259..a491a47d7f5c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4070,6 +4070,11 @@ static void enable_smi_window(struct kvm_vcpu *vcpu)
 	}
 }
 
+static unsigned long svm_mask_cr3_rsvd_bits(struct kvm_vcpu *vcpu, unsigned long cr3)
+{
+	return cr3;
+}
+
 static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, void *insn, int insn_len)
 {
 	bool smep, smap, is_user;
@@ -4285,6 +4290,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.pre_enter_smm = svm_pre_enter_smm,
 	.pre_leave_smm = svm_pre_leave_smm,
 	.enable_smi_window = enable_smi_window,
+	.mask_cr3_rsvd_bits = svm_mask_cr3_rsvd_bits,
 
 	.mem_enc_op = svm_mem_enc_op,
 	.mem_enc_reg_region = svm_register_enc_region,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 47b8357b9751..68920338b36a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7556,6 +7556,11 @@ static void enable_smi_window(struct kvm_vcpu *vcpu)
 	/* RSM will cause a vmexit anyway.  */
 }
 
+static unsigned long vmx_mask_cr3_rsvd_bits(struct kvm_vcpu *vcpu, unsigned long cr3)
+{
+	return cr3;
+}
+
 static bool vmx_apic_init_signal_blocked(struct kvm_vcpu *vcpu)
 {
 	return to_vmx(vcpu)->nested.vmxon;
@@ -7709,6 +7714,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.pre_enter_smm = vmx_pre_enter_smm,
 	.pre_leave_smm = vmx_pre_leave_smm,
 	.enable_smi_window = enable_smi_window,
+	.mask_cr3_rsvd_bits = vmx_mask_cr3_rsvd_bits,
 
 	.can_emulate_instruction = vmx_can_emulate_instruction,
 	.apic_init_signal_blocked = vmx_apic_init_signal_blocked,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f5ede41bf9e6..43a8d40bcfbf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1042,7 +1042,8 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 	}
 
 	if (is_long_mode(vcpu) &&
-	    (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
+	    (kvm_x86_ops.mask_cr3_rsvd_bits(vcpu, cr3) &
+	     rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
 		return 1;
 	else if (is_pae_paging(vcpu) &&
 		 !load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3))

[PATCH 2/2] KVM:SVM: Mask SEV encryption bit from CR3 reserved bits

[Babu Moger]

Add support to the mask_cr3_rsvd_bits() callback to mask the
encryption bit from the CR3 value when SEV is enabled.

Additionally, cache the encryption mask for quick access during
the check.

Fixes: a780a3ea628268b2 ("KVM: X86: Fix reserved bits check for MOV to CR3")
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kvm/svm/svm.c |   11 ++++++++++-
 arch/x86/kvm/svm/svm.h |    3 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a491a47d7f5c..c2b1e52810c6 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3741,6 +3741,7 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
+	struct kvm_cpuid_entry2 *best;
 
 	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
 				    boot_cpu_has(X86_FEATURE_XSAVE) &&
@@ -3771,6 +3772,12 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
 		kvm_request_apicv_update(vcpu->kvm, false,
 					 APICV_INHIBIT_REASON_NESTED);
+
+	best = kvm_find_cpuid_entry(vcpu, 0x8000001F, 0);
+	if (best)
+		svm->sev_enc_mask = ~(1UL << (best->ebx & 0x3f));
+	else
+		svm->sev_enc_mask = ~0UL;
 }
 
 static bool svm_has_wbinvd_exit(void)
@@ -4072,7 +4079,9 @@ static void enable_smi_window(struct kvm_vcpu *vcpu)
 
 static unsigned long svm_mask_cr3_rsvd_bits(struct kvm_vcpu *vcpu, unsigned long cr3)
 {
-	return cr3;
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	return sev_guest(vcpu->kvm) ? (cr3 & svm->sev_enc_mask) : cr3;
 }
 
 static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, void *insn, int insn_len)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 1d853fe4c778..57a36645a0e4 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -152,6 +152,9 @@ struct vcpu_svm {
 	u64 *avic_physical_id_cache;
 	bool avic_is_running;
 
+	/* SEV Memory encryption mask */
+	unsigned long sev_enc_mask;
+
 	/*
 	 * Per-vcpu list of struct amd_svm_iommu_ir:
 	 * This is used mainly to store interrupt remapping information used

Re: [PATCH 2/2] KVM:SVM: Mask SEV encryption bit from CR3 reserved bits

[Paolo Bonzini]

On 12/11/20 01:28, Babu Moger wrote:
> Add support to the mask_cr3_rsvd_bits() callback to mask the
> encryption bit from the CR3 value when SEV is enabled.
> 
> Additionally, cache the encryption mask for quick access during
> the check.
> 
> Fixes: a780a3ea628268b2 ("KVM: X86: Fix reserved bits check for MOV to CR3")
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   arch/x86/kvm/svm/svm.c |   11 ++++++++++-
>   arch/x86/kvm/svm/svm.h |    3 +++
>   2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index a491a47d7f5c..c2b1e52810c6 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3741,6 +3741,7 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
>   static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   {
>   	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct kvm_cpuid_entry2 *best;
>   
>   	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
>   				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> @@ -3771,6 +3772,12 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>   	if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
>   		kvm_request_apicv_update(vcpu->kvm, false,
>   					 APICV_INHIBIT_REASON_NESTED);
> +
> +	best = kvm_find_cpuid_entry(vcpu, 0x8000001F, 0);
> +	if (best)
> +		svm->sev_enc_mask = ~(1UL << (best->ebx & 0x3f));
> +	else
> +		svm->sev_enc_mask = ~0UL;
>   }
>   
>   static bool svm_has_wbinvd_exit(void)
> @@ -4072,7 +4079,9 @@ static void enable_smi_window(struct kvm_vcpu *vcpu)
>   
>   static unsigned long svm_mask_cr3_rsvd_bits(struct kvm_vcpu *vcpu, unsigned long cr3)
>   {
> -	return cr3;
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	return sev_guest(vcpu->kvm) ? (cr3 & svm->sev_enc_mask) : cr3;
>   }
>   
>   static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, void *insn, int insn_len)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 1d853fe4c778..57a36645a0e4 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -152,6 +152,9 @@ struct vcpu_svm {
>   	u64 *avic_physical_id_cache;
>   	bool avic_is_running;
>   
> +	/* SEV Memory encryption mask */
> +	unsigned long sev_enc_mask;
> +
>   	/*
>   	 * Per-vcpu list of struct amd_svm_iommu_ir:
>   	 * This is used mainly to store interrupt remapping information used
> 

Instead of adding a new callback, you can add a field to struct 
kvm_vcpu_arch:

  	if (is_long_mode(vcpu) &&
-	    (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
+	    (cr3 & vcpu->arch.cr3_lm_rsvd_bits))

Set it in kvm_vcpu_after_set_cpuid, and clear the memory encryption bit 
in kvm_x86_ops.vcpu_after_set_cpuid.

Paolo

Re: [PATCH 2/2] KVM:SVM: Mask SEV encryption bit from CR3 reserved bits

[Babu Moger]

On 11/12/20 2:32 AM, Paolo Bonzini wrote:
> On 12/11/20 01:28, Babu Moger wrote:
>> Add support to the mask_cr3_rsvd_bits() callback to mask the
>> encryption bit from the CR3 value when SEV is enabled.
>>
>> Additionally, cache the encryption mask for quick access during
>> the check.
>>
>> Fixes: a780a3ea628268b2 ("KVM: X86: Fix reserved bits check for MOV to
>> CR3")
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   arch/x86/kvm/svm/svm.c |   11 ++++++++++-
>>   arch/x86/kvm/svm/svm.h |    3 +++
>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index a491a47d7f5c..c2b1e52810c6 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3741,6 +3741,7 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu,
>> gfn_t gfn, bool is_mmio)
>>   static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>   {
>>       struct vcpu_svm *svm = to_svm(vcpu);
>> +    struct kvm_cpuid_entry2 *best;
>>         vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu,
>> X86_FEATURE_XSAVE) &&
>>                       boot_cpu_has(X86_FEATURE_XSAVE) &&
>> @@ -3771,6 +3772,12 @@ static void svm_vcpu_after_set_cpuid(struct
>> kvm_vcpu *vcpu)
>>       if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
>>           kvm_request_apicv_update(vcpu->kvm, false,
>>                        APICV_INHIBIT_REASON_NESTED);
>> +
>> +    best = kvm_find_cpuid_entry(vcpu, 0x8000001F, 0);
>> +    if (best)
>> +        svm->sev_enc_mask = ~(1UL << (best->ebx & 0x3f));
>> +    else
>> +        svm->sev_enc_mask = ~0UL;
>>   }
>>     static bool svm_has_wbinvd_exit(void)
>> @@ -4072,7 +4079,9 @@ static void enable_smi_window(struct kvm_vcpu *vcpu)
>>     static unsigned long svm_mask_cr3_rsvd_bits(struct kvm_vcpu *vcpu,
>> unsigned long cr3)
>>   {
>> -    return cr3;
>> +    struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +    return sev_guest(vcpu->kvm) ? (cr3 & svm->sev_enc_mask) : cr3;
>>   }
>>     static bool svm_can_emulate_instruction(struct kvm_vcpu *vcpu, void
>> *insn, int insn_len)
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index 1d853fe4c778..57a36645a0e4 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -152,6 +152,9 @@ struct vcpu_svm {
>>       u64 *avic_physical_id_cache;
>>       bool avic_is_running;
>>   +    /* SEV Memory encryption mask */
>> +    unsigned long sev_enc_mask;
>> +
>>       /*
>>        * Per-vcpu list of struct amd_svm_iommu_ir:
>>        * This is used mainly to store interrupt remapping information used
>>
> 
> Instead of adding a new callback, you can add a field to struct
> kvm_vcpu_arch:
> 
>      if (is_long_mode(vcpu) &&
> -        (cr3 & rsvd_bits(cpuid_maxphyaddr(vcpu), 63)))
> +        (cr3 & vcpu->arch.cr3_lm_rsvd_bits))
> 
> Set it in kvm_vcpu_after_set_cpuid, and clear the memory encryption bit in
> kvm_x86_ops.vcpu_after_set_cpuid.

Yes. That should work. Will resubmit the patches. Thanks