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=-10.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=no 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 C6027C07E96 for ; Thu, 15 Jul 2021 15:46:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A8998613D0 for ; Thu, 15 Jul 2021 15:46:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239378AbhGOPsx (ORCPT ); Thu, 15 Jul 2021 11:48:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42208 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239371AbhGOPsw (ORCPT ); Thu, 15 Jul 2021 11:48:52 -0400 Received: from mail-pf1-x433.google.com (mail-pf1-x433.google.com [IPv6:2607:f8b0:4864:20::433]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A182BC061760 for ; Thu, 15 Jul 2021 08:45:59 -0700 (PDT) Received: by mail-pf1-x433.google.com with SMTP id b12so5805740pfv.6 for ; Thu, 15 Jul 2021 08:45:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=JGShUfArb1jvxyvk/PluPuoZRLwcvoX9Y9zVRmEA/LE=; b=cvMFpY/OwXDzNZaxDIn8hCltxnXPgoxZFD59snXEi3mihiSlY3m0PQ5CTgOpg4squZ AalmskJzdync5QA4IQCm5ffqiWDxrBvwFYYoNj4OuvK7gHrBpci2GtTUFW9X2DHQN0dR HFbEerlqGDC1d4sIS9bwC7aNuwwnKpggKW6noufPRcHWlNV1y8HITB7LrzR6AarPqQ0y 0YNBcIldHRlrOP0V/M5EsmhAkdJeLG9FECbestqwaA742zw0BGPP1fqk0lZKvrFbreFl hdSX1/zFa2nbrcaIeXkSea65z702JJp8xh2Zp3ZabADykfikzrxIsq/POXNItxZrpDTl Z3yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JGShUfArb1jvxyvk/PluPuoZRLwcvoX9Y9zVRmEA/LE=; b=ip3JFbKjSd6WF5B1Te7AfXYvDQVkvwZ3miniJFO4p8XA8kgAqQwK80t07rO2rsegDM X5HbswA7Ptd+xspzviDq2fppBSY+Jq4P04Jzhk3KGZczW/TDz4bXRinLEhBkjEYtjoIt pyRFxpTEIKwNlk7aY4kciRt5X3pV19S6pROw13dKcfUsr+DySVSU45IsQ2UemsUSvlUJ 21vCt2P60nLPjnhOTd4Vo3OLzSdu2onlWlLWTEJRX0U8ErkoM+wHoeH4+gR7BiZcyrpz JsfDkz7HnwSxotx3GQ2gI2iuwxTf88QYL2/3nlUZ4xtKJrp3669P3Z4PLRuDi8iH7XB5 dr+g== X-Gm-Message-State: AOAM5312TO+hSwyUl/Duy63cE8nThvZyXX7Jy+7V6uEAtOCnM5ap3euY hUdyjnDMWjdreghzUaoNOyo4ag== X-Google-Smtp-Source: ABdhPJwQyW7bUtVpri0q5g8r0vZu8sEyqzBKIbVZWvT5YFaKDe5mAIuTkn7DhgaMa4VDH0TCblUGFA== X-Received: by 2002:a65:6a0a:: with SMTP id m10mr5244127pgu.145.1626363958878; Thu, 15 Jul 2021 08:45:58 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id t37sm7049539pfg.14.2021.07.15.08.45.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Jul 2021 08:45:58 -0700 (PDT) Date: Thu, 15 Jul 2021 15:45:54 +0000 From: Sean Christopherson To: Tom Lendacky Cc: Brijesh Singh , x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-coco@lists.linux.dev, linux-mm@kvack.org, linux-crypto@vger.kernel.org, Thomas Gleixner , Ingo Molnar , Joerg Roedel , "H. Peter Anvin" , Ard Biesheuvel , Paolo Bonzini , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Andy Lutomirski , Dave Hansen , Sergio Lopez , Peter Gonda , Peter Zijlstra , Srinivas Pandruvada , David Rientjes , Dov Murik , Tobin Feldman-Fitzthum , Borislav Petkov , Michael Roth , Vlastimil Babka , tony.luck@intel.com, npmccallum@redhat.com, brijesh.ksingh@gmail.com Subject: Re: [PATCH Part2 RFC v4 01/40] KVM: SVM: Add support to handle AP reset MSR protocol Message-ID: References: <20210707183616.5620-1-brijesh.singh@amd.com> <20210707183616.5620-2-brijesh.singh@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: platform-driver-x86@vger.kernel.org On Thu, Jul 15, 2021, Tom Lendacky wrote: > On 7/14/21 3:17 PM, Sean Christopherson wrote: > >> + case GHCB_MSR_AP_RESET_HOLD_REQ: > >> + svm->ap_reset_hold_type = AP_RESET_HOLD_MSR_PROTO; > >> + ret = kvm_emulate_ap_reset_hold(&svm->vcpu); > > > > The hold type feels like it should be a param to kvm_emulate_ap_reset_hold(). > > I suppose it could be, but then the type would have to be tracked in the > kvm_vcpu_arch struct instead of the vcpu_svm struct, so I opted for the > latter. Maybe a helper function, sev_ap_reset_hold(), that sets the type > and then calls kvm_emulate_ap_reset_hold(), but I'm not seeing a big need > for it. Huh. Why is kvm_emulate_ap_reset_hold() in x86.c? That entire concept is very much SEV specific. And if anyone argues its not SEV specific, then the hold type should also be considered generic, i.e. put in kvm_vcpu_arch. > >> + > >> + /* > >> + * Preset the result to a non-SIPI return and then only set > >> + * the result to non-zero when delivering a SIPI. > >> + */ > >> + set_ghcb_msr_bits(svm, 0, > >> + GHCB_MSR_AP_RESET_HOLD_RESULT_MASK, > >> + GHCB_MSR_AP_RESET_HOLD_RESULT_POS); > >> + > >> + set_ghcb_msr_bits(svm, GHCB_MSR_AP_RESET_HOLD_RESP, > >> + GHCB_MSR_INFO_MASK, > >> + GHCB_MSR_INFO_POS); > > > > It looks like all uses set an arbitrary value and then the response. I think > > folding the response into the helper would improve both readability and robustness. > > Joerg pulled this patch out and submitted it as part of a small, three > patch series, so it might be best to address this in general in the > SEV-SNP patches or as a follow-on series specifically for this re-work. > > > I also suspect the helper needs to do WRITE_ONCE() to guarantee the guest sees > > what it's supposed to see, though memory ordering is not my strong suit. > > This is writing to the VMCB that is then used to set the value of the > guest MSR. I don't see anything done in general for writes to the VMCB, so > I wouldn't think this should be any different. Ooooh, right. I was thinking this was writing memory that's shared with the guest, but this is KVM's copy of the GCHB MSR, not the GHCB itself. Thanks! > > Might even be able to squeeze in a build-time assertion. > > > > Also, do the guest-provided contents actually need to be preserved? That seems > > somewhat odd. > > Hmmm... not sure I see where the guest contents are being preserved. The fact that set_ghcb_msr_bits() is a RMW flow implies _something_ is being preserved. And unless KVM explicitly zeros/initializes control.ghcb_gpa, the value being preserved is the value last written by the guest. E.g. for CPUID emulation, KVM reads the guest-requested function and register from ghcb_gpa, then writes back the result. But set_ghcb_msr_bits() is a RMW on a subset of bits, and thus it's preserving the guest's value for the bits not being written. Unless there is an explicit need to preserve the guest value, the whole RMW thing is unnecessary and confusing. case GHCB_MSR_CPUID_REQ: { u64 cpuid_fn, cpuid_reg, cpuid_value; cpuid_fn = get_ghcb_msr_bits(svm, GHCB_MSR_CPUID_FUNC_MASK, GHCB_MSR_CPUID_FUNC_POS); /* Initialize the registers needed by the CPUID intercept */ vcpu->arch.regs[VCPU_REGS_RAX] = cpuid_fn; vcpu->arch.regs[VCPU_REGS_RCX] = 0; ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_CPUID); if (!ret) { ret = -EINVAL; break; } cpuid_reg = get_ghcb_msr_bits(svm, GHCB_MSR_CPUID_REG_MASK, GHCB_MSR_CPUID_REG_POS); if (cpuid_reg == 0) cpuid_value = vcpu->arch.regs[VCPU_REGS_RAX]; else if (cpuid_reg == 1) cpuid_value = vcpu->arch.regs[VCPU_REGS_RBX]; else if (cpuid_reg == 2) cpuid_value = vcpu->arch.regs[VCPU_REGS_RCX]; else cpuid_value = vcpu->arch.regs[VCPU_REGS_RDX]; set_ghcb_msr_bits(svm, cpuid_value, GHCB_MSR_CPUID_VALUE_MASK, GHCB_MSR_CPUID_VALUE_POS); set_ghcb_msr_bits(svm, GHCB_MSR_CPUID_RESP, GHCB_MSR_INFO_MASK, GHCB_MSR_INFO_POS); break; }