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 54970373BFE for ; Thu, 12 Mar 2026 18:13:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773339230; cv=none; b=tP7P0sSnqTy+ZJIbJ4UhS3Kny+vJMt8n38NS5RYgr33iu7qmNayWgbe70BSmqTk8F7/GFHony5nSMv/wmsAkIBoXQq/VguFjY+UKddpXPgPtcUFsDGh06ik4hsW+r1K0cWLgKXOe9IEggW0f8W1HqSHE87/+Huuf3PrzaYnqcvI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773339230; c=relaxed/simple; bh=Ekhb+qCQy2ym84BjGnRnJ9+llgn8+iN3NBF0eKdAvnY=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=A6r8OoJ5w54hvgY+k2+WVekxBiha6FoGHV9APXWzdyEfec/WUaYMA6Mgo3nHZ2HJTO91ysYZ2FqRqFcuCjtvj7EjXyhFI2OhCgSZJ/9NPAPL5KXESlbGkiogTgnjZKYVSZqjWq5LcRPyjuRbpYO4YHu2cf0NXRTdtvNh4BTeV5g= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=nDmnky35; arc=none smtp.client-ip=209.85.214.201 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="nDmnky35" Received: by mail-pl1-f201.google.com with SMTP id d9443c01a7336-2aea7747aeeso14836045ad.2 for ; Thu, 12 Mar 2026 11:13:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1773339227; x=1773944027; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=4zIIF1a0OywISACpCkGA6XhGEw2fktBd7kbIM2f4eTQ=; b=nDmnky35LALUFPQdvBwPVPNxo0JOEv4lA9qjlPUOeIM4kBfW7UAP9V1uWQ69P8BfmR Qm1AVmYwEKGHsxzLG4yRyHediLAr66Fke+C5ktK1SvLtGx05GIYtelku1f13r1dhLJI3 13bw1Djeo9t6h1pLOk7D7fjpQQTxvrKV+GHdtzPPUuBuS4T9Jh5ItOT57P+hN/UERxgL 9E65HoLMyBKEfWSe37iRhMGo1xmhr1GFuXhmqx6dCvx+fBXadgx/DJuwrHw4f0zVqhBX UkXbfB9kd2hgQU66AWMYH5aH9MjYfeBOyf4DyFsvtxX1x91mBRQoApPHLnYsBPsC5Rr8 WcHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1773339227; x=1773944027; 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=4zIIF1a0OywISACpCkGA6XhGEw2fktBd7kbIM2f4eTQ=; b=u/vEmYEQLG9IeUu+yTmAFaFfynhDGrck0uCXHK8Po89G2FBILVU7WX4eZ7/RnnEgaa zQxyjoPwdNxUHERwAzeZ+YfV5sDOk9NdkTkcO+7Jf5MRYdEmyVqDenAIdqfF3T6Gx85A vrUNcyFRiz07Vr5kVYRhlX32Ede8OtFqZHG6t51NfPculp0FVkhGfSJhIowGlLCbH2bx 6TGk/r2//x+VSiPvt2ADLNnsrHpUjRuGXtW87VkjRvV9P1qHCJNThzFyqlSo5RMpfttu pqpDlnMFWBCAQA68faU7BsQ4l7JxknBlCAWKSLeOsKD8EJ8IYl1ry7AZzCmQfsV9I5hi BUdQ== X-Forwarded-Encrypted: i=1; AJvYcCW5+o4mvYMYxSlP+FVNxtV1yxpeXoghzytMW2p8E5rnEqqAcU9I3GqMp87Z67LWQ7HniKPWSW+UDikOjuk=@vger.kernel.org X-Gm-Message-State: AOJu0YxO2mo8+6FJvoyPNsco4d6ULXwLVxxgg+NLfVFgpuTnW33ouzk3 2Yw2ZgQUGlDUihX6M4iSZLWMRry4W/yGmu8TnrDHySI4vPv9YC9GeqCBD6q1OMQN20I1FAIa2FX qoGYx3g== X-Received: from plbke13.prod.google.com ([2002:a17:903:340d:b0:2ae:48ed:5526]) (user=seanjc job=prod-delivery.src-stubby-dispatcher) by 2002:a17:902:ce11:b0:2ae:c34d:8c4e with SMTP id d9443c01a7336-2aecaa9e9c0mr3694775ad.38.1773339226471; Thu, 12 Mar 2026 11:13:46 -0700 (PDT) Date: Thu, 12 Mar 2026 11:13:44 -0700 In-Reply-To: <20260306210900.1933788-3-yosry@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20260306210900.1933788-1-yosry@kernel.org> <20260306210900.1933788-3-yosry@kernel.org> Message-ID: Subject: Re: [PATCH v2 2/6] KVM: nSVM: Simplify error handling of nested_svm_copy_vmcb12_to_cache() From: Sean Christopherson To: Yosry Ahmed Cc: Paolo Bonzini , Jim Mattson , kvm@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="us-ascii" On Fri, Mar 06, 2026, Yosry Ahmed wrote: > nested_svm_vmrun() currently stores the return value of > nested_svm_copy_vmcb12_to_cache() in a local variable 'err', separate > from the generally used 'ret' variable. This is done to have a single > call to kvm_skip_emulated_instruction(), such that we can store the > return value of kvm_skip_emulated_instruction() in 'ret', and then > re-check the return value of nested_svm_copy_vmcb12_to_cache() in 'err'. > > The code is unnecessarily confusing. Instead, call > kvm_skip_emulated_instruction() in the failure path of > nested_svm_copy_vmcb12_to_cache() if the return value is not -EFAULT, > and drop 'err'. > > Suggested-by: Sean Christopherson > Signed-off-by: Yosry Ahmed FYI, I'm going to grab this right now to make it slightly easier to resolve the merge conflict with Paolo's SMM fixes (the ret vs. err stuff is so confusing). > --- > arch/x86/kvm/svm/nested.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index b191c6cab57db..6d4c053778b21 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -1079,7 +1079,7 @@ static int nested_svm_copy_vmcb12_to_cache(struct kvm_vcpu *vcpu, u64 vmcb12_gpa > int nested_svm_vmrun(struct kvm_vcpu *vcpu) > { > struct vcpu_svm *svm = to_svm(vcpu); > - int ret, err; > + int ret; > u64 vmcb12_gpa; > struct vmcb *vmcb01 = svm->vmcb01.ptr; > > @@ -1104,19 +1104,20 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu) > return -EINVAL; > > vmcb12_gpa = svm->vmcb->save.rax; > - err = nested_svm_copy_vmcb12_to_cache(vcpu, vmcb12_gpa); > - if (err == -EFAULT) { > + ret = nested_svm_copy_vmcb12_to_cache(vcpu, vmcb12_gpa); > + > + /* > + * Advance RIP if #GP or #UD are not injected, but otherwise > + * stop if copying and checking vmcb12 failed. > + */ > + if (ret == -EFAULT) { > kvm_inject_gp(vcpu, 0); > return 1; > + } else if (ret) { > + return kvm_skip_emulated_instruction(vcpu); > } I strongly dislike the if-elif approach, because it makes unnecessarily hard to see that *all* ret !=0 cases are handled, i.e. that overwriting ret below is ok. The comment is also super confusing, because there's no #UD in sight, but there is a #GP. This is what I have locally and am planning on pushing to kvm-x86/next. ret = nested_svm_copy_vmcb12_to_cache(vcpu, vmcb12_gpa); if (ret) { if (ret == -EFAULT) { kvm_inject_gp(vcpu, 0); return 1; } /* Advance RIP past VMRUN as part of the nested #VMEXIT. */ return kvm_skip_emulated_instruction(vcpu); } /* At this point, VMRUN is guaranteed to not fault; advance RIP. */ ret = kvm_skip_emulated_instruction(vcpu); > > - /* > - * Advance RIP if #GP or #UD are not injected, but otherwise stop if > - * copying and checking vmcb12 failed. > - */ > ret = kvm_skip_emulated_instruction(vcpu); > - if (err) > - return ret; > > /* > * Since vmcb01 is not in use, we can use it to store some of the L1 > -- > 2.53.0.473.g4a7958ca14-goog >