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

[Paolo Bonzini]

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