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