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

[Oleksandr <olekstysh@gmail.com>]

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