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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 19 Jan 2021, Oleksandr wrote:
> > > > > > >       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.

Yes I agree too


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

Yes, but I think we want to keep the check

    vio->req.state == STATE_IORESP_READY

So maybe (uncompiled, untested):

    if ( v->io.req.state != STATE_IORESP_READY )
        return false;

    if ( handle_ioserv(regs, current) == IO_HANDLED )
    {
        advance_pc(regs, hsr);
        return true;
    }