(CC xen-devel, Ravzan, and Stefao)
Hi Tamas,
Not sure why you only CC me on the answer. I have CCed again xen-devel
as I don't see any sensible discussion in it.
On 26/01/2017 00:11, Tamas K Lengyel wrote:
On Wed, Jan 25, 2017 at 3:41 PM, Julien Grall <julien.grall@xxxxxxx>
wrote:
Hi Tamas,
On 25/01/2017 20:02, Tamas K Lengyel wrote:
During an SMC trap it is possible that the user may change the memory
By user, do you mean the monitor application?
Yes.
It would be worth clarifying in the commit message.
from where the SMC was fetched. However, without flushing the icache
the SMC may still trigger if the pCPU was idle during the trap. Flush
the icache to ensure consistency.
invalidate_icache will invalidate the cache to PoU on all the pCPUs. But
here you only mention a problem on the current pCPU. So which
behavior do
you want to achieve?
It would be sufficient to flush the icache on the specific pCPU that
trapped with the SMC. Didn't see anything defined in Xen to achieve
that.
Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
---
Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
---
xen/arch/arm/monitor.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
index 59ce8f635f..ae1b97993f 100644
--- a/xen/arch/arm/monitor.c
+++ b/xen/arch/arm/monitor.c
@@ -63,6 +63,9 @@ int monitor_smc(void)
.reason = VM_EVENT_REASON_PRIVILEGED_CALL
};
+ /* Nuke the icache as the memory may get changed underneath us. */
+ invalidate_icache();
+
Can you explain why you put this call before the monitor trap and not
after?
From my understanding the monitoring application may change the
memory. But
by the time you modify the instruction, the instruction cache may have
prefetched the previous instruction. So the problem is still there.
Not sure how would that happen exactly? The vCPU gets paused until the
user responds to the vm_event request, so where would it perform the
prefetch again? Also, with this change I've verified that the repeated
SMC issue no longer presents itself (it has been triggered quite
regularly on the hikey before).
The ARM ARM provides a set of expected behavior and where to call the
cache maintenance instruction. If it is not done correctly, it may not
affect your platform but at some point in the future (or even already
today) it will break a platform. I wish you good luck to debug that when
it happens. It is a really nightmare :).
So what I care the most is what is the correct behavior based on the ARM
ARM. A good overview can be found in a talk made by Mark Rutland [1].
What I was concerned about is the instruction cache been shared between
multiple processors (for instance L2 cache or higher). A vCPU could also
get rescheduled to another processor afterwards. Leading to accessing an
out of date instruction cache.
Also, for multi-vCPU guest another vCPU fetching the SMC before the
memory modification happen (ie. just after this flush) is not a
problem and is expected behavior. Providing a non-racy setup for
trapping on multi-vCPU guests requires other work (like altp2m).
I am not that concerned about the SMC itself, but the fact that you may
modify the guest memory whilst it is running. So another vCPU may end up
to execute a mix between new and old instructions depending of the state
of its instruction cache. That would result to a potential undefined
behavior.
Also, you want to make sure that if you write another SMC in memory, it
is effectively affecting all the vCPUs now and not a moment after.
So I still think the invalidate_icache should be done afterwards. IHMO
modifying guest instructions while other vCPU are running is very
fragile as other thread may execute the instructions your are running.
Furthermore, the instruction cache may not snoop the data cache. So
you have
to ensure the data written reached the memory. Otherwise you may read
the
previous instruction. Where is it done? If you expect the monitor app to
flush the data cache, then please comment it.
AFAICT there is no public API for the user to call to request flushing
the caches like that. Memory could get changed by the user any time
under the VM, not just during event callbacks. So I'm not sure where
that comment should be added.
If the monitor app can modify the guest memory at any time. Then you
need to provide an interface to clean the data cache + invalidate the
instruction cache. Otherwise the guest may execute stale instructions.
An hypercall might not be necessary because I think (this need to be
check) that you could do execute both cache flush from the monitor app.
So I would document this in the vm_event header, or in a document
explaining the good practices to implement a monitor app.
OOI, when you are modifying the guest memory, do you pause all the vCPUs?
Lastly, this looks like to me that all the traps will have this
issue. So
maybe this should go in monitor_traps instead?
Memory traps do not have this issue AFAICT as they prohibit the CPU
from fetching the memory to begin with, so the caches wouldn't have
stale contents.
I was not speaking about memory traps but potential other system
register traps (TTBR,...) that would be added in the future.
Regarding the memory trap. I think you can get into the same problem if
you decide to forbid read-write but allow instruction fetch. You would
need some cache maintenance instruction here if you decide to modify the
memory.
Tamas
[1]
https://events.linuxfoundation.org/sites/events/files/slides/slides_17.pdf
