Message ID | 20221129191946.1735662-3-oliver.upton@linux.dev |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp525008wrr; Tue, 29 Nov 2022 11:23:12 -0800 (PST) X-Google-Smtp-Source: AA0mqf4yGBInrK6gQgGY+ivZuFKuA4OD4REL6l8FVi80v89GCN+n41QD++goJ/AW1qBr9B93TFES X-Received: by 2002:a05:6402:1802:b0:461:72cb:e5d with SMTP id g2-20020a056402180200b0046172cb0e5dmr44659444edy.410.1669749792089; Tue, 29 Nov 2022 11:23:12 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669749792; cv=none; d=google.com; s=arc-20160816; b=GJamiKCY03PM5x9kFExIEV5RgkwoTxupJX7w9VTVBHJyYD1aPeS+jCGSvwY2LnCDFR tjipyDlk/bVPFiQhh5oKDEZtMl6NT8DVF4C1C0onSeQr/Stk2RvgiIouHXNXc5/zxAyH g63oYv8Vr5PbQP5h2lSwlfSEkBh9CTglI9CptDyPYjhRN6iQho9DbK/iaWBtQK8irhtr cEkpVwimQ1/SE+bWGD+QBVT2skpOWIvp0HPsXsyVZpv76f5V4ARxG4FZlCosrzXHsWqf NB+WntrZu/j/kUQNL0Bp6XlTeJDny3l8TJ6BpMr+3DlMK1UrxjeyzHv3Niu3H2XL0tFp SxKg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=gZ4zOeCGbv1gvItGbIIGPQNuQz+qC/NQiSdoYbMqOcA=; b=YUbr2rTIfknlGxcoyP3r0f//SVbqMyHFDdQV4Dle+BFjOkY27aWJcr3wIhrWxFzHZ4 XsNqEYqNiXDapatCr9SAUzVxGZd5kSG+vwG5Hw/ejEhkErjJs+wMnR/26hyoLUGSLQQf UGQdGzjyhbNq4He2+TsohqRKwxcgXGjilw3LestvvTnMdT8CZtVGBJ5Iu/NSn9kWQqir TxjKGIvrENMc2XDxMuvjTMO3EtVaEeFSo/QIjeKVoDml4se+TniCeuQu6LEjt/zWRLiz srzjuk6HnXZO+5Prr//d6btft9KyPKFHYYWhugzMd+CI6ynTlPXUen71zWPYm7ZjC/g5 eAXw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=ivgJME64; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id dr12-20020a170907720c00b007c093acadb0si895999ejc.508.2022.11.29.11.22.47; Tue, 29 Nov 2022 11:23:12 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=ivgJME64; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236902AbiK2TVc (ORCPT <rfc822;rbbytesnap@gmail.com> + 99 others); Tue, 29 Nov 2022 14:21:32 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56346 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236815AbiK2TUv (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 29 Nov 2022 14:20:51 -0500 Received: from out2.migadu.com (out2.migadu.com [IPv6:2001:41d0:2:aacc::]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 486FD6D48C; Tue, 29 Nov 2022 11:20:01 -0800 (PST) X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1669749599; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gZ4zOeCGbv1gvItGbIIGPQNuQz+qC/NQiSdoYbMqOcA=; b=ivgJME643G8i5ESEr5NCoBRxVONgW7xNcfo9eS52aTmAiibYNqYar1tSVUGNHBZlpVpKsW fZVLO+TVjMk1oxRvpreTbTajXlBarEJAum8hCMBQG4qntEG7Y43uEXKfviUFRYDfwDIrMV rRTPphdF0qeiMRSwSaQinxn6tNZH9jk= From: Oliver Upton <oliver.upton@linux.dev> To: Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>, Alexandru Elisei <alexandru.elisei@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Oliver Upton <oliver.upton@linux.dev>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org> Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH 2/4] KVM: arm64: Don't serialize if the access flag isn't set Date: Tue, 29 Nov 2022 19:19:44 +0000 Message-Id: <20221129191946.1735662-3-oliver.upton@linux.dev> In-Reply-To: <20221129191946.1735662-1-oliver.upton@linux.dev> References: <20221129191946.1735662-1-oliver.upton@linux.dev> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1750859558048241282?= X-GMAIL-MSGID: =?utf-8?q?1750859558048241282?= |
Series |
[1/4] KVM: arm64: Use KVM's pte type/helpers in handle_access_fault()
|
|
Commit Message
Oliver Upton
Nov. 29, 2022, 7:19 p.m. UTC
Of course, if the PTE wasn't changed then there are absolutely no
serialization requirements. Skip the DSB for an unsuccessful update to
the access flag.
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Comments
On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote: > Of course, if the PTE wasn't changed then there are absolutely no > serialization requirements. Skip the DSB for an unsuccessful update to > the access flag. > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > arch/arm64/kvm/hyp/pgtable.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index b11cf2c618a6..9626f615d9b8 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -1094,9 +1094,13 @@ int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 size) > kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr) > { > kvm_pte_t pte = 0; > - stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, > - &pte, NULL, 0); > - dsb(ishst); > + int ret; > + > + ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, > + &pte, NULL, 0); > + if (!ret) > + dsb(ishst); At the moment, the only reason for stage2_update_leaf_attrs() to not update the PTE is if it's not valid: if (!kvm_pte_valid(pte)) return 0; I guess you could check that as well: + if (!ret || kvm_pte_valid(pte)) + dsb(ishst); > + > return pte; > } > > -- > 2.38.1.584.g0f3c55d4c2-goog > >
Hi Ricardo, Thanks for having a look. On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote: > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote: [...] > > + ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, > > + &pte, NULL, 0); > > + if (!ret) > > + dsb(ishst); > > At the moment, the only reason for stage2_update_leaf_attrs() to not > update the PTE is if it's not valid: > > if (!kvm_pte_valid(pte)) > return 0; > > I guess you could check that as well: > > + if (!ret || kvm_pte_valid(pte)) > + dsb(ishst); Thanks for catching this. Instead of pivoting on the returned PTE value, how about we return -EAGAIN from the early return in stage2_attr_walker()? It would better match the pattern used elsewhere in the pgtable code. -- Thanks, Oliver
On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote: > Hi Ricardo, > > Thanks for having a look. > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote: > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote: > > [...] > > > > + ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, > > > + &pte, NULL, 0); > > > + if (!ret) > > > + dsb(ishst); > > > > At the moment, the only reason for stage2_update_leaf_attrs() to not > > update the PTE is if it's not valid: > > > > if (!kvm_pte_valid(pte)) > > return 0; > > > > I guess you could check that as well: > > > > + if (!ret || kvm_pte_valid(pte)) > > + dsb(ishst); > > Thanks for catching this. > > Instead of pivoting on the returned PTE value, how about we return > -EAGAIN from the early return in stage2_attr_walker()? It would better > match the pattern used elsewhere in the pgtable code. That works, although I would use another return code (e.g., EINVAL)? as that's not exactly a "try again" type of error. > > -- > Thanks, > Oliver
On Wed, 30 Nov 2022 01:23:20 +0000, Ricardo Koller <ricarkol@google.com> wrote: > > On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote: > > Hi Ricardo, > > > > Thanks for having a look. > > > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote: > > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote: > > > > [...] > > > > > > + ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, > > > > + &pte, NULL, 0); > > > > + if (!ret) > > > > + dsb(ishst); > > > > > > At the moment, the only reason for stage2_update_leaf_attrs() to not > > > update the PTE is if it's not valid: > > > > > > if (!kvm_pte_valid(pte)) > > > return 0; > > > > > > I guess you could check that as well: > > > > > > + if (!ret || kvm_pte_valid(pte)) > > > + dsb(ishst); > > > > Thanks for catching this. > > > > Instead of pivoting on the returned PTE value, how about we return > > -EAGAIN from the early return in stage2_attr_walker()? It would better > > match the pattern used elsewhere in the pgtable code. > > That works, although I would use another return code (e.g., EINVAL)? as > that's not exactly a "try again" type of error. EINVAL usually is an indication of something that went horribly wrong. But is that really a failure mode? Here, failing to update the PTE should not be considered a failure, but just a benign race: access fault being taken on a CPU and the page being evicted on another (not unlikely, as the page was marked old before). And if I'm correct above, this is definitely a "try again" situation: you probably won't take the same type of fault the second time though. Thanks, M.
On Wed, Nov 30, 2022 at 08:21:17AM +0000, Marc Zyngier wrote: > On Wed, 30 Nov 2022 01:23:20 +0000, > Ricardo Koller <ricarkol@google.com> wrote: > > > > On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote: > > > Hi Ricardo, > > > > > > Thanks for having a look. > > > > > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote: > > > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote: > > > > > > [...] > > > > > > > > + ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, > > > > > + &pte, NULL, 0); > > > > > + if (!ret) > > > > > + dsb(ishst); > > > > > > > > At the moment, the only reason for stage2_update_leaf_attrs() to not > > > > update the PTE is if it's not valid: > > > > > > > > if (!kvm_pte_valid(pte)) > > > > return 0; > > > > > > > > I guess you could check that as well: > > > > > > > > + if (!ret || kvm_pte_valid(pte)) > > > > + dsb(ishst); > > > > > > Thanks for catching this. > > > > > > Instead of pivoting on the returned PTE value, how about we return > > > -EAGAIN from the early return in stage2_attr_walker()? It would better > > > match the pattern used elsewhere in the pgtable code. > > > > That works, although I would use another return code (e.g., EINVAL)? as > > that's not exactly a "try again" type of error. > > EINVAL usually is an indication of something that went horribly wrong. > > But is that really a failure mode? Here, failing to update the PTE > should not be considered a failure, but just a benign race: access > fault being taken on a CPU and the page being evicted on another (not > unlikely, as the page was marked old before). I see, I agree, what you describe not look like a failure. > > And if I'm correct above, this is definitely a "try again" situation: > you probably won't take the same type of fault the second time though. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. >
On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote: > Hi Ricardo, > > Thanks for having a look. > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote: > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote: > > [...] > > > > + ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, > > > + &pte, NULL, 0); > > > + if (!ret) > > > + dsb(ishst); > > > > At the moment, the only reason for stage2_update_leaf_attrs() to not > > update the PTE is if it's not valid: > > > > if (!kvm_pte_valid(pte)) > > return 0; > > > > I guess you could check that as well: > > > > + if (!ret || kvm_pte_valid(pte)) > > + dsb(ishst); > > Thanks for catching this. > > Instead of pivoting on the returned PTE value, how about we return > -EAGAIN from the early return in stage2_attr_walker()? It would better > match the pattern used elsewhere in the pgtable code. Bugh... Returning EAGAIN has some unfortunate consequences that I've missed until now... The stage2 attr walker is used to handle faults as well as range-based operations. In the former case, EAGAIN is sane as we retry execution but the latter is not. I stupidly got hung up on write protection not working as intended for some time. I think that callers into the page table walker should indicate whether or not the walk is to address a fault. If it is not, __kvm_pgtable_visit() and __kvm_pgtable_walk() should chug along instead of bailing for EAGAIN. Let me mess around with this and figure out what is least ugly. -- Thanks, Oliver
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index b11cf2c618a6..9626f615d9b8 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -1094,9 +1094,13 @@ int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 size) kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr) { kvm_pte_t pte = 0; - stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, - &pte, NULL, 0); - dsb(ishst); + int ret; + + ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0, + &pte, NULL, 0); + if (!ret) + dsb(ishst); + return pte; }