On 24.09.2020 18:45, Oleksandr wrote: > > 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?
When a domain does a decrease-reservation on itself. I thought that's obvious. But perhaps your question was rather meant a to whether a->domain ever is _not_ the right one? > 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? Nothing I can recall / think of right now, except that the change may want to come under a different title and with a different description. As indicated, I don't think this is correct for PVH Dom0 issuing the request against a HVM DomU, and addressing this will likely want this moved out of hvm_memory_op() anyway. Of course an option is to split this into two patches - the proposed bug fix (perhaps wanting backporting) and then the moving of the field out of arch.hvm. If you feel uneasy about the bug fix part, let me know and I (or maybe Roger) will see to put together a patch. Jan