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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan