On Fri, Jun 29, 2018 at 10:21:27AM +0100, Marc Zyngier wrote:
> On Fri, 29 Jun 2018 10:07:50 +0100,
> Christoffer Dall <christoffer.dall@xxxxxxx> wrote:
> >
> > On Fri, Jun 29, 2018 at 09:09:47AM +0100, Marc Zyngier wrote:
> > > On Thu, 28 Jun 2018 21:56:38 +0100,
> > > Christoffer Dall <christoffer.dall@xxxxxxx> 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@xxxxxxx>
> > > > > Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> > > > > ---
> > > > > 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?
> >
> > Yes, this makes sense, but the comment is weird, because it's not
> > because FWB implies IDC that you don't have to clean to PoC, so I would
> > suggest either removing the comment, or rewording it to something like:
> >
> > /*
> > * With FWB we ensure that the guest always accesses memory cacheable
> > * and we don't have to clean to PoC when faulting in pages.
> > * Furthermore, FWB implies IDC, so cleaning to PoU is also not required
> > * in this case.
> > */
> >
> > I think the comment as it stands now, only makes sense in the context of
> > the conversation we've had on this patch on the list.
>
> Fair enough. I'll update the comment to reflect the above.
>
Thanks, with that:
Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxx>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
