Re: [Xen-devel] [PATCH] x86/hvm: finish IOREQ correctly on completion path

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 11 March 2019 11:33
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Igor Druzhinin 
> <igor.druzhinin@xxxxxxxxxx>; Roger Pau
> Monne <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH] x86/hvm: finish IOREQ correctly on completion path
> 
> >>> On 11.03.19 at 12:09, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 11 March 2019 11:04
> >>
> >> >>> On 11.03.19 at 11:30, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> >> From: Igor Druzhinin [mailto:igor.druzhinin@xxxxxxxxxx]
> >> >> 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 = &current->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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel