From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f171.google.com (mail-pf1-f171.google.com [209.85.210.171]) (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 B7E1070 for ; Thu, 15 Jul 2021 15:45:59 +0000 (UTC) Received: by mail-pf1-f171.google.com with SMTP id j199so5795526pfd.7 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=mnvMPZcU8XuDquGx+XDaeF4umFPXhZLvi9V4fFaRAQEsP4VCRbFRM8Jt3FrpKxmN71 /tzh1xu2ULQkR5VAW1OFH9NV1IDJVFobAl7CYPCQsWZnZcD9qu+Rpk4ObZERWe7cETEK 8/pwDXwmdRkRdfukItaJ+3IDTIiiXrCL5vAwCIeuMzrpO1uhrWkg9vrSKSut7Ka0BvLr i6/OWFn6nfLbYjML54gc6VwsCTEFpwfzs7hynt6e28QCK/MSVTy2f7HUBT1dUnCcywhd zngIHOFnFcXijn3NiIjHydZ5V5Bk0tcuUjpdocl1pxIAgmG1Wmd+M39+g+46IHBPosf1 BBzA== X-Gm-Message-State: AOAM532jfAEk+sJb3ALRi4Po2EIgV+2wZ1zBXKi1iiDw8UU47x6XrRWu lXqEVdv72BzEQbJuNTNl3KzaeQ== 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> Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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; }