Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 6 Aug 2020, Jan Beulich wrote:
> On 06.08.2020 02:37, Stefano Stabellini wrote:
> > What should do_trap_stage2_abort_guest do on IO_RETRY? Simply return
> > early and let the scheduler do its job? Something like:
> > 
> >             enum io_state state = try_handle_mmio(regs, hsr, gpa);
> > 
> >             switch ( state )
> >             {
> >             case IO_ABORT:
> >                 goto inject_abt;
> >             case IO_HANDLED:
> >                 advance_pc(regs, hsr);
> >                 return;
> >             case IO_RETRY:
> >                 /* finish later */
> >                 return;
> >             case IO_UNHANDLED:
> >                 /* IO unhandled, try another way to handle it. */
> >                 break;
> >             default:
> >                 ASSERT_UNREACHABLE();
> >             }
> > 
> > Then, xen/arch/arm/ioreq.c:handle_mmio() gets called by
> > handle_hvm_io_completion() after QEMU completes the emulation. Today,
> > handle_mmio just sets the user register with the read value.
> > 
> > But it would be better if it called again the original function
> > do_trap_stage2_abort_guest to actually retry the original operation.
> > This time do_trap_stage2_abort_guest calls try_handle_mmio() and gets
> > IO_HANDLED instead of IO_RETRY, thus, it will advance_pc (the program
> > counter) completing the handling of this instruction.
> > 
> > The user register with the read value could be set by try_handle_mmio if
> > try_fwd_ioserv returns IO_HANDLED instead of IO_RETRY.
> > 
> > Is that how the state machine is expected to work?
> 
> I think so. Just because it has taken us quite some time (years) on
> the x86 side to get reasonably close to how hardware would behave
> (I think we're still not fully there): The re-execution path needs
> to make sure it observes exactly the same machine state as the
> original path did. In particular changes to memory (by another vCPU)
> must not be observed.

Thanks for the heads up. I think I understand how it is supposed to work
now. I hope Oleksandr is on the same page.