> -----Original Message-----
> From: Xen-devel [mailto:[email protected]] On Behalf
> Of Paul Durrant
> Sent: 05 December 2017 14:00
> To: 'Jan Beulich' <[email protected]>
> Cc: [email protected]
> Subject: Re: [Xen-devel] [PATCH v2] x86/hvm: fix interaction between
> internal and external emulation
>
> > -----Original Message-----
> > From: Jan Beulich [mailto:[email protected]]
> > Sent: 05 December 2017 13:53
> > To: Paul Durrant <[email protected]>
> > Cc: [email protected]
> > Subject: Re: [PATCH v2] x86/hvm: fix interaction between internal and
> > external emulation
> >
> > >>> On 28.11.17 at 15:05, <[email protected]> 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(&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){};
> >
> > 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(&hvmemul_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(&hvmemul_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
> [email protected]
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
