Hi Stefano,
On 11/08/2020 00:34, Stefano Stabellini wrote:
On Mon, 10 Aug 2020, Oleksandr wrote:
On 08.08.20 01:19, Oleksandr wrote:
On 08.08.20 00:50, Stefano Stabellini wrote:
On Fri, 7 Aug 2020, Oleksandr wrote:
On 06.08.20 03:37, Stefano Stabellini wrote:
Hi Stefano
Trying to simulate IO_RETRY handling mechanism (according to model
below) I
continuously get IO_RETRY from try_fwd_ioserv() ...
OK, thanks for the details. My interpretation seems to be correct.
In which case, it looks like xen/arch/arm/io.c:try_fwd_ioserv should
return IO_RETRY. Then, xen/arch/arm/traps.c:do_trap_stage2_abort_guest
also needs to handle try_handle_mmio returning IO_RETRY the first
around, and IO_HANDLED when after QEMU does its job.
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,
I may miss some important point, but I failed to see why try_handle_mmio
(try_fwd_ioserv) will return IO_HANDLED instead of IO_RETRY at this
stage.
Or current try_fwd_ioserv() logic needs rework?
I think you should check the ioreq->state in try_fwd_ioserv(), if the
result is ready, then ioreq->state should be STATE_IORESP_READY, and you
can return IO_HANDLED.
I optimized test patch a bit (now it looks much simpler). I didn't face any
issues during a quick test.
Both patches get much closer to following the proper state machine,
great! I think this patch is certainly a good improvement. I think the
other patch you sent earlier, slightly larger, is even better. It makes
the following additional changes that would be good to have:
- try_fwd_ioserv returns IO_HANDLED on state == STATE_IORESP_READY
- handle_mmio simply calls do_trap_stage2_abort_guest
I don't think we should call do_trap_stage2_abort_guest() as part of the
completion because:
* The function do_trap_stage2_abort_guest() is using registers that
are not context switched (such as FAR_EL2). I/O handling is split in two
with likely a context switch in the middle. The second part is the
completion (i.e call to handle_mmio()). So the system registers will be
incorrect.
* A big chunk of do_trap_stage2_abort_guest() is not necessary for
the completion. For instance, there is no need to try to translate the
guest virtual address to a guest physical address.
Therefore the version below is probably the best approach.
Cheers,
--
Julien Grall
