[Part2 PATCH v5 30/31] KVM: SVM: Do not install #UD intercept when SEV is enabled

[Part2 PATCH v5 30/31] KVM: SVM: Do not install #UD intercept when SEV is enabled

[Brijesh Singh]

On #UD, x86_emulate_instruction() fetches the data from guest memory and
decodes the instruction bytes to assist further. When SEV is enabled, the
instruction bytes will be encrypted using the guest-specific key and the
hypervisor will no longer able to fetch the instruction bytes to assist
UD handling. By not installing intercept we let the guest receive and
handle #UD.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/kvm/svm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a47981714433..81fc20998423 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1448,8 +1448,10 @@ static void init_vmcb(struct vcpu_svm *svm)
 		svm->vmcb->control.int_ctl |= V_GIF_ENABLE_MASK;
 	}
 
-	if (sev_guest(svm->vcpu.kvm))
+	if (sev_guest(svm->vcpu.kvm)) {
 		svm->vmcb->control.nested_ctl |= SVM_NESTED_CTL_SEV_ENABLE;
+		clr_exception_intercept(svm, UD_VECTOR);
+	}
 
 	mark_all_dirty(svm->vmcb);
 
-- 
2.9.5

[Part2 PATCH v5 31/31] KVM: X86: Restart the guest when insn_len is zero and SEV is enabled

[Brijesh Singh]

On AMD platforms, under certain conditions insn_len may be zero on #NPF.
This can happen if a guest gets a page-fault on data access but the HW
table walker is not able to read the instruction page (e.g instruction
page is not present in memory).

Typically, when insn_len is zero, x86_emulate_instruction() walks the
guest page table and fetches the instruction bytes from guest memory.
When SEV is enabled, the guest memory is encrypted with guest-specific
key hence hypervisor will not able to fetch the instruction bytes.
In those cases we simply restart the guest.

I have encountered this issue when running kernbench inside the guest.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/kvm/mmu.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index eca30c1eb1d9..2bc0fe84aca2 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4953,6 +4953,23 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code,
 	if (mmio_info_in_cache(vcpu, cr2, direct))
 		emulation_type = 0;
 emulate:
+	/*
+	 * On AMD platforms, under certain conditions insn_len may be zero on #NPF.
+	 * This can happen if a guest gets a page-fault on data access but the HW
+	 * table walker is not able to read the instruction page (e.g instruction
+	 * page is not present in memory).
+	 *
+	 * Typically, when insn_len is zero, x86_emulate_instruction() walks the
+	 * guest page table and fetches the instruction bytes from guest memory.
+	 * When SEV is enabled, the guest memory is encrypted with guest-specific
+	 * key hence hypervisor will not able to fetch the instruction bytes.
+	 * In those cases we simply restart the guest.
+	 */
+	if (unlikely(!insn_len) &&
+	    kvm_x86_ops->mem_enc_enabled &&
+	    kvm_x86_ops->mem_enc_enabled(vcpu))
+		return 1;
+
 	er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len);
 
 	switch (er) {
-- 
2.9.5

Re: [Part2 PATCH v5 31/31] KVM: X86: Restart the guest when insn_len is zero and SEV is enabled

[Paolo Bonzini]

On 04/10/2017 15:17, Brijesh Singh wrote:
> +	/*
> +	 * On AMD platforms, under certain conditions insn_len may be zero on #NPF.
> +	 * This can happen if a guest gets a page-fault on data access but the HW
> +	 * table walker is not able to read the instruction page (e.g instruction
> +	 * page is not present in memory).
> +	 *
> +	 * Typically, when insn_len is zero, x86_emulate_instruction() walks the
> +	 * guest page table and fetches the instruction bytes from guest memory.
> +	 * When SEV is enabled, the guest memory is encrypted with guest-specific
> +	 * key hence hypervisor will not able to fetch the instruction bytes.
> +	 * In those cases we simply restart the guest.
> +	 */
> +	if (unlikely(!insn_len) &&
> +	    kvm_x86_ops->mem_enc_enabled &&
> +	    kvm_x86_ops->mem_enc_enabled(vcpu))
> +		return 1;
> +

Is it needed to test mem_enc_enabled?  Could it instead test for the
availability of decode assists?

Thanks,

Paolo

Re: [Part2 PATCH v5 31/31] KVM: X86: Restart the guest when insn_len is zero and SEV is enabled

[Paolo Bonzini]

On 18/10/2017 11:26, Paolo Bonzini wrote:
> On 04/10/2017 15:17, Brijesh Singh wrote:
>> +	/*
>> +	 * On AMD platforms, under certain conditions insn_len may be zero on #NPF.
>> +	 * This can happen if a guest gets a page-fault on data access but the HW
>> +	 * table walker is not able to read the instruction page (e.g instruction
>> +	 * page is not present in memory).
>> +	 *
>> +	 * Typically, when insn_len is zero, x86_emulate_instruction() walks the
>> +	 * guest page table and fetches the instruction bytes from guest memory.
>> +	 * When SEV is enabled, the guest memory is encrypted with guest-specific
>> +	 * key hence hypervisor will not able to fetch the instruction bytes.
>> +	 * In those cases we simply restart the guest.
>> +	 */
>> +	if (unlikely(!insn_len) &&
>> +	    kvm_x86_ops->mem_enc_enabled &&
>> +	    kvm_x86_ops->mem_enc_enabled(vcpu))
>> +		return 1;
>> +
> 
> Is it needed to test mem_enc_enabled?  Could it instead test for the
> availability of decode assists?

More precisely, you could test "unlikely(insn && !insn_len)" here and,
in svm.c, pass insn as

	static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
                        svm->vmcb->control.insn_bytes : 0

Paolo

Re: [Part2 PATCH v5 31/31] KVM: X86: Restart the guest when insn_len is zero and SEV is enabled

[Brijesh Singh]

Hi Paolo,


On 10/18/17 4:26 AM, Paolo Bonzini wrote:
> On 04/10/2017 15:17, Brijesh Singh wrote:
>> +	/*
>> +	 * On AMD platforms, under certain conditions insn_len may be zero on #NPF.
>> +	 * This can happen if a guest gets a page-fault on data access but the HW
>> +	 * table walker is not able to read the instruction page (e.g instruction
>> +	 * page is not present in memory).
>> +	 *
>> +	 * Typically, when insn_len is zero, x86_emulate_instruction() walks the
>> +	 * guest page table and fetches the instruction bytes from guest memory.
>> +	 * When SEV is enabled, the guest memory is encrypted with guest-specific
>> +	 * key hence hypervisor will not able to fetch the instruction bytes.
>> +	 * In those cases we simply restart the guest.
>> +	 */
>> +	if (unlikely(!insn_len) &&
>> +	    kvm_x86_ops->mem_enc_enabled &&
>> +	    kvm_x86_ops->mem_enc_enabled(vcpu))
>> +		return 1;
>> +
> Is it needed to test mem_enc_enabled?  Could it instead test for the
> availability of decode assists?

We can use X86_FEATURE_DECODEASSIST but since mmu.c is common file hence
I was not sure if its safe for the VMX.  Is it okay to use decode assist
feature bit in mm.c ?

>
> Thanks,
>
> Paolo

Re: [Part2 PATCH v5 31/31] KVM: X86: Restart the guest when insn_len is zero and SEV is enabled

[Paolo Bonzini]

On 18/10/2017 14:19, Brijesh Singh wrote:
>> Is it needed to test mem_enc_enabled?  Could it instead test for the
>> availability of decode assists?
>
> We can use X86_FEATURE_DECODEASSIST but since mmu.c is common file hence
> I was not sure if its safe for the VMX.  Is it okay to use decode assist
> feature bit in mm.c ?

See the follow up email. :)

Paolo