Re: [PATCH v3 1/6] arm64: KVM: Add support for Stage-2 control of memory types and cacheability

[Marc Zyngier <marc.zyngier@arm.com>]

On Thu, 28 Jun 2018 21:56:38 +0100,
Christoffer Dall <christoffer.dall@arm.com> wrote:
> 
> On Wed, Jun 27, 2018 at 01:20:53PM +0100, Marc Zyngier wrote:
> > 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 fetches are coherent with
> > 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 <marc.zyngier@arm.com>
> > ---
> >  arch/arm64/include/asm/cpucaps.h      |  3 ++-
> >  arch/arm64/include/asm/kvm_arm.h      |  1 +
> >  arch/arm64/include/asm/kvm_emulate.h  |  2 ++
> >  arch/arm64/include/asm/kvm_mmu.h      | 26 ++++++++++++++++++++------
> >  arch/arm64/include/asm/memory.h       |  7 +++++++
> >  arch/arm64/include/asm/pgtable-prot.h | 14 ++++++++++++--
> >  arch/arm64/include/asm/sysreg.h       |  1 +
> >  arch/arm64/kernel/cpufeature.c        | 20 ++++++++++++++++++++
> >  virt/kvm/arm/mmu.c                    |  4 ++++
> >  9 files changed, 69 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> > index 8a699c708fc9..ed84d6536830 100644
> > --- a/arch/arm64/include/asm/cpucaps.h
> > +++ b/arch/arm64/include/asm/cpucaps.h
> > @@ -49,7 +49,8 @@
> >  #define ARM64_HAS_CACHE_DIC			28
> >  #define ARM64_HW_DBM				29
> >  #define ARM64_SSBD				30
> > +#define ARM64_HAS_STAGE2_FWB			31
> >  
> > -#define ARM64_NCAPS				31
> > +#define ARM64_NCAPS				32
> >  
> >  #endif /* __ASM_CPUCAPS_H */
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index 6dd285e979c9..aa45df752a16 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -23,6 +23,7 @@
> >  #include <asm/types.h>
> >  
> >  /* Hyp Configuration Register (HCR) bits */
> > +#define HCR_FWB		(UL(1) << 46)
> >  #define HCR_TEA		(UL(1) << 37)
> >  #define HCR_TERR	(UL(1) << 36)
> >  #define HCR_TLOR	(UL(1) << 35)
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index 1dab3a984608..dd98fdf33d99 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -63,6 +63,8 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> >  		/* trap error record accesses */
> >  		vcpu->arch.hcr_el2 |= HCR_TERR;
> >  	}
> > +	if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> > +		vcpu->arch.hcr_el2 |= HCR_FWB;
> >  
> >  	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> >  		vcpu->arch.hcr_el2 &= ~HCR_RW;
> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index fb9a7127bb75..620eb9e06bd8 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -257,6 +257,7 @@ static inline bool kvm_page_empty(void *ptr)
> >  struct kvm;
> >  
> >  #define kvm_flush_dcache_to_poc(a,l)	__flush_dcache_area((a), (l))
> > +#define kvm_flush_dcache_to_pou(a,l)	__clean_dcache_area_pou((a), (l))
> >  
> >  static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> >  {
> > @@ -267,6 +268,13 @@ static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> >  {
> >  	void *va = page_address(pfn_to_page(pfn));
> >  
> > +	/*
> > +	 * FWB implies IDC, so cache clean to PoC is not required in
> > +	 * this case.
> > +	 */
> 
> I don't understand this comment.  __clean_dcache_guest_page is called
> when faulting in pages for translation faults (fault_status != FSC_PERM)
> and PoC is about coherency between ram and caches, not instruction and
> data caches, so how is this related to IDC?

There's a shortcut in this comment, so let me develop a tiny bit:

- FWB makes memory the memory cacheable from the PoV of the guest, so
  we don't need to clean to the PoC for the guest to access it even
  when running with the stage-1 MMU off or non-cacheable mappings.

- FWB implies IDC so there is no need to clean to the PoU for
  instruction fetches to get the right thing either.

The combination of these two statements implies that we don't need to
clean anything at all.

Does it make more sense?

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.