On 28/04/2017 21:15, Brijesh Singh wrote:
> Hi Radim,
>
>>
>> This will probably return false negatives when then vcpu->arch.gpa_val
>> couldn't be used anyway (possibly after a VM exits), so it is hard to
>> draw a conclusion.
>>
>> I would really like if we had a solution that passed the gpa inside
>> function parameters.
>>
>> (Btw. cr2 can also be a virtual address, so we can call it gpa directly)
>>
>
> I've tried the below patch and it seems to work fine. This does not
> consider
> PIO case and as you rightly pointed PIO should trigger #NPF relatively
> rarely.
> At least so far in my runs I have not seen PIO causing #NPF. If this sounds
> acceptable approach then I can submit v2 with these changes and remove
> gpa_val
> additional.
PIO can certainly cause #NPF, albeit rarely, so this is not a viable one.
Paolo
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 13c132b..c040e38 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4662,17 +4662,17 @@ 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;
> + ret = vcpu_is_mmio_gpa(vcpu, addr, gpa, write);
> + } else {
> + dump_stack();
> + ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception,
> write);
> +
> + if (ret < 0)
> + return X86EMUL_PROPAGATE_FAULT;
> }
>
> - 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;
> @@ -7056,11 +7056,11 @@ static int vcpu_run(struct kvm_vcpu *vcpu)
> return r;
> }
>
> -static inline int complete_emulated_io(struct kvm_vcpu *vcpu)
> +static inline int complete_emulated_io(struct kvm_vcpu *vcpu, unsigned
> long cr2)
> {
> int r;
> vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
> - r = emulate_instruction(vcpu, EMULTYPE_NO_DECODE);
> + r = x86_emulate_instruction(vcpu, cr2, EMULTYPE_NO_DECODE, NULL,
> 0);
> srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> if (r != EMULATE_DONE)
> return 0;
> @@ -7071,7 +7071,7 @@ static int complete_emulated_pio(struct kvm_vcpu
> *vcpu)
> {
> BUG_ON(!vcpu->arch.pio.count);
>
> - return complete_emulated_io(vcpu);
> + return complete_emulated_io(vcpu, 0);
> }
> /*
> @@ -7097,6 +7097,7 @@ static int complete_emulated_mmio(struct kvm_vcpu
> *vcpu)
> struct kvm_run *run = vcpu->run;
> struct kvm_mmio_fragment *frag;
> unsigned len;
> + gpa_t gpa;
>
> BUG_ON(!vcpu->mmio_needed);
>
> @@ -7106,6 +7107,12 @@ static int complete_emulated_mmio(struct kvm_vcpu
> *vcpu)
> if (!vcpu->mmio_is_write)
> memcpy(frag->data, run->mmio.data, len);
>
> + /*
> + * lets use the GPA from previous guest page table walk to avoid yet
> + * another guest page table walk when completing the MMIO
> page-fault.
> + */
> + gpa = frag->gpa;
> +
> if (frag->len <= 8) {
> /* Switch to the next fragment. */
> frag++;
> @@ -7124,7 +7131,7 @@ static int complete_emulated_mmio(struct kvm_vcpu
> *vcpu)
> if (vcpu->mmio_is_write)
> return 1;
> vcpu->mmio_read_completed = 1;
> - return complete_emulated_io(vcpu);
> + return complete_emulated_io(vcpu, gpa);
> }
>
> run->exit_reason = KVM_EXIT_MMIO;
>
