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=-17.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham 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 F07F4C63777 for ; Mon, 30 Nov 2020 16:02:55 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 7B80D20665 for ; Mon, 30 Nov 2020 16:02:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="d+uwqQJB"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="iW4rNUNT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7B80D20665 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=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=2gMGVslGdKABrfh5T5tzJV+G7j5F+0Ko1l2shUWoCrk=; b=d+uwqQJB22Zwn+BgeCu4kgcHI ZadhTZ0258sdp8XfRuXle0SlcoHFrqIGPBJTSrsyOerUojkQy9Guta8HTU3+Wugk+jj2kMk5LMh0J 7n2AJF9M/JpJU+P8uMaBMLmnXfkWXCG+zcLIja7rUGsqm+O0RUrH31WW8uJvEMqMB4jnOAMUbptlZ FIGYkfbFfcMyAcT2IV50VNPAg4ujjSVCWmrFM1ePF6fXZp4cDpMxiweWnQmVMrDdHjtBpsnL4Ej7M LNU9zPl/Xnq2tyvB5/8fvARaIPidUIW1D1sAMlG0Go9pcAp+OTI6bMB+wiS7JIo7nENVceXWEEKRJ wBh0Ci4bQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjlcK-0006UW-Kr; Mon, 30 Nov 2020 16:01:32 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjlcG-0006T0-45 for linux-arm-kernel@lists.infradead.org; Mon, 30 Nov 2020 16:01:29 +0000 Received: from willie-the-truck (236.31.169.217.in-addr.arpa [217.169.31.236]) (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 287A320659; Mon, 30 Nov 2020 16:01:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1606752087; bh=AJw4vWeiUmjVdPrXFfXWymHhQkdX3rroVJei2MKvnPU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=iW4rNUNTz5yBlbL4ymBzBDD3iYfBW4R4mXjd/R8Y6X9Hf951cjxFcYgX7RdhhgpEt 2U/O27yoIfW7rzDgCSZ1CgkkoCdoRU41ItBwMmUxcFN+Ff/C9EF4Pp2SHK+UyGhcMO 9z2Ric/1jW7ut1fIdE1dfr53JzGsZgK17FWm+M40= Date: Mon, 30 Nov 2020 16:01:20 +0000 From: Will Deacon To: "wangyanan (Y)" Subject: Re: [RFC PATCH 2/3] KVM: arm64: Fix handling of merging tables into a block entry Message-ID: <20201130160119.GA25051@willie-the-truck> References: <20201130121847.91808-1-wangyanan55@huawei.com> <20201130121847.91808-3-wangyanan55@huawei.com> <20201130133421.GB24837@willie-the-truck> <67e9e393-1836-eca7-4235-6f4a19fed652@huawei.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <67e9e393-1836-eca7-4235-6f4a19fed652@huawei.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201130_110128_346671_09D14586 X-CRM114-Status: GOOD ( 30.09 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: jiangkunkun@huawei.com, Gavin Shan , Suzuki K Poulose , Marc Zyngier , wangjingyi11@huawei.com, Quentin Perret , lushenming@huawei.com, linux-kernel@vger.kernel.org, yezengruan@huawei.com, James Morse , linux-arm-kernel@lists.infradead.org, Catalin Marinas , yuzenghui@huawei.com, wanghaibin.wang@huawei.com, zhukeqian1@huawei.com, Julien Thierry Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, Cheers for the quick reply. See below for more questions... On Mon, Nov 30, 2020 at 11:24:19PM +0800, wangyanan (Y) wrote: > On 2020/11/30 21:34, Will Deacon wrote: > > On Mon, Nov 30, 2020 at 08:18:46PM +0800, Yanan Wang wrote: > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtabl= e.c > > > index 696b6aa83faf..fec8dc9f2baa 100644 > > > --- a/arch/arm64/kvm/hyp/pgtable.c > > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > > @@ -500,6 +500,9 @@ static int stage2_map_walk_table_pre(u64 addr, u6= 4 end, u32 level, > > > return 0; > > > } > > > +static void stage2_flush_dcache(void *addr, u64 size); > > > +static bool stage2_pte_cacheable(kvm_pte_t pte); > > > + > > > static int stage2_map_walk_leaf(u64 addr, u64 end, u32 level, kvm_p= te_t *ptep, > > > struct stage2_map_data *data) > > > { > > > @@ -507,9 +510,17 @@ static int stage2_map_walk_leaf(u64 addr, u64 en= d, u32 level, kvm_pte_t *ptep, > > > struct page *page =3D virt_to_page(ptep); > > > if (data->anchor) { > > > - if (kvm_pte_valid(pte)) > > > + if (kvm_pte_valid(pte)) { > > > + kvm_set_invalid_pte(ptep); > > > + kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, data->mmu, > > > + addr, level); > > > put_page(page); > > This doesn't make sense to me: the page-table pages we're walking when = the > > anchor is set are not accessible to the hardware walker because we unho= oked > > the entire sub-table in stage2_map_walk_table_pre(), which has the nece= ssary > > TLB invalidation. > > = > > Are you seeing a problem in practice here? > = > Yes, I indeed find a problem in practice. > = > When the migration was cancelled, a TLB conflic abort=A0 was found in gue= st. > = > This problem is fixed before rework of the page table code, you can have a > look in the following two links: > = > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit= /?id=3D3c3736cd32bf5197aed1410ae826d2d254a5b277 > = > https://lists.cs.columbia.edu/pipermail/kvmarm/2019-March/035031.html Ok, let's go through this, because I still don't see the bug. Please correct me if you spot any mistakes: 1. We have a block mapping for X =3D> Y 2. Dirty logging is enabled, so the block mapping is write-protected and ends up being split into page mappings 3. Dirty logging is disabled due to a failed migration. --- At this point, I think we agree that the state of the MMU is alright --- 4. We take a stage-2 fault and want to reinstall the block mapping: a. kvm_pgtable_stage2_map() is invoked to install the block mapping b. stage2_map_walk_table_pre() finds a table where we would like to install the block: i. The anchor is set to point at this entry ii. The entry is made invalid iii. We invalidate the TLB for the input address. This is TLBI IPAS2SE1IS without level hint and then TLBI VMALLE1IS. *** At this point, the page-table pointed to by the old table entry is not reachable to the hardware walker *** c. stage2_map_walk_leaf() is called for each leaf entry in the now-unreachable subtree, dropping page-references for each valid entry it finds. d. stage2_map_walk_table_post() is eventually called for the entry which we cleared back in b.ii, so we install the new block mapping. You are proposing to add additional TLB invalidation to (c), but I don't think that is necessary, thanks to the invalidation already performed in b.iii. What am I missing here? > > > + if (stage2_pte_cacheable(pte)) > > > + stage2_flush_dcache(kvm_pte_follow(pte), > > > + kvm_granule_size(level)); > > I don't understand the need for the flush either, as we're just coalesc= ing > > existing entries into a larger block mapping. > = > In my opinion, after unmapping, it is necessary to ensure the cache > coherency, because it is unknown whether the future mapping memory attrib= ute > is changed or not (cacheable -> non_cacheable) theoretically. But in this case we're just changing the structure of the page-tables, not the pages which are mapped, are we? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel