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=-12.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 D990EC43461 for ; Tue, 8 Sep 2020 13:08:01 +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 651F623BCB for ; Tue, 8 Sep 2020 13:08:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="oSrK0NX8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 651F623BCB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com 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:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=OwD2+/VZY+OodceOSbdDSs372wcWGpVg3lPf7O7WanA=; b=oSrK0NX8T0ZGTaBMp6Mdc0/5V wPp5uX9vSqXpcL0XIRNAjacuRP6L4YWjSnuhiJMKHRlhsDobQ8hV1rlmyjE1/nOo/zWRRcQudj70p uoWp84HVzRROoV8rb/0vvFCQAZE41tA+F5jUPCDXcKwaGm+cwXj+pPM3c9bhe2/oyFukXaoNSOYWP rPdUikA8bR8UW+nQXlsogwtZC2MgLWNVpjlpTiVSOX4fCsUtMusRG9GO+72LDBdr8tnfnq+IvjHZG iBMswvvFqAwFJTfFfWFEwELFO622I4XcAHrKK6qOiip/k0gzwIO0gm5Zy8dEnfWhBEt5ItCKiVAfk bCCTIf45g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFdKb-0000hg-7x; Tue, 08 Sep 2020 13:06:41 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFdKY-0000hF-Tx for linux-arm-kernel@lists.infradead.org; Tue, 08 Sep 2020 13:06:39 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AC75115DB; Tue, 8 Sep 2020 06:06:37 -0700 (PDT) Received: from [192.168.0.110] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C78303F73C; Tue, 8 Sep 2020 06:06:36 -0700 (PDT) Subject: Re: [PATCH v3 09/21] KVM: arm64: Convert unmap_stage2_range() to generic page-table API To: Will Deacon References: <20200825093953.26493-1-will@kernel.org> <20200825093953.26493-10-will@kernel.org> <1f2e88b7-0265-195f-3bd8-4e1d5b8694e3@arm.com> <20200903175702.GA8078@willie-the-truck> From: Alexandru Elisei Message-ID: Date: Tue, 8 Sep 2020 14:07:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20200903175702.GA8078@willie-the-truck> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200908_090639_066068_E54AFFD2 X-CRM114-Status: GOOD ( 22.12 ) 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: Marc Zyngier , kernel-team@android.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, Catalin Marinas 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 Hi Will, On 9/3/20 6:57 PM, Will Deacon wrote: > On Wed, Sep 02, 2020 at 05:23:08PM +0100, Alexandru Elisei wrote: >> On 8/25/20 10:39 AM, Will Deacon wrote: >>> Convert unmap_stage2_range() to use kvm_pgtable_stage2_unmap() instead >>> of walking the page-table directly. >>> >>> Cc: Marc Zyngier >>> Cc: Quentin Perret >>> Signed-off-by: Will Deacon >>> --- >>> arch/arm64/kvm/mmu.c | 57 +++++++++++++++++++++++++------------------- >>> 1 file changed, 32 insertions(+), 25 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c >>> index 704b471a48ce..751ce2462765 100644 >>> --- a/arch/arm64/kvm/mmu.c >>> +++ b/arch/arm64/kvm/mmu.c >>> @@ -39,6 +39,33 @@ static bool is_iomap(unsigned long flags) >>> return flags & KVM_S2PTE_FLAG_IS_IOMAP; >>> } >>> >>> +/* >>> + * Release kvm_mmu_lock periodically if the memory region is large. Otherwise, >>> + * we may see kernel panics with CONFIG_DETECT_HUNG_TASK, >>> + * CONFIG_LOCKUP_DETECTOR, CONFIG_LOCKDEP. Additionally, holding the lock too >>> + * long will also starve other vCPUs. We have to also make sure that the page >>> + * tables are not freed while we released the lock. >>> + */ >>> +#define stage2_apply_range(kvm, addr, end, fn, resched) \ >>> +({ \ >>> + int ret; \ >>> + struct kvm *__kvm = (kvm); \ >>> + bool __resched = (resched); \ >>> + u64 next, __addr = (addr), __end = (end); \ >>> + do { \ >>> + struct kvm_pgtable *pgt = __kvm->arch.mmu.pgt; \ >>> + if (!pgt) \ >>> + break; \ >> I'm 100% sure there's a reason why we've dropped the READ_ONCE, but it still looks >> to me like the compiler might decide to optimize by reading pgt once at the start >> of the loop and stashing it in a register. Would you mind explaining what I am >> missing? > The load always happens with the mmu_lock held, so I think it's not a > problem because it means that the pointer is stable. > spin_lock()/spin_unlock() imply compiler barriers. I think you are correct, if this is supposed to always execute with kvm->mmu_lock held, then pgt should not change between iterations. It didn't immediately occur to me that that is the case because we check if pgt is NULL every iteration. If we are relying on the lock being held, maybe we should move the pgt load + comparison against NULL out of the loop? That should avoid any confusion and make the code ever so slightly faster. Also, I see that in __unmap_stage2_range() we check that the mmu_lock is held, but we don't check that at all call sites (for example, in stage2_wp_range()). I realize this is me bikeshedding, but that looks a bit asymmetrical. Should we move the assert_spin_locked(&kvm->mmu_lock) statement in stage2_apply_range(), since the function assumes the pgt will remain unchanged? What do you think? Thanks, Alex _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel