> -----Original Message----- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 11 March 2019 11:33 > To: Paul Durrant <paul.durr...@citrix.com> > Cc: Andrew Cooper <andrew.coop...@citrix.com>; Igor Druzhinin > <igor.druzhi...@citrix.com>; Roger Pau > Monne <roger....@citrix.com>; Wei Liu <wei.l...@citrix.com>; xen-devel <xen- > de...@lists.xenproject.org> > Subject: RE: [PATCH] x86/hvm: finish IOREQ correctly on completion path > > >>> On 11.03.19 at 12:09, <paul.durr...@citrix.com> wrote: > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: 11 March 2019 11:04 > >> > >> >>> On 11.03.19 at 11:30, <paul.durr...@citrix.com> wrote: > >> >> From: Igor Druzhinin [mailto:igor.druzhi...@citrix.com] > >> >> Sent: 08 March 2019 21:31 > >> >> > >> >> --- a/xen/arch/x86/hvm/emulate.c > >> >> +++ b/xen/arch/x86/hvm/emulate.c > >> >> @@ -1080,7 +1080,15 @@ static int linear_read(unsigned long addr, > >> >> unsigned int bytes, void > *p_data, > >> >> uint32_t pfec, struct hvm_emulate_ctxt > >> >> *hvmemul_ctxt) > >> >> { > >> >> pagefault_info_t pfinfo; > >> >> - int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, > >> >> &pfinfo); > >> >> + const struct hvm_vcpu_io *vio = ¤t->arch.hvm.hvm_io; > >> >> + int rc = HVMTRANS_bad_gfn_to_mfn; > >> >> + > >> >> + /* > >> >> + * If the memory access can be handled immediately - do it, > >> >> + * otherwise re-enter ioreq completion path to properly consume it. > >> >> + */ > >> >> + if ( !hvm_ioreq_needs_completion(&vio->io_req) ) > >> >> + rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, > >> >> &pfinfo); > >> > > >> > I think this is the right thing to do > >> > >> It's not, and that's why I had written that earlier explanation which > >> you then responded to saying that the issue needs to be dealt with > >> by enforcing a consistent view of MMIO (or not) during initial try and > >> replay. That's _not_ what the above does in the general case: It > >> simply forces _all_ accesses into the slow path, thus re-introducing > >> the problem of page straddling accesses not getting routed correctly. > > > > Why? If there is no pending ioreq then why would the call to > > hvm_copy_from_guest_linear() not happen? AFAICT vio->io_req will only be > > updated when hvmemul_do_io() issues a new ioreq, so the test appears > > correct. > > How is that _all_ access fail this test? > > "All" was too heavy, as per this discussion: > > >> Even worse, it forces _all_ memory accesses by the insn under > >> emulation into the MMIO path. While this would happen to be okay > >> for a PUSH from MMIO (because the read comes first, and hence the > >> write would no longer see a pending IOREQ), it's wrong for (among > >> many other cases) a POP to MMIO, as the read (from stack, i.e. RAM) > >> will be replayed first, while the IOREQ is still marked incomplete. I'd > >> expect this to trigger the very domain_crash() in hvmemul_do_io() > >> that was also triggering because of the P2M type change behind our > >> backs. > > I should have said "all accesses preceding the one really accessing > MMIO". Using the provided example of POP, the linear_read() invocation > during replay (to read the stack) will find a pending IOREQ, and wrongly > go the MMIO path. This would, in this example, be correct only for > linear_write() to do. So the suggested change is correct only for any > insn accessing no more than one memory location (if there's no memory > access then of course we won't make it here in the first place).
Ok, thanks for the clarification. So, the problem is that memory accesses are not stashed (understandably I guess) in the mmio_cache. If they were then forcing the code down the MMIO path would work. So, what we probably need is a cache of all accesses that an instruction has made to date, regardless of whether they hit RAM or I/O emulation. Paul > > Jan > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
