Hello Oleksandr,
Oleksandr Tyshchenko writes:
> From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>
> This patch adds proper handling of return value of
> vcpu_ioreq_handle_completion() which involves using a loop
> in leave_hypervisor_to_guest().
>
> The reason to use an unbounded loop here is the fact that vCPU
> shouldn't continue until an I/O has completed. In Xen case, if an I/O
> never completes then it most likely means that something went horribly
> wrong with the Device Emulator. And it is most likely not safe to
> continue. So letting the vCPU to spin forever if I/O never completes
> is a safer action than letting it continue and leaving the guest in
> unclear state and is the best what we can do for now.
>
> This wouldn't be an issue for Xen as do_softirq() would be called at
> every loop. In case of failure, the guest will crash and the vCPU
> will be unscheduled.
>
Why you don't block vcpu there and unblock it when response is ready? If
I got it right, "client" vcpu will spin in the loop, eating own
scheduling budget with no useful work done. In the worst case, it will
prevent "server" vcpu from running.
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
> CC: Julien Grall <julien.gr...@arm.com>
>
> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
>
> Changes V1 -> V2:
> - new patch, changes were derived from (+ new explanation):
> arm/ioreq: Introduce arch specific bits for IOREQ/DM features
>
> Changes V2 -> V3:
> - update patch description
> ---
> ---
> xen/arch/arm/traps.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 036b13f..4cef43e 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2257,18 +2257,23 @@ static void check_for_pcpu_work(void)
> * Process pending work for the vCPU. Any call should be fast or
> * implement preemption.
> */
> -static void check_for_vcpu_work(void)
> +static bool check_for_vcpu_work(void)
> {
> struct vcpu *v = current;
>
> #ifdef CONFIG_IOREQ_SERVER
> + bool handled;
> +
> local_irq_enable();
> - vcpu_ioreq_handle_completion(v);
> + handled = vcpu_ioreq_handle_completion(v);
> local_irq_disable();
> +
> + if ( !handled )
> + return true;
> #endif
>
> if ( likely(!v->arch.need_flush_to_ram) )
> - return;
> + return false;
>
> /*
> * Give a chance for the pCPU to process work before handling the vCPU
> @@ -2279,6 +2284,8 @@ static void check_for_vcpu_work(void)
> local_irq_enable();
> p2m_flush_vm(v);
> local_irq_disable();
> +
> + return false;
> }
>
> /*
> @@ -2291,8 +2298,22 @@ void leave_hypervisor_to_guest(void)
> {
> local_irq_disable();
>
> - check_for_vcpu_work();
> - check_for_pcpu_work();
> + /*
> + * The reason to use an unbounded loop here is the fact that vCPU
> + * shouldn't continue until an I/O has completed. In Xen case, if an I/O
> + * never completes then it most likely means that something went horribly
> + * wrong with the Device Emulator. And it is most likely not safe to
> + * continue. So letting the vCPU to spin forever if I/O never completes
> + * is a safer action than letting it continue and leaving the guest in
> + * unclear state and is the best what we can do for now.
> + *
> + * This wouldn't be an issue for Xen as do_softirq() would be called at
> + * every loop. In case of failure, the guest will crash and the vCPU
> + * will be unscheduled.
> + */
> + do {
> + check_for_pcpu_work();
> + } while ( check_for_vcpu_work() );
>
> vgic_sync_to_lrs();
--
Volodymyr Babchuk at EPAM
