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

[David Hildenbrand]

> +++ 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