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 921E2C25B50 for ; Fri, 20 Jan 2023 23:43:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 25FFE6B0071; Fri, 20 Jan 2023 18:43:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1E8BC6B0073; Fri, 20 Jan 2023 18:43:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 062076B007D; Fri, 20 Jan 2023 18:43:00 -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 E45CE6B0071 for ; Fri, 20 Jan 2023 18:42:59 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id C0E6E14070D for ; Fri, 20 Jan 2023 23:42:59 +0000 (UTC) X-FDA: 80376805278.05.8F608BD Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by imf26.hostedemail.com (Postfix) with ESMTP id D7F3E14000C for ; Fri, 20 Jan 2023 23:42:57 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Ylp+WtdD; spf=pass (imf26.hostedemail.com: domain of jarkko@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=jarkko@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1674258178; h=from:from:sender: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=i6ylB6Fs9zd+biUNLG7ORsU4aXXOU4WT8eY76yvtceg=; b=L1LkY9eI1mxAKVlae11vPF3vYZz7JpPKaSh2SiDbh/4KTWHFUHc83TIcdCGIP9DlJTVtwT INIZpdYHU3QMwmRm81jTaCVm5ttE2Hni05/fLVrPNdNYJ/nX6e37Jx8rJxgUST/Koz87Dm 4S8ucesbh9eiiIrpwS4viyCkgjznZHo= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Ylp+WtdD; spf=pass (imf26.hostedemail.com: domain of jarkko@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=jarkko@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1674258178; a=rsa-sha256; cv=none; b=8jLd4SBB+xsxNi3IM12L7VAgRrB5qhXcDBmjYv4mjjfz+SRFSmqi38O39Udt3E2icO33P8 GjJggXZBdoFGSfCgbHxm7Sjeztw5XPor5NJvO7ZOp1Ldx90ugsqBWR8+u1aWffKb9s0Zu/ uONiOhP6MAR8HLNTUfmuo6p8iM6qCd4= 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 213F7B82A99; Fri, 20 Jan 2023 23:42:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 448D0C433D2; Fri, 20 Jan 2023 23:42:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1674258174; bh=jOiDj1nTgDbA+vCo7Om5n16foMSR0Jzva+bUM8XvgWE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Ylp+WtdDSbUV9qYxCK/NhYNvqZKwU6kso6L95frHoTxvaQK4UPPibKrCB1i0HLzmT ZKIqndsDqyR7Dw/XOo9dxa8HTWnJ/IMYETqnqN6CEQ4FYE/FyaFb7s7zv41BIghyoY Gc4AxZYV0IB7R0SP6RJLkachyQPACiAAqsj6xLx5AselcF9UR4clPmRr9XG9IHJF4e WAUbkOeNUQI7NVt+z5Uy8+nR/33vH9oeYh0alW/N0ee1oLcGWG2gR1+tmrAL9f8FYG 4qvzPPqW4R0mcsJC5R4rRhegVnuNxZzsdVxJFdHLdmAe6PXbGdzyhNiZQFiD8k6X+L i90qVnaJiD1qg== Date: Fri, 20 Jan 2023 23:42:51 +0000 From: Jarkko Sakkinen To: Chao Peng Cc: Sean Christopherson , 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 , Arnd Bergmann , Naoya Horiguchi , Miaohe Lin , 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, wei.w.wang@intel.com Subject: Re: [PATCH v10 3/9] KVM: Extend the memslot to support fd-based private memory Message-ID: References: <20221202061347.1070246-1-chao.p.peng@linux.intel.com> <20221202061347.1070246-4-chao.p.peng@linux.intel.com> <20230106094000.GA2297836@chaop.bj.intel.com> <20230110091432.GA2441264@chaop.bj.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230110091432.GA2441264@chaop.bj.intel.com> X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: D7F3E14000C X-Stat-Signature: 8bc3zdk1bqxab4c4w1n36czdjawz7u4e X-Rspam-User: X-HE-Tag: 1674258177-140434 X-HE-Meta: U2FsdGVkX1+S53GWIImgDffY84jzhERWvhK48/F9FJ13G3z+SWNqDDbMdbmyQMLqdIs/xRhniIelk3zNvRFsGRrOWNl3nTtnrlREHdL2I4+Tirmi8LkEDQ/+iQKU2Q9n5skfJ19Yp7xvZmviOUPQ5ApG62OAWThArOISHCM9/9Oyf/E6DnoJsgs4y8n/eisSLZ9AqO2piPUEaHFJGL46386ZcUrvYN5nxFEZIhOgTRJhiQZ2Pqa9pq7AJvfzZAwPZgzPwP/wsiIRs9LV2+D/BPJV8I0UG3ei8g54xmLsT7zqIyBprjWIvRKLIi6LSUybAkmDXMuTnn/JT1ZIYqnWm2WbvoIrMxw1aRY/f7ahmmL6bgwiGgNDmoo5l9o01D572HXBZ4OVmyeyfBnCzF6Lf2RVWlAQAj1FtptM5QmyU5IpWBPIxnT581VOxWCjNVib2ZLSaDPzworwGDh/k77HbRZTNcBLjUf5Xi/FVX3ugBtTuK/wgFvN+d3lEWVCsyqFnty9fXd6pvSUYtG2qqWAoC9LZCr3e7nlQmAbsc66V1OC4hsu0ErCO1otSd6jhx13BtSAvB4bsTnUdAtxyS6u7ZlZKlAl6YuM0S7iIk+MQPZWgQBw4eUEHRfgYxQZ+jYbCXuR1d8r4Tbi8AA8IwQpHCTqHNkpAp4SEfaRT0E5Ovl74aXXEc2rOURFVgmDpOUMwRjMkQ5cf2xZndNBNntMFwiN+LYh5KvKr+0Nv3uAX5GJj8NeTkjUTsIiN8PjmK3lAF20mFQNjjlS2OFa5rgPjy02ObMpkPJ3qoQRJmeudufLud/yZp74h2+qHrJD1yfB+YIwrXDJvYvfAKdhH1dpe5/v0QYz9qgyDT07EXrs6iBunmg3OSZQtHbBZDbo4JhnaQuwGQZC5pmOQN5uF4lKDnTyM8tk/7FYAuzmLR1VGd8mGkVYXZGecARVh0d69YVF+iBIm8EttP6sWX/Dtrt FAPcDoN6 A5IAP9EMpuUDOOy0evCFjNrWSPy6mFyx9NpmsbKMsE9SQ10gUAp+2DpVweVx87kfyWl+M5gM3+LjAl79iA9n2k2ojkLlYOTyklK5tabJN8Qs1i9rQ5EGvJln1TyMZQFjPcujHG5QSiZIj6GjbpwOXT+55XzIZN5ozGxYXc/Kt6Q3m5rd2tTb/kciF47tNooyfjJvEVviMmdtdg0pd6YHFOma+rFBOfe19n9ROVj+wbIYUTiw37ycAh7NED1ilfkIlT/ppgd4SiiOxndhKUfOyOXF66himtIsTYSWatPC1IwoaHmHBUyGuIdv03E2CFyOyfcU8UEzpqc9HNm1SPk4qoQcOqC4W+bJHCA4d6KWRF8DJ74ittGuo3dj8ggut4M5DvNk0ZBx25hXDwQs= 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 Tue, Jan 10, 2023 at 05:14:32PM +0800, Chao Peng wrote: > On Mon, Jan 09, 2023 at 07:32:05PM +0000, Sean Christopherson wrote: > > On Fri, Jan 06, 2023, Chao Peng wrote: > > > On Thu, Jan 05, 2023 at 11:23:01AM +0000, Jarkko Sakkinen wrote: > > > > On Fri, Dec 02, 2022 at 02:13:41PM +0800, Chao Peng wrote: > > > > > To make future maintenance easy, internally use a binary compatible > > > > > alias struct kvm_user_mem_region to handle both the normal and the > > > > > '_ext' variants. > > > > > > > > Feels bit hacky IMHO, and more like a completely new feature than > > > > an extension. > > > > > > > > Why not just add a new ioctl? The commit message does not address > > > > the most essential design here. > > > > > > Yes, people can always choose to add a new ioctl for this kind of change > > > and the balance point here is we want to also avoid 'too many ioctls' if > > > the functionalities are similar. The '_ext' variant reuses all the > > > existing fields in the 'normal' variant and most importantly KVM > > > internally can reuse most of the code. I certainly can add some words in > > > the commit message to explain this design choice. > > > > After seeing the userspace side of this, I agree with Jarkko; overloading > > KVM_SET_USER_MEMORY_REGION is a hack. E.g. the size validation ends up being > > bogus, and userspace ends up abusing unions or implementing kvm_user_mem_region > > itself. > > How is the size validation being bogus? I don't quite follow. Then we > will use kvm_userspace_memory_region2 as the KVM internal alias, right? > I see similar examples use different functions to handle different > versions but it does look easier if we use alias for this function. > > > > > It feels absolutely ridiculous, but I think the best option is to do: > > > > #define KVM_SET_USER_MEMORY_REGION2 _IOW(KVMIO, 0x49, \ > > struct kvm_userspace_memory_region2) > > Just interesting, is 0x49 a safe number we can use? > > > > > /* for KVM_SET_USER_MEMORY_REGION2 */ > > struct kvm_user_mem_region2 { > > __u32 slot; > > __u32 flags; > > __u64 guest_phys_addr; > > __u64 memory_size; > > __u64 userspace_addr; > > __u64 restricted_offset; > > __u32 restricted_fd; > > __u32 pad1; > > __u64 pad2[14]; > > } > > > > And it's consistent with other KVM ioctls(), e.g. KVM_SET_CPUID2. > > Okay, agree from KVM userspace API perspective this is more consistent > with similar existing examples. I see several of them. > > I think we will also need a CAP_KVM_SET_USER_MEMORY_REGION2 for this new > ioctl. The current API in the patch set is trivial for C user space but for any other more "constrained" language such as Rust a new ioctl would be easier to adapt. > > > > Regarding the userspace side of things, please include Vishal's selftests in v11, > > it's impossible to properly review the uAPI changes without seeing the userspace > > side of things. I'm in the process of reviewing Vishal's v2[*], I'll try to > > massage it into a set of patches that you can incorporate into your series. > > Previously I included Vishal's selftests in the github repo, but not > include them in this patch series. It's OK for me to incorporate them > directly into this series and review together if Vishal is fine. > > Chao > > > > [*] https://lore.kernel.org/all/20221205232341.4131240-1-vannapurve@google.com BR, Jarkko