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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id E442EC433FE for ; Tue, 8 Nov 2022 08:28:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4A36C6B0071; Tue, 8 Nov 2022 03:28:51 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 452B48E0002; Tue, 8 Nov 2022 03:28:51 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2F3A88E0001; Tue, 8 Nov 2022 03:28:51 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 1F1916B0071 for ; Tue, 8 Nov 2022 03:28:51 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id E7BCE1C6578 for ; Tue, 8 Nov 2022 08:28:50 +0000 (UTC) X-FDA: 80109599220.11.0CAA021 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by imf28.hostedemail.com (Postfix) with ESMTP id 02E67C0009 for ; Tue, 8 Nov 2022 08:28:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1667896130; x=1699432130; h=date:from:to:cc:subject:message-id:reply-to:references: mime-version:in-reply-to; bh=WIPURCPIsIJH60CXb2Fq3UuwkzckzrtWTTFyYgbY51I=; b=PednyWZwYKpwc5mtuhRnYin0r7N+sSakywSTS+yDPCFh2ETVIZ8L7Dca 0/NlpYRfBUZ6su2PsBcfORtKoBjdmGchSTjaZ5AyIUXH23Tm2HcSnF61x ntYPTe5qZ4m/nexj8igdZiMwzDnmCQyvzlU4NSTfFxCNc22tRQsVhuTrf SMbPp5NK+R7DMS7Pizhur77cFFtjduG3s5bAG+0KhEw4kge/n7srzHcNg nrKRBBfZx+dNQlTW/0llFdktsZyebFFsgNrdN0OyvZOya/ztIxcFL/9uI e518CSU4bsaCvxn5FZRQD2dvFuO5vIGbDHguSw+6idZycoK3yLuJuHNjC A==; X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="308260625" X-IronPort-AV: E=Sophos;i="5.96,147,1665471600"; d="scan'208";a="308260625" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 00:28:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="630798909" X-IronPort-AV: E=Sophos;i="5.96,147,1665471600"; d="scan'208";a="630798909" Received: from chaop.bj.intel.com (HELO localhost) ([10.240.193.75]) by orsmga007.jf.intel.com with ESMTP; 08 Nov 2022 00:28:36 -0800 Date: Tue, 8 Nov 2022 16:24:10 +0800 From: Chao Peng To: Sean Christopherson Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org, linux-doc@vger.kernel.org, qemu-devel@nongnu.org, Paolo Bonzini , Jonathan Corbet , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , Thomas Gleixner , Ingo Molnar , Borislav Petkov , x86@kernel.org, "H . Peter Anvin" , Hugh Dickins , Jeff Layton , "J . Bruce Fields" , Andrew Morton , Shuah Khan , Mike Rapoport , Steven Price , "Maciej S . Szmigiero" , Vlastimil Babka , Vishal Annapurve , Yu Zhang , "Kirill A . Shutemov" , luto@kernel.org, jun.nakajima@intel.com, dave.hansen@intel.com, ak@linux.intel.com, david@redhat.com, aarcange@redhat.com, ddutile@redhat.com, dhildenb@redhat.com, Quentin Perret , tabba@google.com, Michael Roth , mhocko@suse.com, Muchun Song , wei.w.wang@intel.com Subject: Re: [PATCH v9 5/8] KVM: Register/unregister the guest private memory regions Message-ID: <20221108082410.GA84375@chaop.bj.intel.com> Reply-To: Chao Peng References: <20221025151344.3784230-1-chao.p.peng@linux.intel.com> <20221025151344.3784230-6-chao.p.peng@linux.intel.com> <20221104082843.GA4142342@chaop.bj.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1667896130; a=rsa-sha256; cv=none; b=nW4fHwhhC5zTNr4VOnaPNbU0rDLqce5eNWtdJ6aAtVUb5i3EROaTkf4cVpLCguTyNHIyXd e63GQHF9tW/LtO2+LdTGiLnc4D+kTmTGYoqZJUzWpHTtfY0Hi7I2Ee1FwBusT7DgBAhRYK yii2VWQgkhSCluF2Af8E2I6yKlbyiHw= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=PednyWZw; spf=none (imf28.hostedemail.com: domain of chao.p.peng@linux.intel.com has no SPF policy when checking 192.55.52.93) smtp.mailfrom=chao.p.peng@linux.intel.com; dmarc=fail reason="No valid SPF" header.from=intel.com (policy=none) ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1667896130; h=from:from:sender:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=55h2JgkWltKfzXIereN3kkaFUM+D7TNNjzuE7HgUgRs=; b=F6FhVPhs1RI/qDQIfAqCc6VAhM0S06Bz2sg5FmJscK8pfJp2rf9bVJJgwpe2QQVa0i4W/B x/gGy/kz60VxZvXCS85iERMU74sT9Kq0tFVQumyzOkDqX84HMXSR8xCBD5McKINoyEGL1v /hDdDn+Q/k9xOMtbiLYWHvTswoLnnHg= X-Stat-Signature: jp7w5bpmbco1iou7honw4k7yo9kjexzc X-Rspam-User: Authentication-Results: imf28.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=PednyWZw; spf=none (imf28.hostedemail.com: domain of chao.p.peng@linux.intel.com has no SPF policy when checking 192.55.52.93) smtp.mailfrom=chao.p.peng@linux.intel.com; dmarc=fail reason="No valid SPF" header.from=intel.com (policy=none) X-Rspamd-Queue-Id: 02E67C0009 X-Rspamd-Server: rspam09 X-HE-Tag: 1667896129-4825 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Nov 04, 2022 at 09:19:31PM +0000, Sean Christopherson wrote: > Paolo, any thoughts before I lead things further astray? > > On Fri, Nov 04, 2022, Chao Peng wrote: > > On Thu, Nov 03, 2022 at 11:04:53PM +0000, Sean Christopherson wrote: > > > On Tue, Oct 25, 2022, Chao Peng wrote: > > > > @@ -4708,6 +4802,24 @@ static long kvm_vm_ioctl(struct file *filp, > > > > r = kvm_vm_ioctl_set_memory_region(kvm, &mem); > > > > break; > > > > } > > > > +#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM > > > > + case KVM_MEMORY_ENCRYPT_REG_REGION: > > > > + case KVM_MEMORY_ENCRYPT_UNREG_REGION: { > > > > > > I'm having second thoughts about usurping KVM_MEMORY_ENCRYPT_(UN)REG_REGION. Aside > > > from the fact that restricted/protected memory may not be encrypted, there are > > > other potential use cases for per-page memory attributes[*], e.g. to make memory > > > read-only (or no-exec, or exec-only, etc...) without having to modify memslots. > > > > > > Any paravirt use case where the attributes of a page are effectively dictated by > > > the guest is going to run into the exact same performance problems with memslots, > > > which isn't suprising in hindsight since shared vs. private is really just an > > > attribute, albeit with extra special semantics. > > > > > > And if we go with a brand new ioctl(), maybe someday in the very distant future > > > we can deprecate and delete KVM_MEMORY_ENCRYPT_(UN)REG_REGION. > > > > > > Switching to a new ioctl() should be a minor change, i.e. shouldn't throw too big > > > of a wrench into things. > > > > > > Something like: > > > > > > KVM_SET_MEMORY_ATTRIBUTES > > > > > > struct kvm_memory_attributes { > > > __u64 address; > > > __u64 size; > > > __u64 flags; > > Oh, this is half-baked. I lost track of which flags were which. What I intended > was a separate, initially-unused flags, e.g. That makes sense. > > struct kvm_memory_attributes { > __u64 address; > __u64 size; > __u64 attributes; > __u64 flags; > } > > so that KVM can tweak behavior and/or extend the effective size of the struct. > > > I like the idea of adding a new ioctl(). But putting all attributes into > > a flags in uAPI sounds not good to me, e.g. forcing userspace to set all > > attributes in one call can cause pain for userspace, probably for KVM > > implementation as well. For private<->shared memory conversion, we > > actually only care the KVM_MEM_ATTR_SHARED or KVM_MEM_ATTR_PRIVATE bit, > > Not necessarily, e.g. I can see pKVM wanting to convert from RW+PRIVATE => RO+SHARED > or even RW+PRIVATE => NONE+SHARED so that the guest can't write/access the memory > while it's accessible from the host. > > And if this does extend beyond shared/private, dropping from RWX=>R, i.e. dropping > WX permissions, would also be a common operation. > > Hmm, typing that out makes me think that if we do end up supporting other "attributes", > i.e. protections, we should go straight to full RWX protections instead of doing > things piecemeal, i.e. add individual protections instead of combinations like > NO_EXEC and READ_ONLY. The protections would have to be inverted for backwards > compatibility, but that's easy enough to handle. The semantics could be like > protection keys, which also have inverted persmissions, where the final protections > are the combination of memslot+attributes, i.e. a read-only memslot couldn't be made > writable via attributes. > > E.g. userspace could do "NO_READ | NO_WRITE | NO_EXEC" to temporarily block access > to memory without needing to delete the memslot. KVM would need to disallow > unsupported combinations, e.g. disallowed effective protections would be: > > - W or WX [unless there's an arch that supports write-only memory] > - R or RW [until KVM plumbs through support for no-exec, or it's unsupported in hardware] > - X [until KVM plumbs through support for exec-only, or it's unsupported in hardware] > > Anyways, that's all future work... > > > but we force userspace to set other irrelevant bits as well if use this > > API. > > They aren't irrelevant though, as the memory attributes are all describing the > allowed protections for a given page. The 'allowed' protections seems answer my concern. But after we enabled "NO_READ | NO_WRITE | NO_EXEC", are we going to check "NO_READ | NO_WRITE | NO_EXEC" are also set together with the PRIVATE bit? I just can't imagine what the semantic would be if we have the PRIVATE bit set but other bits indicate it's actually can READ/WRITE/EXEC from usrspace. > If there's a use case where userspace "can't" > keep track of the attributes for whatever reason, then userspace could do a RMW > to set/clear attributes. Alternatively, the ioctl() could take an "operation" and > support WRITE/OR/AND to allow setting/clearing individual flags, e.g. tweak the > above to be: A getter would be good, it might also be needed for live migration. > > struct kvm_memory_attributes { > __u64 address; > __u64 size; > __u64 attributes; > __u32 operation; > __u32 flags; > } > > > I looked at kvm_device_attr, sounds we can do similar: > > The device attributes deal with isolated, arbitrary values, whereas memory attributes > are flags, i.e. devices are 1:1 whereas memory is 1:MANY. There is no "unset" for > device attributes, because they aren't flags. Device attributes vs. memory attributes > really are two very different things that just happen to use a common name. > > If it helped clarify things without creating naming problems, we could even use > PROTECTIONS instead of ATTRIBUTES. > > > KVM_SET_MEMORY_ATTR > > > > struct kvm_memory_attr { > > __u64 address; > > __u64 size; > > #define KVM_MEM_ATTR_SHARED BIT(0) > > #define KVM_MEM_ATTR_READONLY BIT(1) > > #define KVM_MEM_ATTR_NOEXEC BIT(2) > > __u32 attr; > > As above, letting userspace set only a single attribute would prevent setting > (or clearing) multiple attributes in a single ioctl(). > > > __u32 pad; > > } > > > > I'm not sure if we need KVM_GET_MEMORY_ATTR/KVM_HAS_MEMORY_ATTR as well, > > Definitely would need to communicate to userspace that various attributes are > supported. That doesn't necessarily require a common ioctl(), but I don't see > any reason not to add a common helper, and adding a common helper would mean > KVM_CAP_PRIVATE_MEM can go away. But it should return a bitmask so that userspace > can do a single query to get all supported attributes, e.g. KVM_SUPPORTED_MEMORY_ATTRIBUTES. Do you have preference on using a new ioctl or just keep it as a cap? E.g. KVM_CAP_MEMORY_ATTIBUTES can also returns a mask. > > As for KVM_GET_MEMORY_ATTRIBUTES, we wouldn't necessarily have to provide such an > API, e.g. we could hold off until someone came along with a RMW use case (as above). > That said, debug would likely be a nightmare without KVM_GET_MEMORY_ATTRIBUTES, > so it's probably best to add it straightway. Dive into the implementation a bit, for KVM_GET_MEMORY_ATTRIBUTES we can have different attributes for different pages in the same user-provided range, in that case we will have to either return a list or just a error number. Or we only support per-page attributes for the getter? Chao > > > but sounds like we need a KVM_UNSET_MEMORY_ATTR. > > No need if the setter operates on all attributes. > > > Since we are exposing the attribute directly to userspace I also think > > we'd better treat shared memory as the default, so even when the private > > memory is not used, the bit can still be meaningful. So define BIT(0) as > > KVM_MEM_ATTR_PRIVATE instead of KVM_MEM_ATTR_SHARED. > > Ah, right.