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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 05 December 2017 14:00
> To: 'Jan Beulich' <JBeulich@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:JBeulich@suse.com]
> > Sent: 05 December 2017 13:53
> > To: Paul Durrant <Paul.Durrant@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.durrant@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(&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
> 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