Re: [PATCH V1 13/16] xen/ioreq: Make x86's invalidate qemu mapcache handling common

[Oleksandr <olekstysh@xxxxxxxxx>]

On 16.09.20 11:50, Jan Beulich wrote:

Hi Jan, Roger

> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1490,6 +1490,12 @@ static void do_trap_hypercall(struct cpu_user_regs 
> > *regs, register_t *nr,
> >       /* Ensure the hypercall trap instruction is re-executed. */
> >       if ( current->hcall_preempted )
> >           regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
> > +
> > +#ifdef CONFIG_IOREQ_SERVER
> > +    if ( unlikely(current->domain->qemu_mapcache_invalidate) &&
> > +         test_and_clear_bool(current->domain->qemu_mapcache_invalidate) )
> > +        send_invalidate_req();
> > +#endif
> >   }
> There's a lot of uses of "current" here now, and these don't look to
> exactly be cheap on Arm either (they aren't on x86), so I wonder
> whether this is the point where at least "current" wants latching
> into a local variable here.
Sounds reasonable, will use local variable


> > --- a/xen/arch/x86/hvm/hypercall.c
> > +++ b/xen/arch/x86/hvm/hypercall.c
> > @@ -18,8 +18,10 @@
> >    *
> >    * Copyright (c) 2017 Citrix Systems Ltd.
> >    */
> > +
> >   #include <xen/lib.h>
> >   #include <xen/hypercall.h>
> > +#include <xen/ioreq.h>
> >   #include <xen/nospec.h>
> While I don't care much about the presence of absence of the blank
> line between head comment and #include-s, I don't see why you add
> one here.
Accidentally), will remove.


> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1651,6 +1651,11 @@ long do_memory_op(unsigned long cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >           break;
> >       }
> >   
> > +#ifdef CONFIG_IOREQ_SERVER
> > +    if ( op == XENMEM_decrease_reservation )
> > +        curr_d->qemu_mapcache_invalidate = true;
> > +#endif
> I don't see why you put this right into decrease_reservation(). This
> isn't just to avoid the extra conditional, but first and foremost to
> avoid bypassing the earlier return from the function (in the case of
> preemption). In the context of this I wonder whether the ordering of
> operations in hvm_hypercall() is actually correct.
Good point, indeed we may return earlier in case of preemption, I missed 
that.
Will move it to decrease_reservation(). But, we may return even earlier 
in case of error...
Now I am wondering should we move it to the very beginning of command 
processing or not?
AFAIU before this patch qemu_mapcache_invalidate was always set in 
hvm_memory_op() if XENMEM_decrease_reservation came
despite of possible errors in the command processing.


> I'm also unconvinced curr_d is the right domain in all cases here;
> while this may be a pre-existing issue in principle, I'm afraid it
> gets more pronounced by the logic getting moved to common code.
Sorry I didn't get your concern here.


> Roger - thoughts either way with, in particular, PVH Dom0 in mind?
> > --- a/xen/include/xen/ioreq.h
> > +++ b/xen/include/xen/ioreq.h
> > @@ -97,6 +97,8 @@ static inline bool hvm_ioreq_needs_completion(const ioreq_t 
> > *ioreq)
> >              (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
> >   }
> >   
> > +void send_invalidate_req(void);
> Perhaps rename to ioreq_send_invalidate(), ioreq_send_invalidate_req(),
> or send_invalidate_ioreq() at this occasion?
I would prefer function with ioreq_ prefix.


> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -512,6 +512,8 @@ struct domain
> >       /* Argo interdomain communication support */
> >       struct argo_domain *argo;
> >   #endif
> > +
> > +    bool_t qemu_mapcache_invalidate;
> "bool" please.
ok

--
Regards,

Oleksandr Tyshchenko