[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V5 15/22] xen/arm: Call vcpu_ioreq_handle_completion() in check_for_vcpu_work()

On 27.01.21 16:49, Julien Grall wrote:
Hi Oleksandr,

Hi Julien, Stefano

On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

This patch adds remaining bits needed for the IOREQ support on Arm.
Besides just calling vcpu_ioreq_handle_completion() we need to handle
it's return value to make sure that all the vCPU works are done before
we return to the guest (the vcpu_ioreq_handle_completion() may return
false if there is vCPU work to do or IOREQ state is invalid).
For that reason we use an unbounded loop in leave_hypervisor_to_guest().

The worse that can happen here if the vCPU will never run again
(the I/O will never complete). But, in Xen case, if the 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 the 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.

Please note, using this loop we will not spin forever on a pCPU,
preventing any other vCPUs from being scheduled. At every loop
we will call check_for_pcpu_work() that will process pending
softirqs. In case of failure, the guest will crash and the vCPU
will be unscheduled. In normal case, if the rescheduling is necessary
the vCPU will be rescheduled to give place to someone else.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>
Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
[On Arm only]
Tested-by: Wei Chen <Wei.Chen@xxxxxxx>

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

Changes V3 -> V4:
    - update patch description and comment in code

Changes V4 -> V5:
    - add Stefano's R-b

Reviewed-by means the person reviewed the code and confirmed it is correct. Given the changes you made below, I don't think this tag can hold.

Please confirm with Stefano he is happy with the tag to be carried.

I think you are right, sorry for that. I should have either clarified this question with Stefano in advance or dropped this tag.

@Stefano, are you happy with the changes for V4 -> V5 (would you mind if your R-b still stands)?

Other than that, the change looks good to me:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>



    - update patch subject/description and comment in code,
      was "xen/arm: Stick around in leave_hypervisor_to_guest until I/O has completed"
    - change loop logic a bit
    - squash with changes to check_for_vcpu_work() from patch #14

  xen/arch/arm/traps.c | 26 +++++++++++++++++++++++---
  1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index b0cd8f9..2039ff5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -21,6 +21,7 @@
  #include <xen/hypercall.h>
  #include <xen/init.h>
  #include <xen/iocap.h>
+#include <xen/ioreq.h>
  #include <xen/irq.h>
  #include <xen/lib.h>
  #include <xen/mem_access.h>
@@ -2261,12 +2262,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;
+    bool handled;
+    local_irq_enable();
+    handled = vcpu_ioreq_handle_completion(v);
+    local_irq_disable();
+    if ( !handled )
+        return true;
      if ( likely(!v->arch.need_flush_to_ram) )
-        return;
+        return false;
       * Give a chance for the pCPU to process work before handling the vCPU
@@ -2277,6 +2289,8 @@ static void check_for_vcpu_work(void)
+    return false;
@@ -2289,7 +2303,13 @@ void leave_hypervisor_to_guest(void)
  -    check_for_vcpu_work();
+    /*
+     * check_for_vcpu_work() may return true if there are more work to before +     * the vCPU can safely resume. This gives us an opportunity to deschedule
+     * the vCPU if needed.
+     */
+    while ( check_for_vcpu_work() )
+        check_for_pcpu_work();


Oleksandr Tyshchenko



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.