diff mbox series

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

Message ID 1611601709-28361-16-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series IOREQ feature (+ virtio-mmio) on Arm | expand

Commit Message

Oleksandr Tyshchenko Jan. 25, 2021, 7:08 p.m. UTC
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

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@epam.com>
CC: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
[On Arm only]
Tested-by: Wei Chen <Wei.Chen@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

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

Changes V4 -> V5:
   - add Stefano's R-b
   - 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(-)

Comments

Julien Grall Jan. 27, 2021, 2:49 p.m. UTC | #1
Hi Oleksandr,

On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> 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@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> [On Arm only]
> Tested-by: Wei Chen <Wei.Chen@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
> 
> 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.

Other than that, the change looks good to me:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

>     - 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;
>   
> +#ifdef CONFIG_IOREQ_SERVER
> +    bool handled;
> +
> +    local_irq_enable();
> +    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
> @@ -2277,6 +2289,8 @@ static void check_for_vcpu_work(void)
>       local_irq_enable();
>       p2m_flush_vm(v);
>       local_irq_disable();
> +
> +    return false;
>   }
>   
>   /*
> @@ -2289,7 +2303,13 @@ void leave_hypervisor_to_guest(void)
>   {
>       local_irq_disable();
>   
> -    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();
>       check_for_pcpu_work();
>   
>       vgic_sync_to_lrs();
>
Oleksandr Tyshchenko Jan. 27, 2021, 3:56 p.m. UTC | #2
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@epam.com>
>>
>> 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@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> [On Arm only]
>> Tested-by: Wei Chen <Wei.Chen@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
>>
>> 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@amazon.com>


Thanks.


>
> Cheers,
>
>>     - 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;
>>   +#ifdef CONFIG_IOREQ_SERVER
>> +    bool handled;
>> +
>> +    local_irq_enable();
>> +    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
>> @@ -2277,6 +2289,8 @@ static void check_for_vcpu_work(void)
>>       local_irq_enable();
>>       p2m_flush_vm(v);
>>       local_irq_disable();
>> +
>> +    return false;
>>   }
>>     /*
>> @@ -2289,7 +2303,13 @@ void leave_hypervisor_to_guest(void)
>>   {
>>       local_irq_disable();
>>   -    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();
>>       check_for_pcpu_work();
>>         vgic_sync_to_lrs();
>>
>
Stefano Stabellini Jan. 27, 2021, 8:34 p.m. UTC | #3
On Wed, 27 Jan 2021, Oleksandr wrote:
> 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@epam.com>
> > > 
> > > 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@epam.com>
> > > CC: Julien Grall <julien.grall@arm.com>
> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > > [On Arm only]
> > > Tested-by: Wei Chen <Wei.Chen@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
> > > 
> > > 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)?

Yes, I am.
diff mbox series

Patch

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;
 
+#ifdef CONFIG_IOREQ_SERVER
+    bool handled;
+
+    local_irq_enable();
+    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
@@ -2277,6 +2289,8 @@  static void check_for_vcpu_work(void)
     local_irq_enable();
     p2m_flush_vm(v);
     local_irq_disable();
+
+    return false;
 }
 
 /*
@@ -2289,7 +2303,13 @@  void leave_hypervisor_to_guest(void)
 {
     local_irq_disable();
 
-    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();
     check_for_pcpu_work();
 
     vgic_sync_to_lrs();