>>> On 28.11.17 at 10:49, <paul.durr...@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 27 November 2017 08:29 >> To: xen-devel <xen-devel@lists.xenproject.org> >> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper >> <andrew.coop...@citrix.com>; Paul Durrant <paul.durr...@citrix.com> >> 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.coop...@citrix.com> >> Signed-off-by: Jan Beulich <jbeul...@suse.com> >> --- >> 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. 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. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel