Hi Alex,
On 2020-05-30 11:46, Alexandru Elisei wrote:
Hi,
[...]
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 48d0ec44ad77..e6378162cdef 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -983,8 +983,11 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct
kvm_vcpu *vcpu,
/*
* Ensure a rebooted VM will fault in RAM pages and detect if the
* guest MMU is turned off and flush the caches as needed.
+ *
+ * S2FWB enforces all memory accesses to RAM being cacheable, we
+ * ensure that the cache is always coherent.
*/
- if (vcpu->arch.has_run_once)
+ if (vcpu->arch.has_run_once &&
!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
I think userspace does not invalidate the icache when loading a new
kernel image,
and if the guest patched instructions, they could potentially still be
in the
icache. Should the icache be invalidated if FWB is present?
I noticed that this was included in the current pull request and I
remembered that
I wasn't sure about this part. Did some more digging and it turns out
that FWB
implies no cache maintenance needed for *data to instruction*
coherence. From ARM
DDI 0487F.b, page D5-2635:
"When ARMv8.4-S2FWB is implemented, the architecture requires that
CLIDR_EL1.{LOUU, LOIUS} are zero so that no levels of data cache need
to be
cleaned in order to manage coherency with instruction fetches".
However, there's no mention that I found for instruction to data
coherence,
meaning that the icache would still need to be invalidated on each vcpu
in order
to prevent fetching of patched instructions from the icache. Am I
missing something?
I think you are right, and this definitely matches the way we deal with
the icache on the fault path. For some bizarre reason, I always assume
that FWB implies DIC, which isn't true at all.
I'm planning to address it as follows. Please let me know what you
think.
Thanks,
M.
From f7860d1d284f41afea176cc17e5c9d895ae665e9 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@xxxxxxxxxx>
Date: Sat, 30 May 2020 17:22:19 +0100
Subject: [PATCH] KVM: arm64: Flush the instruction cache if not
unmapping the
VM on reboot
On a system with FWB, we don't need to unmap Stage-2 on reboot,
as even if userspace takes this opportunity to repaint the whole
of memory, FWB ensures that the data side stays consistent even
if the guest uses non-cacheable mappings.
However, the I-side is not necessarily coherent with the D-side
if CTR_EL0.DIC is 0. In this case, invalidate the i-cache to
preserve coherency.
Reported-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
Fixes: 892713e97ca1 ("KVM: arm64: Sidestep stage2_unmap_vm() on vcpu
reset when S2FWB is supported")
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
---
arch/arm64/kvm/arm.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index b0b569f2cdd0..d6988401c22a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -989,11 +989,17 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct
kvm_vcpu *vcpu,
* Ensure a rebooted VM will fault in RAM pages and detect if the
* guest MMU is turned off and flush the caches as needed.
*
- * S2FWB enforces all memory accesses to RAM being cacheable, we
- * ensure that the cache is always coherent.
+ * S2FWB enforces all memory accesses to RAM being cacheable,
+ * ensuring that the data side is always coherent. We still
+ * need to invalidate the I-cache though, as FWB does *not*
+ * imply CTR_EL0.DIC.
*/
- if (vcpu->arch.has_run_once &&
!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
- stage2_unmap_vm(vcpu->kvm);
+ if (vcpu->arch.has_run_once) {
+ if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB))
+ stage2_unmap_vm(vcpu->kvm);
+ else
+ __flush_icache_all();
+ }
vcpu_reset_hcr(vcpu);
--
2.26.2
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
