| Message ID | 7e27af5e-5978-5b57-f008-58515b218cce@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
Hi Paolo, >> >> 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. > For SEV guest cases, we anyway want to avoid any guest page table walks (even if it happens rarely). Sounds like we have to depend on gpa_val variable. I will refresh the patch to address one of Ramdim's feedback (move vcpu->arch.gpa_val assignment from handle_exit to pf_interception). Brijesh
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)