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 X-Spam-Level: X-Spam-Status: No, score=-18.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EDA0CC433FE for ; Tue, 14 Sep 2021 15:26:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CEF0060234 for ; Tue, 14 Sep 2021 15:26:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233908AbhINP1t (ORCPT ); Tue, 14 Sep 2021 11:27:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51726 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233858AbhINP1r (ORCPT ); Tue, 14 Sep 2021 11:27:47 -0400 Received: from mail-pg1-x535.google.com (mail-pg1-x535.google.com [IPv6:2607:f8b0:4864:20::535]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AA8F6C061574 for ; Tue, 14 Sep 2021 08:26:24 -0700 (PDT) Received: by mail-pg1-x535.google.com with SMTP id w7so13041286pgk.13 for ; Tue, 14 Sep 2021 08:26:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=N7ykuFrZgQNrrQ2Zlhx5pG6g+CHOtU1cJGoIFqwfvqU=; b=XPLge0u+LIdLxAg6W9OajtpRKOHwPg7NImAAwLgOkHLlOB4pzG81/2uI/5+TBdkzK0 SjWs9VLQfLHDujNI+QUUcELIYERuGqUONenOqbTTlq06ysbL3lKwInjCMSq1MEd7nWIP 1XX4OTiDk4NZPxpkSEx6cl7JJ/Z5Dl9XcAVlec9vn1F8Zy4Ntrfk7hm5H9ihrZNMuxRU gw5CZ5Zzy1qdWztusbd48QGxsKRf5gqkDEVLwvrV/64N5m8qz+POFJWKsmgkQo1LuI9t 55CbRsskYOwwzxQFbsjYdRTo59Ck4QMihNmKWnYPgCchsft87zGSIT+R4ZoJewobg6sK qVwQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=N7ykuFrZgQNrrQ2Zlhx5pG6g+CHOtU1cJGoIFqwfvqU=; b=i7IN7LyM2WLlY3Cl9MPcX6zo+Tt7IxJ+dpAlr8HX741vIM3HtFZeF/iYhtQSALUqTQ EiHr+ZpaCyY1oNpbWVOIutiIcX2djENqnyBlb2gqprMOozUqYCm6Z1oCu1jfIHPDi+07 ROtF2mkjLOqfsX06qABRkqdRyH4YW7+h9UL49SuqStVtV9CJ6dX0mJrAusgNrp7eObQk +gM92eIdjlC0+tNfa4QVABIKh16L0PaLi1jRQOZQkT4h076Do3BD9xk/qX4kNBo23JXn zzk4iuvGAyTZugkw4u30bbTFc/Yq3cvXuOk2yHvry0AGn/j3dYDgQPTTrRTlzHHNRrc6 aThg== X-Gm-Message-State: AOAM5301NjwEyqP5vwfSFSrmjaLU4fZps042h/xYcm7ThUCtrgKR6ING nweJt7J+mv9lFWnpX+aDiq0d/w== X-Google-Smtp-Source: ABdhPJw1SHGjvAYnjmvm6bYe5xG2pNY28wJqBOTLgNEVctGBB21oROpxDsdEjZcvt8UkYkGczQH2og== X-Received: by 2002:aa7:8e56:0:b029:3cd:c2ec:6c1c with SMTP id d22-20020aa78e560000b02903cdc2ec6c1cmr5399378pfr.80.1631633183972; Tue, 14 Sep 2021 08:26:23 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id v7sm2005297pjg.34.2021.09.14.08.26.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Sep 2021 08:26:23 -0700 (PDT) Date: Tue, 14 Sep 2021 15:26:19 +0000 From: Sean Christopherson To: Mingwei Zhang Cc: Paolo Bonzini , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Alper Gun , Borislav Petkov , Brijesh Singh , David Rienjes , Marc Orr , John Allen , Peter Gonda , Tom Lendacky , Vipin Sharma Subject: Re: [PATCH] KVM: SVM: fix missing sev_decommission in sev_receive_start Message-ID: References: <20210912181815.3899316-1-mizhang@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210912181815.3899316-1-mizhang@google.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Sep 12, 2021, Mingwei Zhang wrote: > sev_decommission It's not a ubiquitous "requirement", but adding parantheses after function names in changelogs is generally preferred as it succinctly identifies a function call. > is needed in the error path of sev_bind_asid. The purpose "error path of sev_bind_asid()" could be interpreted as meaning DECOMMISSION is needed within sev_bind_asid(), whereas you mean when sev_bind_asid() fails. One way to avoid confusion, and a good habit in general, is to avoid talking in terms of code details when possible. There are certainly cases where talking about the code itself is absolutely necessary, but this could be worded as: DECOMMISSION the current SEV context if binding an ASID fails after RECEIVE_START. Per AMD's SEV API, RECEIVE_START generates a new guest context and thus needs to be paired with DECOMMISSION: The RECEIVE_START command is the only command other than the LAUNCH_START command that generates a new guest context and guest handle. The missing DECOMMISSION can result in subsequent SEV launch failures due to . Note, LAUNCH_START suffered the same bug, but was previously fixed by 934002cd660b ("KVM: SVM: Call SEV Guest Decommission if ASID binding fails"). > of this function is to clear the firmware context. Missing this step may Kind of a nit: "this function" is ambiguous. I'm pretty sure you're referring to sev_decommission(), but it's trivially easy to be 100% unambiguous (see above). > cause subsequent SEV launch failures. > > Although missing sev_decommission issue has previously been found and was > fixed in sev_launch_start function. It is supposed to be fixed on all > scenarios where a firmware context needs to be freed. More nits: provide the commit ID of the previous fix so that folks don't have to hunt it down (even though it's an easy git blame away). And this is better as a footnote of sorts as it's not relevant to the justification of the patch in the sense that it's best to avoid "we do x there, so we should do x here". That might be a true statement, as is the case here, but the patch still needs to be justified without that type of reasoning. > According to the AMD SEV API v0.24 Section 1.3.3: > > "The RECEIVE_START command is the only command other than the LAUNCH_START > command that generates a new guest context and guest handle." > > The above indicates that RECEIVE_START command also requires calling > sev_decommission if ASID binding fails after RECEIVE_START succeeds. > > So add the sev_decommission function in sev_receive_start. And more nits :-) As alluded to above the, last few sentences are somewhat redundant and can be dropped and/or worded into the statement about what the patch is doing. > Cc: Alper Gun > Cc: Borislav Petkov > Cc: Brijesh Singh > Cc: David Rienjes > Cc: Marc Orr > Cc: John Allen > Cc: Peter Gonda > Cc: Sean Christopherson > Cc: Tom Lendacky > Cc: Vipin Sharma > > Reviewed-by: Marc Orr > Acked-by: Brijesh Singh > Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command") Cc: stable@vger.kernel.org > Signed-off-by: Mingwei Zhang > --- With a cleaned up changelog, Reviewed-by: Sean Christopherson