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:04
> 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 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?

  Paul

> 
> 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.
> 
> Together with Andrew's pointing out of the ballooning case as
> another situation where machine state changes behind our backs
> during insn emulation, I meanwhile think that we really need to
> lock out machine state changes while insn emulation is in flight for
> one of the guest's vCPU-s. As this may be rather bad for
> performance, we'd need to strive to make the lock out window
> as small as possible. Otoh my hope would be that (in the common
> case) there aren't too frequent machine state changes in the first
> place.
> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel