From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f201.google.com (mail-pl1-f201.google.com [209.85.214.201]) (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 D17324C9E for ; Fri, 14 Jul 2023 22:35:53 +0000 (UTC) Received: by mail-pl1-f201.google.com with SMTP id d9443c01a7336-1b8b310553bso17333245ad.3 for ; Fri, 14 Jul 2023 15:35:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1689374153; x=1691966153; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=AqPQXu/AIr+STPlGjrumZEqk7MLinV6fP6qlYwF+Hxk=; b=Tt0V9A0qkpbl5zS0LESlEdwvp+4lS8swsnaM5ygU4p8wmWduWJsTsWiMBUrW9Vh2KL aNiclCU+3JOXB1yiqIKXN6QgR9QdqoTo2RC11ivA+QpKq3HQpnI8PiDAYzg63A260iOT /X8Zs4ITCCFalugl0jsN26LMWffPpuzQBP22vn8lpRSG+H4mTHf3+orEXOoeYZYdfyr2 abFsO9FDujBoCuI48/+aMdXB2jBUEMCB+wK+csXPETYbqUkpzTNRqr4u7awtDvgcPp5C u2l7VKka+4qaqK2LwY7H9dgsRxhvzJ+xsqjkh1lzLWjpI9iGg9n1Apbc27ImlTO1X+KO zawA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689374153; x=1691966153; 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=AqPQXu/AIr+STPlGjrumZEqk7MLinV6fP6qlYwF+Hxk=; b=lGBFvxsUlbmWKVBPgYDMscsN0i1PZ0zYyV9hAr9zT+Ihd1+tzg1YTTcP2GEdcP/8d6 crFf03qb0QzRx0PsL1cKEfeXGitibOUhn4exDCVMld0w+k/Vz1WmsBGJDjjARZzugN6v evW53dH3imLuNX5b14i5pspUK+ZLga9aFMfOcP1UH+dhdWM8DdSjlvnPV+97FyBTFmIO wttjPt51qsgUEKLjqSodCIgl68+AbPxSlH7d2CnqS5apMMGlDy4i166n6L7tJZxJldZu u86TwaakUO0lOniVPIgB5jk08tKtnvycH5JC7Zi4ci2qFqL38KW9G8Uq/ey1hAlg6uGn 9zOA== X-Gm-Message-State: ABy/qLbfgaRwdyakJv7q9o3rA05ZFse5SwWVinpmHe33jU+hg9x8TpDH A+g0oLgALX/fyY/xJHftLMewLbJLJXA= X-Google-Smtp-Source: APBJJlF9Rovb1aladgpgZPHMCXoqG55aen/Zed897i+LVWocHvfN1RJ6IbmpkTUFw7J9N1WkMXgLw6ub6IY= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:902:b492:b0:1b9:e338:a90d with SMTP id y18-20020a170902b49200b001b9e338a90dmr17230plr.3.1689374153026; Fri, 14 Jul 2023 15:35:53 -0700 (PDT) Date: Fri, 14 Jul 2023 22:35:51 +0000 In-Reply-To: <20230714115715.000026fe.zhi.wang.linux@gmail.com> Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <358fb191b3690a5cbc2c985d3ffc67224df11cf3.1687991811.git.isaku.yamahata@intel.com> <20230714115715.000026fe.zhi.wang.linux@gmail.com> Message-ID: Subject: Re: [RFC PATCH v3 08/11] KVM: Fix set_mem_attr ioctl when error case From: Sean Christopherson To: Zhi Wang Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com, Paolo Bonzini , erdemaktas@google.com, Sagi Shahar , David Matlack , Kai Huang , chen.bo@intel.com, linux-coco@lists.linux.dev, Chao Peng , Ackerley Tng , Vishal Annapurve , Michael Roth , Yuan Yao Content-Type: text/plain; charset="us-ascii" On Fri, Jul 14, 2023, Zhi Wang wrote: > On Thu, 13 Jul 2023 15:03:54 -0700 > Sean Christopherson wrote: > > + /* > > + * Reserve memory ahead of time to avoid having to deal with failures > > + * partway through setting the new attributes. > > + */ > > + for (i = start; i < end; i++) { > > + r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT); > > + if (r) > > + goto out_unlock; > > + } > > + > > KVM_MMU_LOCK(kvm); > > kvm_mmu_invalidate_begin(kvm); > > kvm_mmu_invalidate_range_add(kvm, start, end); > > KVM_MMU_UNLOCK(kvm); > > > > for (i = start; i < end; i++) { > > - if (xa_err(xa_store(&kvm->mem_attr_array, i, entry, > > - GFP_KERNEL_ACCOUNT))) > > + r = xa_err(xa_store(&kvm->mem_attr_array, i, entry, > > + GFP_KERNEL_ACCOUNT)); > > + if (KVM_BUG_ON(r, kvm)) > > break; > > } > > > > IIUC, If an error happenes here, we should bail out and call xa_release()? > Or the code below (which is not shown here) still changes the memory attrs > partially. I'm pretty sure we want to continue on. The VM is dead (killed by KVM_BUG_ON()), so the attributes as seen by userspace and/or the VM don't matter. What does matter is that KVM's internal state is consistent, e.g. that KVM doesn't have shared SPTEs while the attributes say a GFN is private. That might not matter for teardown, but I can't think of any reason not to tidy up. And there can also be other ioctls() in flight. KVM_REQ_VM_DEAD ensures vCPU can't enter the guest, and vm->vm_dead ensures no new ioctls() cant start, but neither of those guarantees there aren't other tasks doing KVM things. Regardless, we definitely don't need to do xa_release(). The VM is dead and all its memory will be reclaimed soon enough. And there's no guarantee xa_release() will actually be able to free anything, e.g. already processed entries won't be freed, nor will any entries that existed _before_ the ioctl() was invoked. Not to mention that the xarray probably isn't consuming much memory, relatively speaking. I.e. in the majority of scenarios, it's likely preferable to get out and destroy the VM asap.