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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 06.08.20 23:32, Stefano Stabellini wrote:

Hi Stefano

> 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.
Not completely. I am still going through the discussion and navigating 
the code trying to understand missing bits for me.
Thanks for trying to clarify how it supposed to work.


--
Regards,

Oleksandr Tyshchenko