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
