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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 24.09.20 14:16, Jan Beulich wrote:

Hi Jan

> On 22.09.2020 21:32, Oleksandr wrote:
> > On 16.09.20 11:50, Jan Beulich wrote:
> > > On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> > > > --- 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?
> In _this_ series I'd strongly recommend you keep things working as
> they are. If independently you think you've found a reason to
> re-order certain operations, then feel free to send a patch with
> suitable justification.
Of course, I will try to retain current behavior.


> > > 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.
> Well, you need to be concerned whose qemu_mapcache_invalidate flag
> you set.
May I ask, in what cases the *curr_d* is the right domain?

We need to make sure that domain is using IOREQ server(s) at least. 
Hopefully, we have a helper for this
which is hvm_domain_has_ioreq_server(). Please clarify, anything else I 
should taking care of?

--
Regards,

Oleksandr Tyshchenko