Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1542709 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] kvm: svm: Use the hardware provided GPA instead of  page walk David Hildenbrand <david@redhat.com> - 2016-12-15 13:50 +0100
    Re: [PATCH v3] kvm: svm: Use the hardware provided GPA instead of  page walk Paolo Bonzini <pbonzini@redhat.com> - 2016-12-15 14:10 +0100
      Re: [PATCH v3] kvm: svm: Use the hardware provided GPA instead of  page walk David Hildenbrand <david@redhat.com> - 2016-12-15 14:20 +0100
        Re: [PATCH v3] kvm: svm: Use the hardware provided GPA instead of  page walk Paolo Bonzini <pbonzini@redhat.com> - 2016-12-15 14:40 +0100

#1542709 — 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

[toc] | [next] | [standalone]

#1542718

On 15/12/2016 13:42, David Hildenbrand wrote:
> 
>> +++ 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
> }

No, this would be a layering violation.  The emulator ops don't know
about svm and exit codes (and in fact it's trivial to implement this
optimization for vmx, with a slightly different logic), so we need to
have gpa_available.

Paolo

> Then you could move the whole comment into that function.
> 
> 
> Looks good to me in general.
> 

[toc] | [prev] | [next] | [standalone]

#1542727

>>> +     * 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
>> }
>
> No, this would be a layering violation.  The emulator ops don't know
> about svm and exit codes (and in fact it's trivial to implement this
> optimization for vmx, with a slightly different logic), so we need to
> have gpa_available.

I was rather thinking about adding an vmx/svm independent callback, 
which would return false for vmx for now. I just saw the variable
and was wondering if it is really necessary.

-- 

David

[toc] | [prev] | [next] | [standalone]

#1542735

On 15/12/2016 14:09, David Hildenbrand wrote:
>>>
>>> bool exception_gpa_valid(struct kvm_vcpu)
>>> {
>>>     // check if svm
>>>     // check if exit code is NPF
>>>     // check ctxt
>>> }
>>
>> No, this would be a layering violation.  The emulator ops don't know
>> about svm and exit codes (and in fact it's trivial to implement this
>> optimization for vmx, with a slightly different logic), so we need to
>> have gpa_available.
> 
> I was rather thinking about adding an vmx/svm independent callback,
> which would return false for vmx for now. I just saw the variable
> and was wondering if it is really necessary.

The variable would probably just move into struct {vmx,svm}_vcpu.  Maybe
you could access the VMX/SVM exitcode directly, but doing that worries
me a bit...

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web