Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between internal and external emulation

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 05 December 2017 14:32
> To: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org
> Subject: RE: [PATCH v2] x86/hvm: fix interaction between internal and
> external emulation
> 
> >>> On 05.12.17 at 15:00,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 05 December 2017 13:53
> >> >>> On 28.11.17 at 15:05,  wrote:
> >> rc = x86_emulate(_ctxt->ctxt, ops);
> >>
> >> if ( rc != X86EMUL_RETRY )
> >> {
> >> vio->mmio_cache_count = 0;
> >> vio->mmio_insn_bytes = 0;
> >> }
> >> else
> >> {
> >> ...
> >> }
> >> if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> >> rc = X86EMUL_RETRY;
> >>
> >
> > But that's not safe is it? If we've only completed some of the reps of an
> > instruction then we can't flush the instruction cache and we can't allow the
> > guest to take interrupts, can we?
> 
> Of course we can, just like a repeated string insn may be
> interrupted on bare hardware between any two iterations (with
> RIP still pointing at that insn). In fact with EFLAGS.TF set it is a
> requirement to deliver #DB after every iteration.

Ok. In that case your re-ordering would seem sound.

  Paul

> 
> Jan


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

Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between internal and external emulation

[Jan Beulich]

>>> On 05.12.17 at 15:11,  wrote:
>> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf
>> Of Paul Durrant
>> Sent: 05 December 2017 14:00
>> To: 'Jan Beulich' 
>> > From: Jan Beulich [mailto:jbeul...@suse.com]
>> > Sent: 05 December 2017 13:53
>> > >>> On 28.11.17 at 15:05,  wrote:
>> > > --- 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();
>> > >
>> > > -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){};
>> >
>> > While I can't (yet) say why without this change things would have
>> > behaved better on that old AMD box which is causing the osstest
>> > failure, I think Andrew's suggestion that we might be trying to
>> > emulate from a stale instruction cache is spot on: Doesn't
>> 
>> Yes, I can't see how the above was ever correct.
> 
> I think I see why this worked before...
> 
> Setting up the io_completion value meant that when hvm_do_resume() called 
> handle_hvm_io_completion() there was apparently an mmio outstanding and thus 
> handle_mmio() was called. At some point handle_mmio() has become a static 
> inline that calls hvm_emulate_one_insn() and that took care of the remaining 
> reps.

That would have worked sometimes, but as we've recently clarified
handle_hvm_io_completion() won't be called every time before
exiting back to the guest. IOW I would have expected random
failures with the same pattern as we see now, but I don't recall
ever having seen such before. (Otoh I admit I don't always look
closely when a transient failure occurs and then disappears in the
next flight.)

Jan


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

Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between internal and external emulation

[Jan Beulich]

>>> On 05.12.17 at 15:00,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 05 December 2017 13:53
>> >>> On 28.11.17 at 15:05,  wrote:
>> rc = x86_emulate(_ctxt->ctxt, ops);
>> 
>> if ( rc != X86EMUL_RETRY )
>> {
>> vio->mmio_cache_count = 0;
>> vio->mmio_insn_bytes = 0;
>> }
>> else
>> {
>> ...
>> }
>> if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>> rc = X86EMUL_RETRY;
>> 
> 
> But that's not safe is it? If we've only completed some of the reps of an 
> instruction then we can't flush the instruction cache and we can't allow the 
> guest to take interrupts, can we?

Of course we can, just like a repeated string insn may be
interrupted on bare hardware between any two iterations (with
RIP still pointing at that insn). In fact with EFLAGS.TF set it is a
requirement to deliver #DB after every iteration.

Jan


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

Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between internal and external emulation

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 05 December 2017 14:00
> To: 'Jan Beulich' <jbeul...@suse.com>
> Cc: xen-devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between
> internal and external emulation
> 
> > -Original Message-
> > From: Jan Beulich [mailto:jbeul...@suse.com]
> > Sent: 05 December 2017 13:53
> > To: Paul Durrant <paul.durr...@citrix.com>
> > Cc: xen-devel@lists.xenproject.org
> > Subject: Re: [PATCH v2] x86/hvm: fix interaction between internal and
> > external emulation
> >
> > >>> On 28.11.17 at 15:05, <paul.durr...@citrix.com> wrote:
> > > --- 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();
> > >
> > > -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){};
> >
> > While I can't (yet) say why without this change things would have
> > behaved better on that old AMD box which is causing the osstest
> > failure, I think Andrew's suggestion that we might be trying to
> > emulate from a stale instruction cache is spot on: Doesn't
> 
> Yes, I can't see how the above was ever correct.

I think I see why this worked before...

Setting up the io_completion value meant that when hvm_do_resume() called 
handle_hvm_io_completion() there was apparently an mmio outstanding and thus 
handle_mmio() was called. At some point handle_mmio() has become a static 
inline that calls hvm_emulate_one_insn() and that took care of the remaining 
reps.

  Paul

> 
> >
> > rc = x86_emulate(_ctxt->ctxt, ops);
> >
> > if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> > rc = X86EMUL_RETRY;
> > if ( rc != X86EMUL_RETRY )
> > {
> > vio->mmio_cache_count = 0;
> > vio->mmio_insn_bytes = 0;
> > }
> > else
> > ...
> >
> > in _hvm_emulate_one() need re-ordering of the two conditionals?
> > ->mmio_retry set, as described earlier, means we're exiting back to
> > the guest. At that point the guest can take interrupts and alike,
> > which means that if we're being re-entered we're not necessarily
> > going to continue emulation of the same previous instruction. I.e.
> >
> > rc = x86_emulate(_ctxt->ctxt, ops);
> >
> > if ( rc != X86EMUL_RETRY )
> > {
> > vio->mmio_cache_count = 0;
> > vio->mmio_insn_bytes = 0;
> > }
> > else
> > {
> > ...
> > }
> > if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> > rc = X86EMUL_RETRY;
> >
> 
> But that's not safe is it? If we've only completed some of the reps of an
> instruction then we can't flush the instruction cache and we can't allow the
> guest to take interrupts, can we?
> 
>   Paul
> 
> > (or the equivalent thereof with switch() and fall-through from
> > OKAY to default). Any "more clever" solution (like deferring the
> > cache invalidation until we're being re-entered, making it
> > dependent on CS:RIP having changed) feels fragile.
> >
> > 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: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between internal and external emulation

[Jan Beulich]

>>> On 28.11.17 at 15:05,  wrote:
> --- 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();
>  
> -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){};

While I can't (yet) say why without this change things would have
behaved better on that old AMD box which is causing the osstest
failure, I think Andrew's suggestion that we might be trying to
emulate from a stale instruction cache is spot on: Doesn't

rc = x86_emulate(_ctxt->ctxt, ops);

if ( rc == X86EMUL_OKAY && vio->mmio_retry )
rc = X86EMUL_RETRY;
if ( rc != X86EMUL_RETRY )
{
vio->mmio_cache_count = 0;
vio->mmio_insn_bytes = 0;
}
else
...

in _hvm_emulate_one() need re-ordering of the two conditionals?
->mmio_retry set, as described earlier, means we're exiting back to
the guest. At that point the guest can take interrupts and alike,
which means that if we're being re-entered we're not necessarily
going to continue emulation of the same previous instruction. I.e.

rc = x86_emulate(_ctxt->ctxt, ops);

if ( rc != X86EMUL_RETRY )
{
vio->mmio_cache_count = 0;
vio->mmio_insn_bytes = 0;
}
else
{
...
}
if ( rc == X86EMUL_OKAY && vio->mmio_retry )
rc = X86EMUL_RETRY;

(or the equivalent thereof with switch() and fall-through from
OKAY to default). Any "more clever" solution (like deferring the
cache invalidation until we're being re-entered, making it
dependent on CS:RIP having changed) feels fragile.

Jan


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

Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between internal and external emulation

[Julien Grall]

Hi,

On 30/11/17 14:28, Jan Beulich wrote:

On 28.11.17 at 15:05,  wrote:

A call to handle_hvm_io_completion() is needed for completing I/O
that requires external emulation. Such completion should be requested when
hvm_vcpu_io_need_completion() returns true after hvm_emulate_once() has
completed. This is indicative of the underlying I/O emulation having
returned X86EMUL_RETRY and hence a re-emulation of the instruction is
needed to pick up the result of the I/O.

A call to handle_hvm_io_completion() is NOT needed when the underlying
I/O has not returned X86EMUL_RETRY since there will be no result to pick
up. Hence it bogus to request such completion when mmio_retry is set,
since this can only happen if the underlying I/O emulation has returned
X86EMUL_OKAY (meaning the I/O has completed successfully).

Reported-by: Andrew Cooper 
Signed-off-by: Paul Durrant 
Reviewed-by: Jan Beulich 


Hmm, I notice Paul didn't Cc you on this one - despite it getting late,
this is still something to be considered for 4.10. It's certainly going
to be a backporting candidate.


Release-acked-by: Julien Grall 

Could this be committed today?

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between internal and external emulation

[Jan Beulich]

>>> On 28.11.17 at 15:05,  wrote:
> A call to handle_hvm_io_completion() is needed for completing I/O
> that requires external emulation. Such completion should be requested when
> hvm_vcpu_io_need_completion() returns true after hvm_emulate_once() has
> completed. This is indicative of the underlying I/O emulation having
> returned X86EMUL_RETRY and hence a re-emulation of the instruction is
> needed to pick up the result of the I/O.
> 
> A call to handle_hvm_io_completion() is NOT needed when the underlying
> I/O has not returned X86EMUL_RETRY since there will be no result to pick
> up. Hence it bogus to request such completion when mmio_retry is set,
> since this can only happen if the underlying I/O emulation has returned
> X86EMUL_OKAY (meaning the I/O has completed successfully).
> 
> Reported-by: Andrew Cooper 
> Signed-off-by: Paul Durrant 
> Reviewed-by: Jan Beulich 

Hmm, I notice Paul didn't Cc you on this one - despite it getting late,
this is still something to be considered for 4.10. It's certainly going
to be a backporting candidate.

Jan


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