linux-kernel - Re: [PATCH 2/3] KVM: x86: Avoid guest page table walk when gpa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <24a65134-308b-755b-049a-701fe6f215e7@redhat.com>
Date:   Thu, 17 Aug 2017 09:58:06 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     wanpeng.li@...mail.com, rkrcmar@...hat.com,
        Brijesh Singh <brijesh.singh@....com>
Subject: Re: [PATCH 2/3] KVM: x86: Avoid guest page table walk when
 gpa_available is set

On 11.08.2017 18:52, Paolo Bonzini wrote:
> From: Brijesh Singh <brijesh.singh@....com>
> 
> When a guest causes a page fault which requires emulation, the
> vcpu->arch.gpa_available flag is set to indicate that cr2 contains a
> valid GPA.
> 
> Currently, emulator_read_write_onepage() makes use of gpa_available flag
> to avoid a guest page walk for a known MMIO regions. Lets not limit
> the gpa_available optimization to just MMIO region. The patch extends
> the check to avoid page walk whenever gpa_available flag is set.

Can we move that to a separate patch?

> 
> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
> [Fix EPT=0 according to Wanpeng Li's fix, plus ensure VMX also uses the
>  new code. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---

[...]
> +++ b/arch/x86/kvm/svm.c
> @@ -4235,8 +4235,7 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>  	u32 exit_code = svm->vmcb->control.exit_code;
>  
>  	trace_kvm_exit(exit_code, vcpu, KVM_ISA_SVM);
> -
> -	vcpu->arch.gpa_available = (exit_code == SVM_EXIT_NPF);
> +	vcpu->arch.gpa_available = false;

Can we move resetting to false to vcpu_enter_guest()? It should be reset
before handle_exit() is called for both cases.

(maybe an additional patch)

>  
>  	if (!is_cr_intercept(svm, INTERCEPT_CR0_WRITE))
>  		vcpu->arch.cr0 = svm->vmcb->save.cr0;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 45fb0ea78ee8..79efb00dd70d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6393,9 +6393,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu)
>  	error_code |= (exit_qualification & 0x100) != 0 ?
>  	       PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK;
>  
> -	vcpu->arch.gpa_available = true;
>  	vcpu->arch.exit_qualification = exit_qualification;
> -
>  	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
>  }
>  
> @@ -6410,7 +6408,6 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
>  		return kvm_skip_emulated_instruction(vcpu);
>  	}
>  
> -	vcpu->arch.gpa_available = true;
>  	ret = kvm_mmu_page_fault(vcpu, gpa, PFERR_RSVD_MASK, NULL, 0);
>  	if (ret >= 0)
>  		return ret;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e40a779711a9..bb05b705c295 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4657,25 +4657,18 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>  	 */
>  	if (vcpu->arch.gpa_available &&
>  	    emulator_can_use_gpa(ctxt) &&
> -	    vcpu_is_mmio_gpa(vcpu, addr, exception->address, write) &&
> -	    (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {
> -		gpa = exception->address;
> -		goto mmio;
> +	    (addr & ~PAGE_MASK) == (vcpu->arch.gpa_val & ~PAGE_MASK)) {
> +		gpa = vcpu->arch.gpa_val;
> +		ret = vcpu_is_mmio_gpa(vcpu, addr, gpa, write);
> +	} else {
> +		ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
>  	}
>  
> -	ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
> -
>  	if (ret < 0)
>  		return X86EMUL_PROPAGATE_FAULT;
> -
> -	/* For APIC access vmexit */
> -	if (ret)
> -		goto mmio;
> -
> -	if (ops->read_write_emulate(vcpu, gpa, val, bytes))
> +	if (!ret && ops->read_write_emulate(vcpu, gpa, val, bytes))
>  		return X86EMUL_CONTINUE;
>  
> -mmio:
>  	/*
>  	 * Is this MMIO handled locally?
>  	 */
> 

Looks good to me.

-- 

Thanks,

David