From mboxrd@z Thu Jan 1 00:00:00 1970 From: Quentin Perret Date: Mon, 31 Jul 2023 09:30:43 +0000 Subject: [RFC PATCH v11 06/29] KVM: Introduce KVM_SET_USER_MEMORY_REGION2 In-Reply-To: References: <20230718234512.1690985-1-seanjc@google.com> <20230718234512.1690985-7-seanjc@google.com> Message-ID: List-Id: To: kvm-riscv@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Friday 28 Jul 2023 at 17:03:33 (-0700), Sean Christopherson wrote: > On Fri, Jul 28, 2023, Quentin Perret wrote: > > On Tuesday 18 Jul 2023 at 16:44:49 (-0700), Sean Christopherson wrote: > > > --- a/include/uapi/linux/kvm.h > > > +++ b/include/uapi/linux/kvm.h > > > @@ -95,6 +95,16 @@ struct kvm_userspace_memory_region { > > > __u64 userspace_addr; /* start of the userspace allocated memory */ > > > }; > > > > > > +/* for KVM_SET_USER_MEMORY_REGION2 */ > > > +struct kvm_userspace_memory_region2 { > > > + __u32 slot; > > > + __u32 flags; > > > + __u64 guest_phys_addr; > > > + __u64 memory_size; > > > + __u64 userspace_addr; > > > + __u64 pad[16]; > > > > Should we replace that pad[16] with: > > > > __u64 size; > > > > where 'size' is the size of the structure as seen by userspace? This is > > used in other UAPIs (see struct sched_attr for example) and is a bit > > more robust for future extensions (e.g. an 'old' kernel can correctly > > reject a newer version of the struct with additional fields it doesn't > > know about if that makes sense, etc). > > "flags" serves that purpose, i.e. allows userspace to opt-in to having KVM actually > consume what is currently just padding. Sure, I've just grown to dislike static padding of that type -- it ends up being either a waste a space, or is too small, while the 'superior' alternative (having a 'size' member) doesn't cost much and avoids those problems. But no strong opinion really, this struct really shouldn't grow much, so I'm sure that'll be fine in practice. > The padding is there mainly to simplify kernel/KVM code, e.g. the number of bytes > that KVM needs to copy in is static. > > But now that I think more on this, I don't know why we didn't just unconditionally > bump the size of kvm_userspace_memory_region. We tried to play games with unions > and overlays, but that was a mess[*]. > > KVM would need to do multiple uaccess reads, but that's not a big deal. Am I > missing something, or did past us just get too clever and miss the obvious solution? > > [*] https://lkml.kernel.org/r/Y7xrtf9FCuYRYm1q%40google.com Right, so the first uaccess would get_user() the flags, based on that we'd figure out the size of the struct, copy_from_user() what we need, and then sanity check the flags are the same from both reads, or something along those lines? That doesn't sound too complicated to me, and as long as every extension to the struct does come with a new flag I can't immediately see what would go wrong. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f48.google.com (mail-ed1-f48.google.com [209.85.208.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4226B63A2 for ; Mon, 31 Jul 2023 09:30:48 +0000 (UTC) Received: by mail-ed1-f48.google.com with SMTP id 4fb4d7f45d1cf-522bc9556f5so2216721a12.0 for ; Mon, 31 Jul 2023 02:30:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=guIUa9TpRYBjI0stX0rR0q2KYRVdgo42Rwix6s3W6cxhGOSx6PVyCTdA5g/AS2h33d qccXGdTTxsaL3dKPpzdUGCwz9sOnNjZP2QZ+IRlflzPGUckJBW/eFq4DipVJqbUG5NQ/ 5teoOLbLNLvidR0nDX0dInMVzU60IzUEZpiCJ78DCgLwH4VDVpyvS+PFDJmp2H2Cqsaq ckd8/Aqk96/l2tM08ubMYTGsra64KF4plRKJW4CV/d6dRMYvZWy3I0q2vTBh3JUMjdY2 e0xEUNA9wJufu3+8SkFanLOhloqOVlBS3eZCjfmk+1nbmTk3VOjLRNz4/M5kaTwNZDzq GaNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=GawylovnRa/36+FWKr1aiVUFjapGdPtVwVZbwi8UgC6Y0fYOX08SmrRGVjZvGK/FYc kXyaxSS3/tE44QnTD/TPqPDCLS2MguoNI05D00XxlFggfNofhyWW3GAbNE+EDCYONlQD 2bpXmAVqC5aFlSPsce5NpjiDwtLr2GZmi2Yv6MstUPSqhBAFXEKsLkax0xGoZ8Th6OWM TFq2p6A/b0AwkhKQ8BKpUJI1ykXhYg5gcxwBjIdqmid7wDOhX2qCkqaJniWpA56dM0Va BgAGAoJDRP/Rflh1dLoqP2MfpuqxfWe0t8AnB2+lfJCq0B85A779fqn/dIx7kcs4+l+k 7Npw== X-Gm-Message-State: ABy/qLZi4aovDK/mTw6J10ugURPjvB6SyJyrjOvKvWl6yiyyWe0JKV1N A0aGTRJAsfVDXezLiK0rvz4/OQ== X-Google-Smtp-Source: APBJJlH9uJ20HU/xu6w4BzlZ8VKOTZ9pA8N8iNM1wVNpNPl9nuSdUi/k4VZmc0YpyQk9a9eYCl1pHA== X-Received: by 2002:aa7:c554:0:b0:522:40dd:74f3 with SMTP id s20-20020aa7c554000000b0052240dd74f3mr9248786edr.39.1690795847077; Mon, 31 Jul 2023 02:30:47 -0700 (PDT) Received: from google.com (64.227.90.34.bc.googleusercontent.com. [34.90.227.64]) by smtp.gmail.com with ESMTPSA id q20-20020aa7da94000000b005228c045515sm5165439eds.14.2023.07.31.02.30.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 31 Jul 2023 02:30:46 -0700 (PDT) Date: Mon, 31 Jul 2023 09:30:43 +0000 From: Quentin Perret To: Sean Christopherson Cc: Paolo Bonzini , Marc Zyngier , Oliver Upton , Huacai Chen , Michael Ellerman , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , "Matthew Wilcox (Oracle)" , Andrew Morton , Paul Moore , James Morris , "Serge E. Hallyn" , kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Chao Peng , Fuad Tabba , Jarkko Sakkinen , Yu Zhang , Vishal Annapurve , Ackerley Tng , Maciej Szmigiero , Vlastimil Babka , David Hildenbrand , Michael Roth , Wang , Liam Merwick , Isaku Yamahata , "Kirill A . Shutemov" Subject: Re: [RFC PATCH v11 06/29] KVM: Introduce KVM_SET_USER_MEMORY_REGION2 Message-ID: References: <20230718234512.1690985-1-seanjc@google.com> <20230718234512.1690985-7-seanjc@google.com> Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Friday 28 Jul 2023 at 17:03:33 (-0700), Sean Christopherson wrote: > On Fri, Jul 28, 2023, Quentin Perret wrote: > > On Tuesday 18 Jul 2023 at 16:44:49 (-0700), Sean Christopherson wrote: > > > --- a/include/uapi/linux/kvm.h > > > +++ b/include/uapi/linux/kvm.h > > > @@ -95,6 +95,16 @@ struct kvm_userspace_memory_region { > > > __u64 userspace_addr; /* start of the userspace allocated memory */ > > > }; > > > > > > +/* for KVM_SET_USER_MEMORY_REGION2 */ > > > +struct kvm_userspace_memory_region2 { > > > + __u32 slot; > > > + __u32 flags; > > > + __u64 guest_phys_addr; > > > + __u64 memory_size; > > > + __u64 userspace_addr; > > > + __u64 pad[16]; > > > > Should we replace that pad[16] with: > > > > __u64 size; > > > > where 'size' is the size of the structure as seen by userspace? This is > > used in other UAPIs (see struct sched_attr for example) and is a bit > > more robust for future extensions (e.g. an 'old' kernel can correctly > > reject a newer version of the struct with additional fields it doesn't > > know about if that makes sense, etc). > > "flags" serves that purpose, i.e. allows userspace to opt-in to having KVM actually > consume what is currently just padding. Sure, I've just grown to dislike static padding of that type -- it ends up being either a waste a space, or is too small, while the 'superior' alternative (having a 'size' member) doesn't cost much and avoids those problems. But no strong opinion really, this struct really shouldn't grow much, so I'm sure that'll be fine in practice. > The padding is there mainly to simplify kernel/KVM code, e.g. the number of bytes > that KVM needs to copy in is static. > > But now that I think more on this, I don't know why we didn't just unconditionally > bump the size of kvm_userspace_memory_region. We tried to play games with unions > and overlays, but that was a mess[*]. > > KVM would need to do multiple uaccess reads, but that's not a big deal. Am I > missing something, or did past us just get too clever and miss the obvious solution? > > [*] https://lkml.kernel.org/r/Y7xrtf9FCuYRYm1q%40google.com Right, so the first uaccess would get_user() the flags, based on that we'd figure out the size of the struct, copy_from_user() what we need, and then sanity check the flags are the same from both reads, or something along those lines? That doesn't sound too complicated to me, and as long as every extension to the struct does come with a new flag I can't immediately see what would go wrong. 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 EBD6DC001DE for ; Mon, 31 Jul 2023 09:31:04 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=od2n6umrf92x/OuoNsTSkonIiLU3F8/sdCj1ocHDxG8=; b=2YPMIsnHKCnZyS SULBYWReEKOfUI2HrqphCOOYQIFioVq6Gx8IvKblRbWuAcAX4lBocJCVnGK+wQZ+tGcAKltNB404H oQIHZdkS86zB6HlbezWSjPPbwg6l7jZfBMmX77CUADm5sP5mvqdNS36q3By/25XUHPsC+hjLbld4D lklvXHVMH964ztZw00XnTIeAiyjfB3GpeQTMQaJeIXP3b9++zqploUKGPwOoW+yn40qZm3PhTwvzU rureOzT5v3kRfGNFJY+cVaNlBRexoB4+skbX4mpjI7hDN8Pes2n52cniHdm3v0kVPjDHkK1c5VU/S t3YAC/aefAm9g8yYbduQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qQPEv-00EmQS-1Y; Mon, 31 Jul 2023 09:30:57 +0000 Received: from mail-ed1-x531.google.com ([2a00:1450:4864:20::531]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qQPEs-00EmOe-04 for linux-riscv@lists.infradead.org; Mon, 31 Jul 2023 09:30:55 +0000 Received: by mail-ed1-x531.google.com with SMTP id 4fb4d7f45d1cf-52229f084beso6789690a12.2 for ; Mon, 31 Jul 2023 02:30:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=guIUa9TpRYBjI0stX0rR0q2KYRVdgo42Rwix6s3W6cxhGOSx6PVyCTdA5g/AS2h33d qccXGdTTxsaL3dKPpzdUGCwz9sOnNjZP2QZ+IRlflzPGUckJBW/eFq4DipVJqbUG5NQ/ 5teoOLbLNLvidR0nDX0dInMVzU60IzUEZpiCJ78DCgLwH4VDVpyvS+PFDJmp2H2Cqsaq ckd8/Aqk96/l2tM08ubMYTGsra64KF4plRKJW4CV/d6dRMYvZWy3I0q2vTBh3JUMjdY2 e0xEUNA9wJufu3+8SkFanLOhloqOVlBS3eZCjfmk+1nbmTk3VOjLRNz4/M5kaTwNZDzq GaNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=N6o2uW1ZQg/ztrcQ1I6cxFey0RA1vSXj1vsaErnO9HnJi+mlUg2ytF5xuuMi4z5D7C H9k5MwIL3Jju1xRIatyD3k8for0x/GOa5b2ToirVjvsjystFHRiP9PQQDZDMXWRlsggA DdPvGxJWHQogfWzJZC+iDhWte661wOw29ITUXB2Tz3dRe/ifue4pJnt1o3Yc+PctfJ/o 1LIay7PdO9r+QyYy+5LXfPmv2WKHfp8pV0R+ljutzw0GQfcKn1a2PEhGDOyr2q6BXJ2p KuGUdyYKJpswlRZFkvAfjU1H/jOXU76M9B+FahIKYoUA3+Hm66gsqpitwuYqiZB2hBuz ZGfQ== X-Gm-Message-State: ABy/qLalejR3E9UqKqq3IdCQ/5ZiePe/BDOBwXQvCP7tbGu4nVBvrsVM cfOxW93mmmOixcEjfRtsHdMGZQ== X-Google-Smtp-Source: APBJJlH9uJ20HU/xu6w4BzlZ8VKOTZ9pA8N8iNM1wVNpNPl9nuSdUi/k4VZmc0YpyQk9a9eYCl1pHA== X-Received: by 2002:aa7:c554:0:b0:522:40dd:74f3 with SMTP id s20-20020aa7c554000000b0052240dd74f3mr9248786edr.39.1690795847077; Mon, 31 Jul 2023 02:30:47 -0700 (PDT) Received: from google.com (64.227.90.34.bc.googleusercontent.com. [34.90.227.64]) by smtp.gmail.com with ESMTPSA id q20-20020aa7da94000000b005228c045515sm5165439eds.14.2023.07.31.02.30.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 31 Jul 2023 02:30:46 -0700 (PDT) Date: Mon, 31 Jul 2023 09:30:43 +0000 From: Quentin Perret To: Sean Christopherson Cc: Paolo Bonzini , Marc Zyngier , Oliver Upton , Huacai Chen , Michael Ellerman , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , "Matthew Wilcox (Oracle)" , Andrew Morton , Paul Moore , James Morris , "Serge E. Hallyn" , kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Chao Peng , Fuad Tabba , Jarkko Sakkinen , Yu Zhang , Vishal Annapurve , Ackerley Tng , Maciej Szmigiero , Vlastimil Babka , David Hildenbrand , Michael Roth , Wang , Liam Merwick , Isaku Yamahata , "Kirill A . Shutemov" Subject: Re: [RFC PATCH v11 06/29] KVM: Introduce KVM_SET_USER_MEMORY_REGION2 Message-ID: References: <20230718234512.1690985-1-seanjc@google.com> <20230718234512.1690985-7-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230731_023054_058636_9239F4FF X-CRM114-Status: GOOD ( 26.40 ) X-BeenThere: linux-riscv@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-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Friday 28 Jul 2023 at 17:03:33 (-0700), Sean Christopherson wrote: > On Fri, Jul 28, 2023, Quentin Perret wrote: > > On Tuesday 18 Jul 2023 at 16:44:49 (-0700), Sean Christopherson wrote: > > > --- a/include/uapi/linux/kvm.h > > > +++ b/include/uapi/linux/kvm.h > > > @@ -95,6 +95,16 @@ struct kvm_userspace_memory_region { > > > __u64 userspace_addr; /* start of the userspace allocated memory */ > > > }; > > > > > > +/* for KVM_SET_USER_MEMORY_REGION2 */ > > > +struct kvm_userspace_memory_region2 { > > > + __u32 slot; > > > + __u32 flags; > > > + __u64 guest_phys_addr; > > > + __u64 memory_size; > > > + __u64 userspace_addr; > > > + __u64 pad[16]; > > > > Should we replace that pad[16] with: > > > > __u64 size; > > > > where 'size' is the size of the structure as seen by userspace? This is > > used in other UAPIs (see struct sched_attr for example) and is a bit > > more robust for future extensions (e.g. an 'old' kernel can correctly > > reject a newer version of the struct with additional fields it doesn't > > know about if that makes sense, etc). > > "flags" serves that purpose, i.e. allows userspace to opt-in to having KVM actually > consume what is currently just padding. Sure, I've just grown to dislike static padding of that type -- it ends up being either a waste a space, or is too small, while the 'superior' alternative (having a 'size' member) doesn't cost much and avoids those problems. But no strong opinion really, this struct really shouldn't grow much, so I'm sure that'll be fine in practice. > The padding is there mainly to simplify kernel/KVM code, e.g. the number of bytes > that KVM needs to copy in is static. > > But now that I think more on this, I don't know why we didn't just unconditionally > bump the size of kvm_userspace_memory_region. We tried to play games with unions > and overlays, but that was a mess[*]. > > KVM would need to do multiple uaccess reads, but that's not a big deal. Am I > missing something, or did past us just get too clever and miss the obvious solution? > > [*] https://lkml.kernel.org/r/Y7xrtf9FCuYRYm1q%40google.com Right, so the first uaccess would get_user() the flags, based on that we'd figure out the size of the struct, copy_from_user() what we need, and then sanity check the flags are the same from both reads, or something along those lines? That doesn't sound too complicated to me, and as long as every extension to the struct does come with a new flag I can't immediately see what would go wrong. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv 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 lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (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 41975C001DE for ; Mon, 31 Jul 2023 09:31:52 +0000 (UTC) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20221208 header.b=guIUa9Tp; dkim-atps=neutral Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4RDtKy5ygnz3bt0 for ; Mon, 31 Jul 2023 19:31:50 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20221208 header.b=guIUa9Tp; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=google.com (client-ip=2a00:1450:4864:20::52f; helo=mail-ed1-x52f.google.com; envelope-from=qperret@google.com; receiver=lists.ozlabs.org) Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4RDtJt5SQvz2ykW for ; Mon, 31 Jul 2023 19:30:53 +1000 (AEST) Received: by mail-ed1-x52f.google.com with SMTP id 4fb4d7f45d1cf-52227884855so6799870a12.1 for ; Mon, 31 Jul 2023 02:30:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=guIUa9TpRYBjI0stX0rR0q2KYRVdgo42Rwix6s3W6cxhGOSx6PVyCTdA5g/AS2h33d qccXGdTTxsaL3dKPpzdUGCwz9sOnNjZP2QZ+IRlflzPGUckJBW/eFq4DipVJqbUG5NQ/ 5teoOLbLNLvidR0nDX0dInMVzU60IzUEZpiCJ78DCgLwH4VDVpyvS+PFDJmp2H2Cqsaq ckd8/Aqk96/l2tM08ubMYTGsra64KF4plRKJW4CV/d6dRMYvZWy3I0q2vTBh3JUMjdY2 e0xEUNA9wJufu3+8SkFanLOhloqOVlBS3eZCjfmk+1nbmTk3VOjLRNz4/M5kaTwNZDzq GaNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=igtZ7ImUF04fhlFbJtjyMWb7ofEch7hFuZScPLHa7Y3p/gXHyoTYU1twlvDY4Kibtm ta3Mo0iNbaB2I4uTlgB4HLbYkoWY8a8GKvaIp9oR395pnq21Xv1KDXJXWILo62l3p73E p86JJgfETenS7lsropaXIG+pGff6CeuVgCAVAY2QVSW9JXdG5AwTcJMNYQXnbf0l2Uvd Ogx4gzRc7pqIzSpXmWsCiRgjKUbXidneZG2poRioDU6JKQn/FW/OVFW3MNbDzEp+BVwa lcH8vKYTEQSUAt09u+lyADtjZRTheYOwDUV96ceHp6AKcUHe+hMPno5DFEJpBeFrEdDh 8V4Q== X-Gm-Message-State: ABy/qLYVu9HxstqWHiVuz8q4WstFZqETZ0Vc5rMnNmsVqGK9rpEw84i1 //1OhDcR02+ii6H3999DMpmfrA== X-Google-Smtp-Source: APBJJlH9uJ20HU/xu6w4BzlZ8VKOTZ9pA8N8iNM1wVNpNPl9nuSdUi/k4VZmc0YpyQk9a9eYCl1pHA== X-Received: by 2002:aa7:c554:0:b0:522:40dd:74f3 with SMTP id s20-20020aa7c554000000b0052240dd74f3mr9248786edr.39.1690795847077; Mon, 31 Jul 2023 02:30:47 -0700 (PDT) Received: from google.com (64.227.90.34.bc.googleusercontent.com. [34.90.227.64]) by smtp.gmail.com with ESMTPSA id q20-20020aa7da94000000b005228c045515sm5165439eds.14.2023.07.31.02.30.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 31 Jul 2023 02:30:46 -0700 (PDT) Date: Mon, 31 Jul 2023 09:30:43 +0000 From: Quentin Perret To: Sean Christopherson Subject: Re: [RFC PATCH v11 06/29] KVM: Introduce KVM_SET_USER_MEMORY_REGION2 Message-ID: References: <20230718234512.1690985-1-seanjc@google.com> <20230718234512.1690985-7-seanjc@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kvm@vger.kernel.org, David Hildenbrand , Yu Zhang , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Chao Peng , linux-riscv@lists.infradead.org, Isaku Yamahata , Paul Moore , Marc Zyngier , Huacai Chen , James Morris , "Matthew Wilcox \(Oracle\)" , Wang , Fuad Tabba , Jarkko Sakkinen , "Serge E. Hallyn" , Maciej Szmigiero , Albert Ou , Vlastimil Babka , Michael Roth , Ackerley Tng , Paul Walmsley , kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, Liam Merwick , linux-mips@vger.kernel.org, Oliver Upton , linu x-security-module@vger.kernel.org, Palmer Dabbelt , kvm-riscv@lists.infradead.org, Anup Patel , linux-fsdevel@vger.kernel.org, Paolo Bonzini , Andrew Morton , Vishal Annapurve , linuxppc-dev@lists.ozlabs.org, "Kirill A . Shutemov" Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Friday 28 Jul 2023 at 17:03:33 (-0700), Sean Christopherson wrote: > On Fri, Jul 28, 2023, Quentin Perret wrote: > > On Tuesday 18 Jul 2023 at 16:44:49 (-0700), Sean Christopherson wrote: > > > --- a/include/uapi/linux/kvm.h > > > +++ b/include/uapi/linux/kvm.h > > > @@ -95,6 +95,16 @@ struct kvm_userspace_memory_region { > > > __u64 userspace_addr; /* start of the userspace allocated memory */ > > > }; > > > > > > +/* for KVM_SET_USER_MEMORY_REGION2 */ > > > +struct kvm_userspace_memory_region2 { > > > + __u32 slot; > > > + __u32 flags; > > > + __u64 guest_phys_addr; > > > + __u64 memory_size; > > > + __u64 userspace_addr; > > > + __u64 pad[16]; > > > > Should we replace that pad[16] with: > > > > __u64 size; > > > > where 'size' is the size of the structure as seen by userspace? This is > > used in other UAPIs (see struct sched_attr for example) and is a bit > > more robust for future extensions (e.g. an 'old' kernel can correctly > > reject a newer version of the struct with additional fields it doesn't > > know about if that makes sense, etc). > > "flags" serves that purpose, i.e. allows userspace to opt-in to having KVM actually > consume what is currently just padding. Sure, I've just grown to dislike static padding of that type -- it ends up being either a waste a space, or is too small, while the 'superior' alternative (having a 'size' member) doesn't cost much and avoids those problems. But no strong opinion really, this struct really shouldn't grow much, so I'm sure that'll be fine in practice. > The padding is there mainly to simplify kernel/KVM code, e.g. the number of bytes > that KVM needs to copy in is static. > > But now that I think more on this, I don't know why we didn't just unconditionally > bump the size of kvm_userspace_memory_region. We tried to play games with unions > and overlays, but that was a mess[*]. > > KVM would need to do multiple uaccess reads, but that's not a big deal. Am I > missing something, or did past us just get too clever and miss the obvious solution? > > [*] https://lkml.kernel.org/r/Y7xrtf9FCuYRYm1q%40google.com Right, so the first uaccess would get_user() the flags, based on that we'd figure out the size of the struct, copy_from_user() what we need, and then sanity check the flags are the same from both reads, or something along those lines? That doesn't sound too complicated to me, and as long as every extension to the struct does come with a new flag I can't immediately see what would go wrong. 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 3D669C001DE for ; Mon, 31 Jul 2023 09:31:30 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=X0Pd/YJw1wBUK6D/VrnYeZc0vDHQZapPVIEHyqBkYms=; b=g+uwOLQdUpri7l ner4+OMOuTcIuQMaQdEESV7+jzq9q0B+9Xas5bBh1ODCA+sr1/RUgoxGbq4ZhH+FH7ObIgeZipkhU RXyhCARP/6uKM5a7TeGoqUZlGo68k3TJwpLrydQOrSs74GA5FIFGNC0CFhtaX3Pa7HP3PF+vkuKSA Ezofk0YX8wPFNqYyo3vTazsMxj0HjnvE3gVYE2ivOXW8VOZxhHQAnL+5WSOVRl7hseDuby/zUP8Ce PD+gC03o05nKBCBdpfuqZZBxLx/rRRgNXPvF8KMdD5fvJB+0hMwVczDKPcRTs5YgwlAOX6TXbQLuG CNTI2SGLsOhI4+ntN3HQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qQPEx-00EmQm-0T; Mon, 31 Jul 2023 09:30:59 +0000 Received: from mail-ed1-x52c.google.com ([2a00:1450:4864:20::52c]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qQPEu-00EmOb-0P for linux-arm-kernel@lists.infradead.org; Mon, 31 Jul 2023 09:30:57 +0000 Received: by mail-ed1-x52c.google.com with SMTP id 4fb4d7f45d1cf-52256241c50so6798199a12.3 for ; Mon, 31 Jul 2023 02:30:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=guIUa9TpRYBjI0stX0rR0q2KYRVdgo42Rwix6s3W6cxhGOSx6PVyCTdA5g/AS2h33d qccXGdTTxsaL3dKPpzdUGCwz9sOnNjZP2QZ+IRlflzPGUckJBW/eFq4DipVJqbUG5NQ/ 5teoOLbLNLvidR0nDX0dInMVzU60IzUEZpiCJ78DCgLwH4VDVpyvS+PFDJmp2H2Cqsaq ckd8/Aqk96/l2tM08ubMYTGsra64KF4plRKJW4CV/d6dRMYvZWy3I0q2vTBh3JUMjdY2 e0xEUNA9wJufu3+8SkFanLOhloqOVlBS3eZCjfmk+1nbmTk3VOjLRNz4/M5kaTwNZDzq GaNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690795847; x=1691400647; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ziEHdMiDKlKGEA4myzpOfHCTiCmmAiAT9VETtxXexS8=; b=BuyY1knUS/1xLMoqLgjvwlfNAhFDX4fjAbPrek+FdeOCD8EtnQidpli+sS87WXrBch 6sUQxPfAlrh1ewRH3JYmJAZPk37JWNNkaei96FZmHp5zyCB8V35k7sSP5sbmUDyxMkzq 0OkX/1l6J1yAqor1yBhN8Ps3WUv8fl4dq/LnLXj+Iaim4cp67EkkVO/XIhPIEkwv5Zwv ccUWBr4F9420kGCOQIc7lxNIuKK7DpSOIozjFMRpPY/7FEVfl3UPrCNDSrxyLLDJk5fX 1GgCNMUuvgKrbvpZvukqX2l9o1FcrwEbJ6yw3KXy72wK8eUkty7czYXoLTp5b63qbll0 bI0g== X-Gm-Message-State: ABy/qLZNXF8nrWaoFFxZvHx/Ra99xEe6MT+Vqo5olreBvFzYpcYO4ek2 NI/IrE3UqSMGt0i9ZcDr9x0zXQ== X-Google-Smtp-Source: APBJJlH9uJ20HU/xu6w4BzlZ8VKOTZ9pA8N8iNM1wVNpNPl9nuSdUi/k4VZmc0YpyQk9a9eYCl1pHA== X-Received: by 2002:aa7:c554:0:b0:522:40dd:74f3 with SMTP id s20-20020aa7c554000000b0052240dd74f3mr9248786edr.39.1690795847077; Mon, 31 Jul 2023 02:30:47 -0700 (PDT) Received: from google.com (64.227.90.34.bc.googleusercontent.com. [34.90.227.64]) by smtp.gmail.com with ESMTPSA id q20-20020aa7da94000000b005228c045515sm5165439eds.14.2023.07.31.02.30.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 31 Jul 2023 02:30:46 -0700 (PDT) Date: Mon, 31 Jul 2023 09:30:43 +0000 From: Quentin Perret To: Sean Christopherson Cc: Paolo Bonzini , Marc Zyngier , Oliver Upton , Huacai Chen , Michael Ellerman , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , "Matthew Wilcox (Oracle)" , Andrew Morton , Paul Moore , James Morris , "Serge E. Hallyn" , kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Chao Peng , Fuad Tabba , Jarkko Sakkinen , Yu Zhang , Vishal Annapurve , Ackerley Tng , Maciej Szmigiero , Vlastimil Babka , David Hildenbrand , Michael Roth , Wang , Liam Merwick , Isaku Yamahata , "Kirill A . Shutemov" Subject: Re: [RFC PATCH v11 06/29] KVM: Introduce KVM_SET_USER_MEMORY_REGION2 Message-ID: References: <20230718234512.1690985-1-seanjc@google.com> <20230718234512.1690985-7-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230731_023056_164481_FD6D6CD9 X-CRM114-Status: GOOD ( 27.71 ) 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 Friday 28 Jul 2023 at 17:03:33 (-0700), Sean Christopherson wrote: > On Fri, Jul 28, 2023, Quentin Perret wrote: > > On Tuesday 18 Jul 2023 at 16:44:49 (-0700), Sean Christopherson wrote: > > > --- a/include/uapi/linux/kvm.h > > > +++ b/include/uapi/linux/kvm.h > > > @@ -95,6 +95,16 @@ struct kvm_userspace_memory_region { > > > __u64 userspace_addr; /* start of the userspace allocated memory */ > > > }; > > > > > > +/* for KVM_SET_USER_MEMORY_REGION2 */ > > > +struct kvm_userspace_memory_region2 { > > > + __u32 slot; > > > + __u32 flags; > > > + __u64 guest_phys_addr; > > > + __u64 memory_size; > > > + __u64 userspace_addr; > > > + __u64 pad[16]; > > > > Should we replace that pad[16] with: > > > > __u64 size; > > > > where 'size' is the size of the structure as seen by userspace? This is > > used in other UAPIs (see struct sched_attr for example) and is a bit > > more robust for future extensions (e.g. an 'old' kernel can correctly > > reject a newer version of the struct with additional fields it doesn't > > know about if that makes sense, etc). > > "flags" serves that purpose, i.e. allows userspace to opt-in to having KVM actually > consume what is currently just padding. Sure, I've just grown to dislike static padding of that type -- it ends up being either a waste a space, or is too small, while the 'superior' alternative (having a 'size' member) doesn't cost much and avoids those problems. But no strong opinion really, this struct really shouldn't grow much, so I'm sure that'll be fine in practice. > The padding is there mainly to simplify kernel/KVM code, e.g. the number of bytes > that KVM needs to copy in is static. > > But now that I think more on this, I don't know why we didn't just unconditionally > bump the size of kvm_userspace_memory_region. We tried to play games with unions > and overlays, but that was a mess[*]. > > KVM would need to do multiple uaccess reads, but that's not a big deal. Am I > missing something, or did past us just get too clever and miss the obvious solution? > > [*] https://lkml.kernel.org/r/Y7xrtf9FCuYRYm1q%40google.com Right, so the first uaccess would get_user() the flags, based on that we'd figure out the size of the struct, copy_from_user() what we need, and then sanity check the flags are the same from both reads, or something along those lines? That doesn't sound too complicated to me, and as long as every extension to the struct does come with a new flag I can't immediately see what would go wrong. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel