Re: [Xen-devel] [PATCH] x86/HVM: fix interaction between internal and extern emulation

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 28 November 2017 10:02
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> emulation
> 
> >>> On 28.11.17 at 10:49, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 27 November 2017 08:29
> >> To: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> >> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> Subject: [PATCH] x86/HVM: fix interaction between internal and extern
> >> emulation
> >>
> >> handle_hvm_io_completion() is being involved in resuming from requests
> >> sent to a device model only, while re-invocation of internally handled
> >> I/O which couldn't be handled in one go simply re-starts the affected
> >> instruction. When an internally handled split request is being followed
> >> by one sent to a device model, so far nothing reset vio->io_completion,
> >> leading to an MMIO emulation attempt on the next instruction _after_
> the
> >> one succesfully sent to qemu if that one doesn't itself require
> >> completion handling.
> >>
> >> Since only repeated string instructions are affected, strictly speaking
> >> the adjustment to handle_pio() isn't needed. Do it nevertheless for
> >> consistency as well as to avoid the lack thereof becoming an issue in
> >> the future; put the main change in generic enough a place to also cover
> >> VMX real mode emulation.
> >>
> >> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> It has been puzzling me for years how we could get away without clearing
> >> vio->io_completion in any more central place, i.e. other than as part of
> >> handling the completion.
> >
> > The idea is that, because HVMIO_no_completion is zero and thus the initial
> > value of vio->io_completion, no explicit initialization is required. If it 
> > is
> > set to anything other than that then there needs to be a call to
> > handle_hvm_io_completion() which will duly set it back
> HVMIO_no_completion.
> > So the question is how it is being set and why does this not result in the
> > appropriate completion call? I fear this patch is covering up a more
> > fundamental problem with the state model in certain cases.
> 
> Well - see the patch description: vio->mmio_retry being set after an
> emulation means hvm_emulate_one_insn() setting ->io_completion
> to HVMIO_mmio_completion no matter whether the request needs to
> go to qemu or is being handled internally.

Well that sounds like the problem then.

> Internally handled requests,
> as explained, don't need a completion to be run, though, and it will
> be the exception rather than the rule that handle_hvm_io_completion()
> would be invoked in such a case, causing ->io_completion to be cleared
> again.
> 
> Quite the contrary to what you say, I don't see why ->io_completion
> wasn't zapped the way the patch does it from the beginning. Nothing
> good can come from stale state being used _regardless_ of whether
> the most recent operation was handled externally or internally.

Because the state should never be stale. It sounds like use of mmio_retry is 
being overloaded and that's leading to this issue.

  Paul

> 
> Jan


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