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: 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.

  Paul

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2107,6 +2107,7 @@ static int _hvm_emulate_one(struct hvm_e
>      hvm_emulate_init_per_insn(hvmemul_ctxt, vio->mmio_insn,
>                                vio->mmio_insn_bytes);
> 
> +    vio->io_completion = HVMIO_no_completion;
>      vio->mmio_retry = 0;
> 
>      rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -139,6 +139,8 @@ bool handle_pio(uint16_t port, unsigned
>      if ( dir == IOREQ_WRITE )
>          data = guest_cpu_user_regs()->eax;
> 
> +    vio->io_completion = HVMIO_no_completion;
> +
>      rc = hvmemul_do_pio_buffer(port, size, dir, &data);
> 
>      if ( hvm_vcpu_io_need_completion(vio) )
> 
> 


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