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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8AD63C001DF for ; Wed, 26 Jul 2023 16:00:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233149AbjGZQAI (ORCPT ); Wed, 26 Jul 2023 12:00:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58140 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234826AbjGZP76 (ORCPT ); Wed, 26 Jul 2023 11:59:58 -0400 Received: from mail-yb1-xb4a.google.com (mail-yb1-xb4a.google.com [IPv6:2607:f8b0:4864:20::b4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E805E19A4 for ; Wed, 26 Jul 2023 08:59:55 -0700 (PDT) Received: by mail-yb1-xb4a.google.com with SMTP id 3f1490d57ef6-c8f360a07a2so6265450276.2 for ; Wed, 26 Jul 2023 08:59:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690387195; x=1690991995; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=x1RCz9AGh0t/+COzjYLcha5fkSMq/GNrC4CuEfgGTfc=; b=ypSxifbz+3xr2TyQi3H8IoM4jyz7RGQWDnGcHi1NC0Po8RTEHh8icWGPEeM5z47awl loHCPZy4rtZMbUI9RqdI0QLfF+ad98/C7r2wxAa7ZpGCgkUVuuHTSwwJ1OUjNqHnXY8X 84BBat8mly4SBV0f6KqsOvHk8cSJA6wdZ1HsQOFr2IirR9RtsavVXkabaGLdXW2RSaSS UJmT6LqQAsULk7Nz99DvDaEAqSJ5Xa6WPtogY6EBxsD6vOblRb8mRKx+BIAVyZOlnkaE pEMJT5wcLP195pWDDcCypB4wv+dW/iWZkWl4YuLww/x6MLh6go/yMgsbLUu6B2dDA/yw w4bA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690387195; x=1690991995; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=x1RCz9AGh0t/+COzjYLcha5fkSMq/GNrC4CuEfgGTfc=; b=UUE1tHp7UXxYM+XJffbasroH+qd4sYsAY5pgQlElXcga3XwhGMnx/AgHQkCxVIIJ/I +ApFB7pxscDIbGRXV3NGgmuqtNT1ZgAPH1nS5eDeQw/IMMuN2Ojek8OBBfsnWQwVxn8F wO7FFfNL1nmgnTfGTTDV8uKtqb6krSIX+gJWofOG+5IQpqMWG7ARlFyhioHu5oVGlvwG sT6AWYKs24sUIZHe7/1Rcsu2bBA7WZH+WVmdjhxad5Y+YPUbqtstuFfeYpq9DN2bT/7u HLMM/MuMFcuK2FSDNUivpa6owmgpMG3Gapkd6aF3OdZUhZZhZS2nSYo6+WUiynsX5hqo +J/Q== X-Gm-Message-State: ABy/qLaf4UTwqjIll5z0+GvvprZa1ZisHC7rFNLjikGgsja40RXJfErS HAjXZw28XjyxvUC0b6rTEDu3w+6DSVg= X-Google-Smtp-Source: APBJJlE6mP/UcpH4lPNK9j4UUpMqMy7oSio4zbtJsZXhMStHU1Br/fIarHdKjyAaokfpTf45S20N2ryKLpo= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a25:99c8:0:b0:d1c:e102:95a5 with SMTP id q8-20020a2599c8000000b00d1ce10295a5mr15014ybo.7.1690387195164; Wed, 26 Jul 2023 08:59:55 -0700 (PDT) Date: Wed, 26 Jul 2023 08:59:53 -0700 In-Reply-To: Mime-Version: 1.0 References: <20230718234512.1690985-1-seanjc@google.com> <20230718234512.1690985-9-seanjc@google.com> Message-ID: Subject: Re: [RFC PATCH v11 08/29] KVM: Introduce per-page memory attributes From: Sean Christopherson To: Xu Yilun 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 , Quentin Perret , Michael Roth , Wang , Liam Merwick , Isaku Yamahata , "Kirill A . Shutemov" Content-Type: text/plain; charset="us-ascii" Precedence: bulk List-ID: On Mon, Jul 24, 2023, Xu Yilun wrote: > On 2023-07-18 at 16:44:51 -0700, Sean Christopherson wrote: > > @@ -1346,6 +1350,9 @@ static void kvm_destroy_vm(struct kvm *kvm) > > kvm_free_memslots(kvm, &kvm->__memslots[i][0]); > > kvm_free_memslots(kvm, &kvm->__memslots[i][1]); > > } > > +#ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES > > + xa_destroy(&kvm->mem_attr_array); > > +#endif > > Is it better to make the destruction in reverse order from the creation? Yeah. It _shoudn't_ matter, but there's no reason not keep things tidy and consistent. > To put xa_destroy(&kvm->mem_attr_array) after cleanup_srcu_struct(&kvm->srcu), > or put xa_init(&kvm->mem_attr_array) after init_srcu_struct(&kvm->irq_srcu). The former, because init_srcu_struct() can fail (allocates memory), whereas xa_init() is a "pure" initialization routine. > > cleanup_srcu_struct(&kvm->irq_srcu); > > cleanup_srcu_struct(&kvm->srcu); > > kvm_arch_free_vm(kvm); > > @@ -2346,6 +2353,145 @@ static int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm, > > } > > #endif /* CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT */ > > [...] > > > +static int kvm_vm_ioctl_set_mem_attributes(struct kvm *kvm, > > + struct kvm_memory_attributes *attrs) > > +{ > > + gfn_t start, end; > > + > > + /* flags is currently not used. */ > > + if (attrs->flags) > > + return -EINVAL; > > + if (attrs->attributes & ~kvm_supported_mem_attributes(kvm)) > > + return -EINVAL; > > + if (attrs->size == 0 || attrs->address + attrs->size < attrs->address) > > + return -EINVAL; > > + if (!PAGE_ALIGNED(attrs->address) || !PAGE_ALIGNED(attrs->size)) > > + return -EINVAL; > > + > > + start = attrs->address >> PAGE_SHIFT; > > + end = (attrs->address + attrs->size - 1 + PAGE_SIZE) >> PAGE_SHIFT; > > As the attrs->address/size are both garanteed to be non-zero, non-wrap > and page aligned in prevous check. Is it OK to simplify the calculation, > like: > > end = (attrs->address + attrs->size) >> PAGE_SHIFT; Yes, that should work. Chao, am I missing something? Or did we just end up with unnecessarly convoluted code as things evolved? > > + > > + if (WARN_ON_ONCE(start == end)) > > + return -EINVAL; > > Also, is this check possible to be hit? Maybe remove it? It should be impossible to, hence the WARN. I added the check for two reasons: (1) to help document that end is exclusive, and (2) to guard against future bugs.