linux-kernel - Re: [PATCH v3] kvm: svm: Use the hardware provided GPA instead of page walk

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]

Date:   Thu, 15 Dec 2016 13:42:05 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Brijesh Singh <brijesh.singh@....com>, kvm@...r.kernel.org
Cc:     thomas.lendacky@....com, rkrcmar@...hat.com, joro@...tes.org,
        x86@...nel.org, linux-kernel@...r.kernel.org, mingo@...hat.com,
        hpa@...or.com, pbonzini@...hat.com, tglx@...utronix.de, bp@...e.de
Subject: Re: [PATCH v3] kvm: svm: Use the hardware provided GPA instead of
 page walk


> +++ b/arch/x86/kvm/x86.c
> @@ -4420,6 +4420,21 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
>  }
>  EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
>
> +static int vcpu_is_mmio_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
> +			    gpa_t gpa, bool write)
> +{
> +	/* For APIC access vmexit */
> +	if ((gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE)
> +		return 1;
> +
> +	if (vcpu_match_mmio_gpa(vcpu, gpa)) {
> +		trace_vcpu_match_mmio(gva, gpa, write, true);
> +		return 1;
> +	}
> +
> +	return 0;
> +}

I think I'd prefer that in a separate patch. But I don't have any
strong feelings about this.

> +
>  static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
>  				gpa_t *gpa, struct x86_exception *exception,
>  				bool write)
> @@ -4446,16 +4461,7 @@ static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
>  	if (*gpa == UNMAPPED_GVA)
>  		return -1;
>
> -	/* For APIC access vmexit */
> -	if ((*gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE)
> -		return 1;
> -
> -	if (vcpu_match_mmio_gpa(vcpu, *gpa)) {
> -		trace_vcpu_match_mmio(gva, *gpa, write, true);
> -		return 1;
> -	}
> -
> -	return 0;
> +	return vcpu_is_mmio_gpa(vcpu, gva, *gpa, write);
>  }
>
>  int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
> @@ -4552,6 +4558,22 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>  	int handled, ret;
>  	bool write = ops->write;
>  	struct kvm_mmio_fragment *frag;
> +	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> +
> +	/*
> +	 * If the exit was due to a NPF we may already have a GPA.
> +	 * If the GPA is present, use it to avoid the GVA to GPA table walk.
> +	 * Note, this cannot be used on string operations since string
> +	 * operation using rep will only have the initial GPA from the NPF
> +	 * occurred.
> +	 */

I was wondering if it would make sense to get rid of gpa_available and 
rather define a new function:

bool exception_gpa_valid(struct kvm_vcpu)
{
	// check if svm
	// check if exit code is NPF
	// check ctxt
}

Then you could move the whole comment into that function.


Looks good to me in general.

-- 

David