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

[Oleksandr <olekstysh@xxxxxxxxx>]

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>

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();
--
Regards,

Oleksandr Tyshchenko