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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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. 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel