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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id C5D10C32789 for ; Tue, 23 Aug 2022 22:48:28 +0000 (UTC) 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=vHLW7w0ePGcncUtCig4h4DoV7IGW7Gpq5C+uCi16M6s=; b=Iymg1xSRXnpqFe ZgC6tzpR0UMnsdshmySmmlnAHpO3S3eurwaylPUPHjyxUqGFBWQbXMn/VPqxDTZ+bMn5zNNOXZULg AoGqNfoY8D3V7UVTNbVFazUtNwJ3wk2CFJn2NpEXwUOOMUYxmlV8ELPkd+1OKjyu+ViLUHrwIK0iC wq+CSDLzeW/47Rg76k3X+uDHXWLC0Bt6iHMBC9UfuT3msyCtBR9k457LmNheCJzDHT0+5ZtT53Zh0 +cPyNsRUufFf7fQ8g3M+fLFZJX9Wf6vivrJDoNqCikPNJ/GB24kVftGikMgN8NpnIZWFI5ZJ6pxwi BRVJ3VcU0qb0gH8QMZ7Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oQcfw-00909r-BC; Tue, 23 Aug 2022 22:47:12 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oQcft-00908k-DG for linux-arm-kernel@lists.infradead.org; Tue, 23 Aug 2022 22:47:11 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 03F68B821E7; Tue, 23 Aug 2022 22:47:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AFC92C433D6; Tue, 23 Aug 2022 22:47:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1661294825; bh=mIdkXaJU12XPc/v1eI1SmEBn2eXK40Tc2GKZ2bh56IQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=PvKWuYH7LzURRWG2uBfTZoqdR2VuQQqVwYk91ZT4xs+pNAX8p0ZPGebqwKGcGvWsw GoUsisFbfXbphw/ArYdEGhpuApqBSfKBE1hCpdK0MtV5idNvsmpHu4izogGKT4agwJ didDFhjjvxPLmLCTKMkhCG7QeQTsf7RLoFgcC6e5tFw4gcSQJDZ6CVaAGQOa6SApau zFP5jzetWE1fQW5n6Ts6y7cE8gcK3rMH6LJsw1t4kBC1LzIOyRQ1weWVya8eBlE6j8 sGSESGojwA+XkGsawcc45JFYMZnl9aP7YRxwddrD68qtzwFb8Eq2/hEwVZR9SjbKfY nxiP9lu/ioTug== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oQcfn-005JQ9-Di; Tue, 23 Aug 2022 23:47:03 +0100 Date: Tue, 23 Aug 2022 23:47:03 +0100 Message-ID: <878rnewpaw.wl-maz@kernel.org> From: Marc Zyngier To: Peter Xu Cc: Gavin Shan , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org, pbonzini@redhat.com, corbet@lwn.net, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, catalin.marinas@arm.com, will@kernel.org, shuah@kernel.org, seanjc@google.com, dmatlack@google.com, bgardon@google.com, ricarkol@google.com, zhenyzha@redhat.com, shan.gavin@gmail.com Subject: Re: [PATCH v1 1/5] KVM: arm64: Enable ring-based dirty memory tracking In-Reply-To: References: <20220819005601.198436-1-gshan@redhat.com> <20220819005601.198436-2-gshan@redhat.com> <87lerkwtm5.wl-maz@kernel.org> <41fb5a1f-29a9-e6bb-9fab-4c83a2a8fce5@redhat.com> <87fshovtu0.wl-maz@kernel.org> <171d0159-4698-354b-8b2f-49d920d03b1b@redhat.com> <87bksawz0w.wl-maz@kernel.org> 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: 185.219.108.64 X-SA-Exim-Rcpt-To: peterx@redhat.com, gshan@redhat.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org, pbonzini@redhat.com, corbet@lwn.net, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, catalin.marinas@arm.com, will@kernel.org, shuah@kernel.org, seanjc@google.com, dmatlack@google.com, bgardon@google.com, ricarkol@google.com, zhenyzha@redhat.com, shan.gavin@gmail.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-20220823_154709_779047_A76C1F25 X-CRM114-Status: GOOD ( 47.99 ) 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 Tue, 23 Aug 2022 22:20:32 +0100, Peter Xu wrote: > > On Tue, Aug 23, 2022 at 08:17:03PM +0100, Marc Zyngier wrote: > > I don't think we really need this check on the hot path. All we need > > is to make the request sticky until userspace gets their act together > > and consumes elements in the ring. Something like: > > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index 986cee6fbc7f..e8ed5e1af159 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -747,6 +747,14 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu) > > > > if (kvm_check_request(KVM_REQ_SUSPEND, vcpu)) > > return kvm_vcpu_suspend(vcpu); > > + > > + if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) && > > + kvm_dirty_ring_soft_full(vcpu)) { > > + kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu); > > + vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL; > > + trace_kvm_dirty_ring_exit(vcpu); > > + return 0; > > + } > > } > > > > return 1; > > Right, this seems working. We can also use kvm_test_request() here. > > > > > > > However, I'm a bit concerned by the reset side of things. It iterates > > over the vcpus and expects the view of each ring to be consistent, > > even if userspace is hacking at it from another CPU. For example, I > > can't see what guarantees that the kernel observes the writes from > > userspace in the order they are being performed (the documentation > > provides no requirements other than "it must collect the dirty GFNs in > > sequence", which doesn't mean much from an ordering perspective). > > > > I can see that working on a strongly ordered architecture, but on > > something as relaxed as ARM, the CPUs may^Wwill aggressively reorder > > stuff that isn't explicitly ordered. I have the feeling that a CAS > > operation on both sides would be enough, but someone who actually > > understands how this works should have a look... > > I definitely don't think I 100% understand all the ordering things since > they're complicated.. but my understanding is that the reset procedure > didn't need memory barrier (unlike pushing, where we have explicit wmb), > because we assumed the userapp is not hostile so logically it should only > modify the flags which is a 32bit field, assuming atomicity guaranteed. Atomicity doesn't guarantee ordering, unfortunately. Take the following example: CPU0 is changing a bunch of flags for GFNs A, B, C, D that exist in the ring in that order, and CPU1 performs an ioctl to reset the page state. CPU0: write_flag(A, KVM_DIRTY_GFN_F_RESET) write_flag(B, KVM_DIRTY_GFN_F_RESET) write_flag(C, KVM_DIRTY_GFN_F_RESET) write_flag(D, KVM_DIRTY_GFN_F_RESET) [...] CPU1: ioctl(KVM_RESET_DIRTY_RINGS) Since CPU0 writes do not have any ordering, CPU1 can observe the writes in a sequence that have nothing to do with program order, and could for example observe that GFN A and D have been reset, but not B and C. This in turn breaks the logic in the reset code (B, C, and D don't get reset), despite userspace having followed the spec to the letter. If each was a store-release (which is the case on x86), it wouldn't be a problem, but nothing calls it in the documentation. Maybe that's not a big deal if it is expected that each CPU will issue a KVM_RESET_DIRTY_RINGS itself, ensuring that it observe its own writes. But expecting this to work across CPUs without any barrier is wishful thinking. > IIRC we used to discuss similar questions on "what if the user is hostile > and wants to hack the process by messing up with the ring", and our > conclusion was as long as the process wouldn't mess up anything outside > itself it should be okay. E.g. It should not be able to either cause the > host to misfunction, or trigger kernel warnings in dmesg, etc.. I'm not even discussing safety here. I'm purely discussing the interactions between userspace and kernel based on the documentation and the existing kernel code. 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