On 31/05/18 13:38, Marc Zyngier wrote:

On 31/05/18 12:49, Mark Rutland wrote:

quoted

On Wed, May 30, 2018 at 01:47:01PM +0100, Marc Zyngier wrote:

quoted

Up to ARMv8.3, the combinaison of Stage-1 and Stage-2 attributes
results in the strongest attribute of the two stages.  This means
that the hypervisor has to perform quite a lot of cache maintenance
just in case the guest has some non-cacheable mappings around.

ARMv8.4 solves this problem by offering a different mode (FWB) where
Stage-2 has total control over the memory attribute (this is limited
to systems where both I/O and instruction caches are coherent with

s/caches/fetches/ -- the I-caches themselves aren't coherent with the
D-caches (or we could omit I-cache maintenance).

i.e. this implies IDC, but not DIC.

It may imply IDC behaviour, but not quite IDC itself. I agree, this
looks dodgy. I've asked for clarification on the spec.

I've now received confirmation that FWB implies the IDC behaviour. It
doesn't guarantee that CTR_EL0.IDC will be set though, only that
CLIDR_EL1.LOU{U,IS} are both 0.

quoted

quoted

the dcache). This is achieved by having a different set of memory
attributes in the page tables, and a new bit set in HCR_EL2.

On such a system, we can then safely sidestep any form of dcache
management.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <redacted>
---

quoted

 static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
 {

@@ -268,7 +269,10 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
 {
 	void *va = page_address(pfn_to_page(pfn));
 
-	kvm_flush_dcache_to_poc(va, size);
+	if (!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
+		kvm_flush_dcache_to_poc(va, size);
+	else
+		kvm_flush_dcache_to_pou(va, size);
 }

Te commit message said instruction fetches were coherent, and that no
D-cache maintenance was necessary, so why do we need maintenance to the
PoU?

That maintenance will be elided if we actually have IDC set. I'm happy
to drop it once I have confirmation that we have an identical behaviour.

Given the above, I'll drop the clean to PoU.

quoted

quoted

+static void cpu_has_fwb(const struct arm64_cpu_capabilities *__unused)
+{
+	u64 val = read_sysreg_s(SYS_CLIDR_EL1);
+
+	/* Check that CLIDR_EL1.LOU{U,IS} are both 0 */
+	WARN_ON(val & (7 << 27 | 7 << 21));
+}

What about CTR_EL0.IDC?

Again, that depends on whether FWB implies IDC or not.

The spec doesn't seem to guarantee IDC, but does mandate that
CLIDR_EL1.LOU{U,IS} are set 0.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...