[PATCH] x86/HVM: fix interaction between internal and extern emulation

[PATCH] x86/HVM: fix interaction between internal and extern emulation

[Jan Beulich]

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@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@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.

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

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

[Andrew Cooper]

On 27/11/17 08:28, Jan Beulich wrote:
> 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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 27 November 2017 08:29
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@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.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@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.

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

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

[Jan Beulich]

>>> On 28.11.17 at 10:49, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 27 November 2017 08:29
>> To: xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@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.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 November 2017 10:02
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> emulation
> 
> >>> On 28.11.17 at 10:49, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 27 November 2017 08:29
> >> To: xen-devel <xen-devel@lists.xenproject.org>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@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.cooper3@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@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.

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

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

[Jan Beulich]

>>> On 28.11.17 at 11:05, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 28 November 2017 10:02
>> >>> On 28.11.17 at 10:49, <Paul.Durrant@citrix.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: 27 November 2017 08:29
>> >> 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@citrix.com>
>> >> Signed-off-by: Jan Beulich <jbeulich@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.
> 
> 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.

Looking forward to an alternative (preferably not overly intrusive)
patch proposal then, if you dislike this one.

Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 November 2017 10:17
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> emulation
> 
> >>> On 28.11.17 at 11:05, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 28 November 2017 10:02
> >> >>> On 28.11.17 at 10:49, <Paul.Durrant@citrix.com> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: 27 November 2017 08:29
> >> >> 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@citrix.com>
> >> >> Signed-off-by: Jan Beulich <jbeulich@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.
> >
> > 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.
> 
> Looking forward to an alternative (preferably not overly intrusive)
> patch proposal then, if you dislike this one.

It would definitely be good to only reset io_completion when it is clear that handle_hvm_io_completion() is not going to be called (i.e. for internally handled I/O) and perhaps even add ASSERTs in _hvm_emulate_one() and handle_pio().

  Paul

> 
> Jan


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

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

[Jan Beulich]

>>> On 28.11.17 at 11:22, <Paul.Durrant@citrix.com> wrote:
> It would definitely be good to only reset io_completion when it is clear 
> that handle_hvm_io_completion() is not going to be called (i.e. for 
> internally handled I/O)

Where would you suggest to do that? These two ...

> and perhaps even add ASSERTs in _hvm_emulate_one() 
> and handle_pio().

... sit down the call tree from handle_hvm_io_completion(). Plus
internal vs external isn't distinguishable in _hvm_emulate_one()
afaict (neither on the way in nor on the way out). Adding
ASSERT()s there suggests the distinction would need to be done
up the call stack, yet up the call stack may only be the VM exit
handler. I don't think the state reset should be done in vendor-
specific code.

Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 November 2017 10:40
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> emulation
> 
> >>> On 28.11.17 at 11:22, <Paul.Durrant@citrix.com> wrote:
> > It would definitely be good to only reset io_completion when it is clear
> > that handle_hvm_io_completion() is not going to be called (i.e. for
> > internally handled I/O)
> 
> Where would you suggest to do that? These two ...
> 
> > and perhaps even add ASSERTs in _hvm_emulate_one()
> > and handle_pio().
> 
> ... sit down the call tree from handle_hvm_io_completion(). Plus
> internal vs external isn't distinguishable in _hvm_emulate_one()
> afaict (neither on the way in nor on the way out).

Whether the emulation is being handed internally or externally should be apparent on the way out because that's what hvm_vcpu_io_need_completion() is testing for after the call to hvm_emulate_one() in hvm_emulate_one_insn(). The problem is completion being requested if mmio_retry is set even if the former test fails, and I can't remember why that is. On the face of it, it looks wrong.

> Adding
> ASSERT()s there suggests the distinction would need to be done
> up the call stack, yet up the call stack may only be the VM exit
> handler. I don't think the state reset should be done in vendor-
> specific code.
> 

I was hoping that an argument could be passed into the call stack by handle_hvm_io_completion() so that the lower layers would be able to distinguish a re-emulation from an initial call and thus be able to verify state. Maybe that is not practical though.

  Paul

> Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 28 November 2017 11:01
> To: 'Jan Beulich' <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Julien Grall
> <julien.grall@arm.com>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH] x86/HVM: fix interaction between internal
> and extern emulation
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 28 November 2017 10:40
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> > devel@lists.xenproject.org>
> > Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> > emulation
> >
> > >>> On 28.11.17 at 11:22, <Paul.Durrant@citrix.com> wrote:
> > > It would definitely be good to only reset io_completion when it is clear
> > > that handle_hvm_io_completion() is not going to be called (i.e. for
> > > internally handled I/O)
> >
> > Where would you suggest to do that? These two ...
> >
> > > and perhaps even add ASSERTs in _hvm_emulate_one()
> > > and handle_pio().
> >
> > ... sit down the call tree from handle_hvm_io_completion(). Plus
> > internal vs external isn't distinguishable in _hvm_emulate_one()
> > afaict (neither on the way in nor on the way out).
> 
> Whether the emulation is being handed internally or externally should be
> apparent on the way out because that's what
> hvm_vcpu_io_need_completion() is testing for after the call to
> hvm_emulate_one() in hvm_emulate_one_insn(). The problem is
> completion being requested if mmio_retry is set even if the former test fails,
> and I can't remember why that is. On the face of it, it looks wrong.

Yes, it appears that mmio_retry is only set when the underlying emulation returned X86EMUL_OKAY but not all reps were completed. If the underlying emulation did not return X86EMUL_RETRY then I can't figure out why vio->io_completion should need to be set to anything other than HVMIO_no_completion since any other return value indicates there should be nothing pending.

  Paul

> 
> > Adding
> > ASSERT()s there suggests the distinction would need to be done
> > up the call stack, yet up the call stack may only be the VM exit
> > handler. I don't think the state reset should be done in vendor-
> > specific code.
> >
> 
> I was hoping that an argument could be passed into the call stack by
> handle_hvm_io_completion() so that the lower layers would be able to
> distinguish a re-emulation from an initial call and thus be able to verify state.
> Maybe that is not practical though.
> 
>   Paul
> 
> > Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 28.11.17 at 12:06, <Paul.Durrant@citrix.com> wrote:
> Yes, it appears that mmio_retry is only set when the underlying emulation 
> returned X86EMUL_OKAY but not all reps were completed. If the underlying 
> emulation did not return X86EMUL_RETRY then I can't figure out why 
> vio->io_completion should need to be set to anything other than 
> HVMIO_no_completion since any other return value indicates there should be 
> nothing pending.

So am I getting it right that you're suggesting to remove the
mmio_retry part of the condition in hvm_emulate_one_insn()?
That looks like it might work (I was previously only considering
to get rid of mmio_retry altogether, and that didn't look like a
viable route).

Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 November 2017 11:26
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> emulation
> 
> >>> On 28.11.17 at 12:06, <Paul.Durrant@citrix.com> wrote:
> > Yes, it appears that mmio_retry is only set when the underlying emulation
> > returned X86EMUL_OKAY but not all reps were completed. If the
> underlying
> > emulation did not return X86EMUL_RETRY then I can't figure out why
> > vio->io_completion should need to be set to anything other than
> > HVMIO_no_completion since any other return value indicates there should
> be
> > nothing pending.
> 
> So am I getting it right that you're suggesting to remove the
> mmio_retry part of the condition in hvm_emulate_one_insn()?
> That looks like it might work (I was previously only considering
> to get rid of mmio_retry altogether, and that didn't look like a
> viable route).

Yes, that's what I'm suggesting. I really can't see why it is needed. It could have been a mistake in my original patches or a semantic change in a subsequent patch, but it certainly looks wrong in current context.
Andrew has just sent me his xtf repro so I'll give the change a go with that.

  Paul

  Paul

> 
> Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 28 November 2017 11:31
> To: 'Jan Beulich' <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Julien Grall
> <julien.grall@arm.com>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH] x86/HVM: fix interaction between internal
> and extern emulation
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 28 November 2017 11:26
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> > devel@lists.xenproject.org>
> > Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> > emulation
> >
> > >>> On 28.11.17 at 12:06, <Paul.Durrant@citrix.com> wrote:
> > > Yes, it appears that mmio_retry is only set when the underlying
> emulation
> > > returned X86EMUL_OKAY but not all reps were completed. If the
> > underlying
> > > emulation did not return X86EMUL_RETRY then I can't figure out why
> > > vio->io_completion should need to be set to anything other than
> > > HVMIO_no_completion since any other return value indicates there
> should
> > be
> > > nothing pending.
> >
> > So am I getting it right that you're suggesting to remove the
> > mmio_retry part of the condition in hvm_emulate_one_insn()?
> > That looks like it might work (I was previously only considering
> > to get rid of mmio_retry altogether, and that didn't look like a
> > viable route).
> 
> Yes, that's what I'm suggesting. I really can't see why it is needed. It could
> have been a mistake in my original patches or a semantic change in a
> subsequent patch, but it certainly looks wrong in current context.
> Andrew has just sent me his xtf repro so I'll give the change a go with that.

Yes, this patch fixed the problem for me. I'll do some more tests to check for collateral damage now... If it's all good, do you want me to submit it or do you want to send it as a v2 of your patch?

  Paul

diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index e449b4196e..9d9e1b0e40 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -88,7 +88,7 @@ bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate, const char *descr)

     rc = hvm_emulate_one(&ctxt);

-    if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry )
+    if ( hvm_vcpu_io_need_completion(vio) )
         vio->io_completion = HVMIO_mmio_completion;
     else
         vio->mmio_access = (struct npfec){};
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index 03dea6c0fc..11211c8cd8 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -103,7 +103,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)

     rc = hvm_emulate_one(hvmemul_ctxt);

-    if ( hvm_vcpu_io_need_completion(vio) || vio->mmio_retry )
+    if ( hvm_vcpu_io_need_completion(vio) )
         vio->io_completion = HVMIO_realmode_completion;

     if ( rc == X86EMUL_UNHANDLEABLE )

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

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

[Jan Beulich]

>>> On 28.11.17 at 12:58, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
>> Of Paul Durrant
>> Sent: 28 November 2017 11:31
>> To: 'Jan Beulich' <JBeulich@suse.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Julien Grall
>> <julien.grall@arm.com>; xen-devel <xen-devel@lists.xenproject.org>
>> Subject: Re: [Xen-devel] [PATCH] x86/HVM: fix interaction between internal
>> and extern emulation
>> 
>> > -----Original Message-----
>> > From: Jan Beulich [mailto:JBeulich@suse.com]
>> > Sent: 28 November 2017 11:26
>> > To: Paul Durrant <Paul.Durrant@citrix.com>
>> > Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> > <Andrew.Cooper3@citrix.com>; xen-devel <xen-
>> > devel@lists.xenproject.org>
>> > Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
>> > emulation
>> >
>> > >>> On 28.11.17 at 12:06, <Paul.Durrant@citrix.com> wrote:
>> > > Yes, it appears that mmio_retry is only set when the underlying
>> emulation
>> > > returned X86EMUL_OKAY but not all reps were completed. If the
>> > underlying
>> > > emulation did not return X86EMUL_RETRY then I can't figure out why
>> > > vio->io_completion should need to be set to anything other than
>> > > HVMIO_no_completion since any other return value indicates there
>> should
>> > be
>> > > nothing pending.
>> >
>> > So am I getting it right that you're suggesting to remove the
>> > mmio_retry part of the condition in hvm_emulate_one_insn()?
>> > That looks like it might work (I was previously only considering
>> > to get rid of mmio_retry altogether, and that didn't look like a
>> > viable route).
>> 
>> Yes, that's what I'm suggesting. I really can't see why it is needed. It 
> could
>> have been a mistake in my original patches or a semantic change in a
>> subsequent patch, but it certainly looks wrong in current context.
>> Andrew has just sent me his xtf repro so I'll give the change a go with 
> that.
> 
> Yes, this patch fixed the problem for me. I'll do some more tests to check 
> for collateral damage now... If it's all good, do you want me to submit it or 
> do you want to send it as a v2 of your patch?

It's yours, so please submit it (perhaps nevertheless as v2). Feel
free to add my R-b right away if no other change turns out
necessary.

Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 November 2017 12:04
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] x86/HVM: fix interaction between internal and extern
> emulation
> 
> >>> On 28.11.17 at 12:58, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> Behalf
> >> Of Paul Durrant
> >> Sent: 28 November 2017 11:31
> >> To: 'Jan Beulich' <JBeulich@suse.com>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Julien Grall
> >> <julien.grall@arm.com>; xen-devel <xen-devel@lists.xenproject.org>
> >> Subject: Re: [Xen-devel] [PATCH] x86/HVM: fix interaction between
> internal
> >> and extern emulation
> >>
> >> > -----Original Message-----
> >> > From: Jan Beulich [mailto:JBeulich@suse.com]
> >> > Sent: 28 November 2017 11:26
> >> > To: Paul Durrant <Paul.Durrant@citrix.com>
> >> > Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> > <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> >> > devel@lists.xenproject.org>
> >> > Subject: RE: [PATCH] x86/HVM: fix interaction between internal and
> extern
> >> > emulation
> >> >
> >> > >>> On 28.11.17 at 12:06, <Paul.Durrant@citrix.com> wrote:
> >> > > Yes, it appears that mmio_retry is only set when the underlying
> >> emulation
> >> > > returned X86EMUL_OKAY but not all reps were completed. If the
> >> > underlying
> >> > > emulation did not return X86EMUL_RETRY then I can't figure out why
> >> > > vio->io_completion should need to be set to anything other than
> >> > > HVMIO_no_completion since any other return value indicates there
> >> should
> >> > be
> >> > > nothing pending.
> >> >
> >> > So am I getting it right that you're suggesting to remove the
> >> > mmio_retry part of the condition in hvm_emulate_one_insn()?
> >> > That looks like it might work (I was previously only considering
> >> > to get rid of mmio_retry altogether, and that didn't look like a
> >> > viable route).
> >>
> >> Yes, that's what I'm suggesting. I really can't see why it is needed. It
> > could
> >> have been a mistake in my original patches or a semantic change in a
> >> subsequent patch, but it certainly looks wrong in current context.
> >> Andrew has just sent me his xtf repro so I'll give the change a go with
> > that.
> >
> > Yes, this patch fixed the problem for me. I'll do some more tests to check
> > for collateral damage now... If it's all good, do you want me to submit it or
> > do you want to send it as a v2 of your patch?
> 
> It's yours, so please submit it (perhaps nevertheless as v2). Feel
> free to add my R-b right away if no other change turns out
> necessary.

Ok. Will do. Thanks,

  Paul

> 
> Jan


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