From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 45E0AC4708F for ; Wed, 2 Jun 2021 10:21:22 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0FE16613B8 for ; Wed, 2 Jun 2021 10:21:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0FE16613B8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=0N+/e1e68pfQMnM3hjuDtYvl2dtEmL4hQxsMWSwnrrQ=; b=pZ3uWvCeCZyRc/ ck+OMkS8/NHmDKc3My2JB59ezsq9WQcmaVbE54D3+iCLhWNV2mYni1XZOZZOjBCbs+4nUW5gks/7p T5vzH4oMfEGACOLS0GDviyP2v+O2nUyD+JYaCdu8Wh0LYKU2HWOBj64X5jyqILbc+QvkxMauKDR9E Ei8nJbFHE6gokNtyWwNZtgPwByUzttxRNZhk8tGb5ZGP9MnCHHQnCRUskfL+69TN10hIJP0hOK8Fe Y/t08o3YWY6oxhawSNFpaeVXpC5hi8AVrH4PMoVaqJVfrjXpFwP2QXCK84mS12uZwDCl5zADT8eQX SMYgxSRhet4Kq2xWjWSw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1loNyh-003GSQ-HH; Wed, 02 Jun 2021 10:19:59 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1loNyc-003GR0-NX for linux-arm-kernel@lists.infradead.org; Wed, 02 Jun 2021 10:19:56 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id D8487613BA; Wed, 2 Jun 2021 10:19:53 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1loNyZ-0050AU-C9; Wed, 02 Jun 2021 11:19:51 +0100 Date: Wed, 02 Jun 2021 11:19:49 +0100 Message-ID: <877djc1sca.wl-maz@kernel.org> From: Marc Zyngier To: Yanan Wang Cc: Will Deacon , "Quentin\ Perret" , Alexandru Elisei , , , , , Catalin Marinas , James Morse , Julien Thierry , "Suzuki K Poulose" , Gavin Shan , , , Subject: Re: [PATCH v5 2/6] KVM: arm64: Move D-cache flush to the fault handlers In-Reply-To: <20210415115032.35760-3-wangyanan55@huawei.com> References: <20210415115032.35760-1-wangyanan55@huawei.com> <20210415115032.35760-3-wangyanan55@huawei.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: wangyanan55@huawei.com, will@kernel.org, qperret@google.com, alexandru.elisei@arm.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, gshan@redhat.com, wanghaibin.wang@huawei.com, zhukeqian1@huawei.com, yuzenghui@huawei.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210602_031954_871489_3C00D842 X-CRM114-Status: GOOD ( 40.31 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 15 Apr 2021 12:50:28 +0100, Yanan Wang wrote: > > We currently uniformly permorm CMOs of D-cache and I-cache in function > user_mem_abort before calling the fault handlers. If we get concurrent > guest faults(e.g. translation faults, permission faults) or some really > unnecessary guest faults caused by BBM, CMOs for the first vcpu are > necessary while the others later are not. > > By moving CMOs to the fault handlers, we can easily identify conditions > where they are really needed and avoid the unnecessary ones. As it's a > time consuming process to perform CMOs especially when flushing a block > range, so this solution reduces much load of kvm and improve efficiency > of the page table code. > > This patch only moves clean of D-cache to the map path, and drop the > original APIs in mmu.c/mmu.h for D-cache maintenance by using what we > already have in pgtable.c. Change about the I-side will come from a > later patch. But this means that until patch #5, this is broken (invalidation on the i-side will happen before the clean to PoU on the d-side). You need to keep the series bisectable and not break things in the middle. It would be OK to have two set of D-cache CMOs in the interval, for example. > > Signed-off-by: Yanan Wang > --- > arch/arm64/include/asm/kvm_mmu.h | 16 ---------------- > arch/arm64/kvm/hyp/pgtable.c | 20 ++++++++++++++------ > arch/arm64/kvm/mmu.c | 14 +++----------- > 3 files changed, 17 insertions(+), 33 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h > index 25ed956f9af1..e9b163c5f023 100644 > --- a/arch/arm64/include/asm/kvm_mmu.h > +++ b/arch/arm64/include/asm/kvm_mmu.h > @@ -187,22 +187,6 @@ static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu) > return (vcpu_read_sys_reg(vcpu, SCTLR_EL1) & 0b101) == 0b101; > } > > -static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size) > -{ > - void *va = page_address(pfn_to_page(pfn)); > - > - /* > - * With FWB, we ensure that the guest always accesses memory using > - * cacheable attributes, and we don't have to clean to PoC when > - * faulting in pages. Furthermore, FWB implies IDC, so cleaning to > - * PoU is not required either in this case. > - */ > - if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) > - return; > - > - kvm_flush_dcache_to_poc(va, size); > -} > - > static inline void __invalidate_icache_guest_page(kvm_pfn_t pfn, > unsigned long size) > { > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index c37c1dc4feaf..e3606c9dcec7 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -562,6 +562,12 @@ static bool stage2_pte_is_counted(kvm_pte_t pte) > return !!pte; > } > > +static bool stage2_pte_cacheable(struct kvm_pgtable *pgt, kvm_pte_t pte) > +{ > + u64 memattr = pte & KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR; > + return memattr == KVM_S2_MEMATTR(pgt, NORMAL); > +} > + > static void stage2_put_pte(kvm_pte_t *ptep, struct kvm_s2_mmu *mmu, u64 addr, > u32 level, struct kvm_pgtable_mm_ops *mm_ops) > { > @@ -583,6 +589,7 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, > { > kvm_pte_t new, old = *ptep; > u64 granule = kvm_granule_size(level), phys = data->phys; > + struct kvm_pgtable *pgt = data->mmu->pgt; > struct kvm_pgtable_mm_ops *mm_ops = data->mm_ops; > > if (!kvm_block_mapping_supported(addr, end, phys, level)) > @@ -606,6 +613,13 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, > stage2_put_pte(ptep, data->mmu, addr, level, mm_ops); > } > > + /* Perform CMOs before installation of the guest stage-2 PTE */ > + if (pgt->flags & KVM_PGTABLE_S2_GUEST) { > + if (stage2_pte_cacheable(pgt, new) && !stage2_has_fwb(pgt)) > + __flush_dcache_area(mm_ops->phys_to_virt(phys), > + granule); > + } Rather than this, why not provide new callbacks in mm_ops, even if we have to provide one that is specific to guests (and let the protected stuff do its own thing)? One thing I really dislike though is that the page-table code is starting to be littered with things that are not directly related to page tables. We are re-creating the user_mem_abort() mess in a different place. > + > smp_store_release(ptep, new); > if (stage2_pte_is_counted(new)) > mm_ops->get_page(ptep); > @@ -798,12 +812,6 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > return ret; > } > > -static bool stage2_pte_cacheable(struct kvm_pgtable *pgt, kvm_pte_t pte) > -{ > - u64 memattr = pte & KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR; > - return memattr == KVM_S2_MEMATTR(pgt, NORMAL); > -} > - > static int stage2_unmap_walker(u64 addr, u64 end, u32 level, kvm_pte_t *ptep, > enum kvm_pgtable_walk_flags flag, > void * const arg) > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 2cfcfc5f4e4e..86f7dd1c234f 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -694,11 +694,6 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm, > kvm_mmu_write_protect_pt_masked(kvm, slot, gfn_offset, mask); > } > > -static void clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size) > -{ > - __clean_dcache_guest_page(pfn, size); > -} > - > static void invalidate_icache_guest_page(kvm_pfn_t pfn, unsigned long size) > { > __invalidate_icache_guest_page(pfn, size); > @@ -972,9 +967,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > if (writable) > prot |= KVM_PGTABLE_PROT_W; > > - if (fault_status != FSC_PERM && !device) > - clean_dcache_guest_page(pfn, vma_pagesize); > - > if (exec_fault) { > prot |= KVM_PGTABLE_PROT_X; > invalidate_icache_guest_page(pfn, vma_pagesize); > @@ -1234,10 +1226,10 @@ int kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte) > trace_kvm_set_spte_hva(hva); > > /* > - * We've moved a page around, probably through CoW, so let's treat it > - * just like a translation fault and clean the cache to the PoC. > + * We've moved a page around, probably through CoW, so let's treat > + * it just like a translation fault and the map handler will clean > + * the cache to the PoC. > */ > - clean_dcache_guest_page(pfn, PAGE_SIZE); > handle_hva_to_gpa(kvm, hva, end, &kvm_set_spte_handler, &pfn); > return 0; > } Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel