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=-0.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no 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 B565AC2D0DB for ; Fri, 31 Jan 2020 22:16:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 875702082E for ; Fri, 31 Jan 2020 22:16:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="cJToakrM" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726163AbgAaWQn (ORCPT ); Fri, 31 Jan 2020 17:16:43 -0500 Received: from us-smtp-2.mimecast.com ([205.139.110.61]:34086 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726180AbgAaWQn (ORCPT ); Fri, 31 Jan 2020 17:16:43 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580509002; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=vNWAMFUjo2BxvgCqYLA09WvnKRsNZUCV/NyH0B71vPw=; b=cJToakrMoYYh4Y/qTnKLvU1c70LxnQWG9aaDAVeIOctQReXHaXPKpVCdauUkNVGEchL8Yg AR9NDF0PsfX/yhCgiHtD9jnm4NJjOIXv99pcKDAMDo/FeBsq5ViRjJNxFNWpcEvW9DWsLQ mzZkv2oNf+j0Zu7zIKMqDfQhofx2/Ls= Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-332-lqCLY4KUPA29G1Pecx6SSg-1; Fri, 31 Jan 2020 17:16:40 -0500 X-MC-Unique: lqCLY4KUPA29G1Pecx6SSg-1 Received: by mail-qv1-f71.google.com with SMTP id cn2so5374468qvb.1 for ; Fri, 31 Jan 2020 14:16:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=vNWAMFUjo2BxvgCqYLA09WvnKRsNZUCV/NyH0B71vPw=; b=cjgfGzP7fmDDacJvsxvZhi5ycpTNBxgyeqDLLK8q7sjfXbCz/U/GT9MeScco6b0S1T vhuPVZz2myhL19PZo3hMW1ulhxBDo9nYUdFFfXRMCGBF2j/xYtJyhxSzrpmHz9YOxc9Y mTNW/6xU+yaB5ZNdy66qOvJkYygINHBfelEWkjoJEs0eCNX/33T5OryJ5Sb3//nP6K+b P/1Lx0eRonBBVJ9T6f6e4+ldVAGRQE/x66A7SzwJPcSNhDq/GvkhteFRs7mq99LPq/mA dI7BzNCShiU3sRWGye3fS6Q1B97ogDnY/giMBMXZylnrCcwUzeds2of1TdF9PU7PnY6E 7AIA== X-Gm-Message-State: APjAAAXz6vlJoPMgxKZGNuxU1UoNPzeZqwq7vZ9dqQjLo5LeHy9IW5nx noH1tmSfs+owaZEkuHtwEWsbJ0L3fxx5ZmmNutUSUoM2dZuqFw+kV1Odh9CzvpMe6rcPCsfHH6V FqPb0lMP58HBodAWLj+PX2FCH X-Received: by 2002:a05:620a:9d9:: with SMTP id y25mr12995303qky.41.1580508999714; Fri, 31 Jan 2020 14:16:39 -0800 (PST) X-Google-Smtp-Source: APXvYqyFBEIl/LpAFBCeYudTZUb4k/rDVnIohho0H6H5nDgOCPBmQQeP0I4pvYuc2vcz1diZHlcEEw== X-Received: by 2002:a05:620a:9d9:: with SMTP id y25mr12995274qky.41.1580508999416; Fri, 31 Jan 2020 14:16:39 -0800 (PST) Received: from xz-x1 ([2607:9880:19c8:32::2]) by smtp.gmail.com with ESMTPSA id u24sm5247491qkm.40.2020.01.31.14.16.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 31 Jan 2020 14:16:38 -0800 (PST) Date: Fri, 31 Jan 2020 17:16:37 -0500 From: Peter Xu To: Sean Christopherson Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Christophe de Dinechin , "Michael S . Tsirkin" , Paolo Bonzini , Yan Zhao , Alex Williamson , Jason Wang , Kevin Kevin , Vitaly Kuznetsov , "Dr . David Alan Gilbert" Subject: Re: [PATCH v3 09/21] KVM: X86: Don't track dirty for KVM_SET_[TSS_ADDR|IDENTITY_MAP_ADDR] Message-ID: <20200131221637.GC7063@xz-x1> References: <20200109145729.32898-10-peterx@redhat.com> <20200121155657.GA7923@linux.intel.com> <20200128055005.GB662081@xz-x1> <20200128182402.GA18652@linux.intel.com> <20200131150832.GA740148@xz-x1> <20200131193301.GC18946@linux.intel.com> <20200131202824.GA7063@xz-x1> <20200131203622.GF18946@linux.intel.com> <20200131205550.GB7063@xz-x1> <20200131212928.GH18946@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200131212928.GH18946@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 31, 2020 at 01:29:28PM -0800, Sean Christopherson wrote: > On Fri, Jan 31, 2020 at 03:55:50PM -0500, Peter Xu wrote: > > On Fri, Jan 31, 2020 at 12:36:22PM -0800, Sean Christopherson wrote: > > > On Fri, Jan 31, 2020 at 03:28:24PM -0500, Peter Xu wrote: > > > > On Fri, Jan 31, 2020 at 11:33:01AM -0800, Sean Christopherson wrote: > > > > > For the same reason we don't take mmap_sem, it gains us nothing, i.e. KVM > > > > > still has to use copy_{to,from}_user(). > > > > > > > > > > In the proposed __x86_set_memory_region() refactor, vmx_set_tss_addr() > > > > > would be provided the hva of the memory region. Since slots_lock and SRCU > > > > > only protect gfn->hva, why would KVM take slots_lock since it already has > > > > > the hva? > > > > > > > > OK so you're suggesting to unlock the lock earlier to not cover > > > > init_rmode_tss() rather than dropping the whole lock... Yes it looks > > > > good to me. I think that's the major confusion I got. > > > > > > Ya. And I missed where the -EEXIST was coming from. I think we're on the > > > same page. > > > > Good to know. Btw, for me I would still prefer to keep the lock be > > after the __copy_to_user()s because "HVA is valid without lock" is > > only true for these private memslots. > > No. From KVM's perspective, the HVA is *never* valid. Even if you rewrote > this statement to say "the gfn->hva translation is valid without lock" it > would still be incorrect. > > KVM is *always* using HVAs without holding lock, e.g. every time it enters > the guest it is deferencing a memslot because the translations stored in > the TLB are effectively gfn->hva->hpa. Obviously KVM ensures that it won't > dereference a memslot that has been deleted/moved, but it's a lot more > subtle than simply holding a lock. > > > After all this is super slow path so I wouldn't mind to take the lock > > for some time longer. > > Holding the lock doesn't affect this super slow vmx_set_tss_addr(), it > affects everything else that wants slots_lock. Now, admittedly it's > extremely unlikely userspace is going to do KVM_SET_USER_MEMORY_REGION in > parallel, but that's not the point and it's not why I'm objecting to > holding the lock. > > Holding the lock implies protection that is *not* provided. You and I know > it's not needed for copy_{to,from}_user(), but look how long it's taken us > to get on the same page. A future KVM developer comes along, sees this > code, and thinks "oh, I need to hold slots_lock to dereference a gfn", and > propagates the unnecessary locking to some other code. At least for a user memory slot, we "need to hold slots_lock to dereference a gfn" (or srcu), right? You know I'm suffering from a jetlag today, I thought I was still fine, now I start to doubt it. :-) > > > Or otherwise if you really like the unlock() to > > be earlier I can comment above the unlock: > > > > /* > > * We can unlock before using the HVA only because this KVM private > > * memory slot will never change until the end of VM lifecycle. > > */ > > How about: > > /* > * No need to hold slots_lock while filling the TSS, the TSS private > * memslot is guaranteed to be valid until the VM is destroyed, i.e. > * there is no danger of corrupting guest memory by consuming a stale > * gfn->hva lookup. > */ Sure for this. -- Peter Xu