Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi Julien


> > > > > > +
> > > > > >       PROGRESS(xen):
> > > > > >           ret = relinquish_memory(d, &d->xenpage_list);
> > > > > >           if ( ret )
> > > > > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > > > > > index ae7ef96..9814481 100644
> > > > > > --- a/xen/arch/arm/io.c
> > > > > > +++ b/xen/arch/arm/io.c
> > > > > > @@ -16,6 +16,7 @@
> > > > > >    * GNU General Public License for more details.
> > > > > >    */
> > > > > >   +#include <xen/ioreq.h>
> > > > > >   #include <xen/lib.h>
> > > > > >   #include <xen/spinlock.h>
> > > > > >   #include <xen/sched.h>
> > > > > > @@ -23,6 +24,7 @@
> > > > > >   #include <asm/cpuerrata.h>
> > > > > >   #include <asm/current.h>
> > > > > >   #include <asm/mmio.h>
> > > > > > +#include <asm/hvm/ioreq.h>
> > > > > Shouldn't this have been included by "xen/ioreq.h"?
> > > > Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it 
> > > > turned out that there was nothing inside common header required 
> > > > arch one to be included and
> > > > I was asked to include arch header where it was indeed needed 
> > > > (several *.c files).
> > > Fair enough.
> > >
> > > [...]
> > >
> > > > > If you return IO_HANDLED here, then it means the we will take care 
> > > > > of previous I/O but the current one is going to be ignored. 
> > > > Which current one? As I understand, if try_fwd_ioserv() gets called 
> > > > with vio->req.state == STATE_IORESP_READY then this is a second 
> > > > round after emulator completes the emulation (the first round was when
> > > > we returned IO_RETRY down the function and claimed that we would 
> > > > need a completion), so we are still dealing with previous I/O.
> > > > vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> 
> > > > try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv()
> > > > And after we return IO_HANDLED here, handle_ioserv() will be called 
> > > > to complete the handling of this previous I/O emulation.
> > > > Or I really missed something?
> > > Hmmm... I somehow thought try_fw_ioserv() would only be called the 
> > > first time. Do you have a branch with your code applied? This would 
> > > help to follow the different paths.
> > Yes, I mentioned about it in cover letter.
> >
> > Please see
> > https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml5
> > why 5 - because I started counting from the RFC)
> Oh, I looked at the cover letter and didn't find it. Hence why I 
> asked. I should have looked more carefully. Thanks!
> I have looked closer at the question and I am not sure to understand 
> why arch_ioreq_complete_mmio() is going to call try_handle_mmio().
> This looks pretty innefficient to me because we already now the IO was 
> handled by the IOREQ server.
> I realize that x86 is calling handle_mmio() again. However, I don't 
> think we need the same on Arm because the instruction for accessing 
> device memory are a lot simpler (you can only read or store at most a 
> 64-bit value).
I think, I agree.


> So I would like to keep our emulation simple and not rely on 
> try_ioserv_fw() to always return true when call from completion 
> (AFAICT it is not possible to return false then).

So what you are proposing is just a replacement try_ioserv_fw() by 
handle_ioserv() technically?

diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 40b9e59..0508bd8 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs 
*regs,
 bool arch_ioreq_complete_mmio(void)
 {
-    struct vcpu *v = current;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     const union hsr hsr = { .bits = regs->hsr };
-    paddr_t addr = v->io.req.addr;

-    if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED )
+    if ( handle_ioserv(regs, current) == IO_HANDLED )
     {
         advance_pc(regs, hsr);
         return true;


> I will answer to the rest separately.
Thank you.


> Cheers,
--
Regards,

Oleksandr Tyshchenko