[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




Hi Jan


On 25.09.20 16:05, Oleksandr wrote:

On 25.09.20 10:03, Jan Beulich wrote:

Hi Jan.

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?
No, my question was about *curr_d*. I saw your answer
> I'm also unconvinced curr_d is the right domain in all cases here;
and just wanted to clarify these cases. Sorry if I was unclear.



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.

Thank you for the clarification.

Yes, it would be really nice if you (or maybe Roger) could create a patch for the bug fix part.


Thank you for your patch [1].

If I got it correctly there won't be a suitable common place where to set qemu_mapcache_invalidate flag anymore as XENMEM_decrease_reservation is not a single place we need to make a decision whether to set it By principle of analogy, on Arm we probably want to do so in guest_physmap_remove_page (or maybe better in p2m_remove_mapping).
Julien, what do you think?


I will modify current patch to not alter the common code.


[1] https://patchwork.kernel.org/patch/11803383/

--

Regards,

Oleksandr Tyshchenko




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.