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=-11.6 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 B9AC0C636C9 for ; Wed, 21 Jul 2021 19:53:04 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 84EEC61221 for ; Wed, 21 Jul 2021 19:53:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 84EEC61221 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id EE14C6B005D; Wed, 21 Jul 2021 15:53:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E6A366B006C; Wed, 21 Jul 2021 15:53:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CE2DE6B0070; Wed, 21 Jul 2021 15:53:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0201.hostedemail.com [216.40.44.201]) by kanga.kvack.org (Postfix) with ESMTP id AE4AA6B005D for ; Wed, 21 Jul 2021 15:53:02 -0400 (EDT) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 5D616231B8 for ; Wed, 21 Jul 2021 19:53:02 +0000 (UTC) X-FDA: 78387643404.24.966E5CC Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by imf07.hostedemail.com (Postfix) with ESMTP id 159EC100BC17 for ; Wed, 21 Jul 2021 19:53:01 +0000 (UTC) Received: by mail-pj1-f52.google.com with SMTP id bt15so2537130pjb.2 for ; Wed, 21 Jul 2021 12:53:01 -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=1ZFdogakN+U+D6SX6LhX2SwuYGYaPQsFnYtn2JJoTak=; b=tvsyIWhNU3cg/xV3WaOgqDwBpspmMJ50HUSKczTQOHxoia0Gf97fLeNPk6repjxMxU 5jlQjcRGy7FAvAeYmsOGOEYiTkvB0EteJixLlWUNljGhhG0PxH0M7Jr0YMrWsiBC+SVB pIOpLAoFAx54KbgFB0OJI2cv1cLaVXjySbSkqcTiKacy5Jiswx7XbaxYaezXwq5abeO8 d9qYtq/rp/48EjBy9Fe3JmA2tU0HXYTEFUlhqjbmenaY06xhIi59kAojLPYzjgdIIkUG pwhusBBAxUzr7N/6g9a8yVt99NfLdGGQmtyY79jbo2dQWDvQacDkC6QpUzuye67Hh0CG pWSQ== 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=1ZFdogakN+U+D6SX6LhX2SwuYGYaPQsFnYtn2JJoTak=; b=hmQL/kzdqwnSLImSJeuqVoHU9Ktc04RAfTTk2BSKJVAA4vsNJRmF6QJWGZmfRfgugS WVnTsI8QIgjJWCnzgjIR88Qwd8IJLI9fCkWOLtYIOHjW3kBKYXtj1x+4nf3VZL+AtJw7 nm2tCV5md7jt/K8dE/wArZJ2YEwKjRQzpbvZg3KPbaoBJDnCHc9P2pDldLIUkIcx6lN4 7QnEEzayOxABgT905lNXjgbvFvTvd34U8/OOV11QoP5cTF723MMP2/G03ftSTqrVOwnj B/VHFTQxyIoBuZTBGh54IWso8N1IbUXbMJgCxSY6VD2dvLPgK3Frl8VK4btcHO/nnsVp umIA== X-Gm-Message-State: AOAM531LkMXrTIlWxHPuRXa1x2jhtNI4s3csrKusgQkUp7VDwAco1jHR 1CQbB6CT4RlPbFJrGTQkA1N9kA== X-Google-Smtp-Source: ABdhPJx0SNXBSozA1zqonbwhhcSmrLxNop72v66ohRghPV4r836hvfWk3fIwfoZPziXuNwE9t9Hl/Q== X-Received: by 2002:a17:90b:2382:: with SMTP id mr2mr5315994pjb.169.1626897180725; Wed, 21 Jul 2021 12:53:00 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id 5sm31461466pgv.25.2021.07.21.12.52.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Jul 2021 12:52:59 -0700 (PDT) Date: Wed, 21 Jul 2021 19:52:55 +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 40/40] KVM: SVM: Support SEV-SNP AP Creation NAE event Message-ID: References: <20210707183616.5620-1-brijesh.singh@amd.com> <20210707183616.5620-41-brijesh.singh@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=tvsyIWhN; spf=pass (imf07.hostedemail.com: domain of seanjc@google.com designates 209.85.216.52 as permitted sender) smtp.mailfrom=seanjc@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 159EC100BC17 X-Stat-Signature: ngbqytksai37wg4kzrwe7ujfk3xm3nhu X-HE-Tag: 1626897181-410504 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Jul 21, 2021, Tom Lendacky wrote: > On 7/20/21 7:01 PM, Sean Christopherson wrote: > > On Wed, Jul 07, 2021, Brijesh Singh wrote: > >> From: Tom Lendacky > >> + > >> + svm->snp_vmsa_pfn = pfn; > >> + > >> + /* Use the new VMSA in the sev_es_init_vmcb() path */ > >> + svm->vmsa_pa = pfn_to_hpa(pfn); > >> + svm->vmcb->control.vmsa_pa = svm->vmsa_pa; > >> + > >> + vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE; > >> + } else { > >> + vcpu->arch.pv.pv_unhalted = false; > > > > Shouldn't the RUNNABLE path also clear pv_unhalted? > > If anything it should set it, since it will be "unhalted" now. But, I > looked through the code to try and understand if there was a need to set > it and didn't really see any reason. It is only ever set (at least > directly) in one place and is cleared everywhere else. It was odd to me. pv_unhalted is specifically used for a "magic" IPI (KVM hijacked a defunct IPI type) in the context of PV spinlocks. The idea is that a vCPU that's releasing a spinlock can kick the next vCPU in the queue, and KVM will directly yield to the vCPU being kicked so that the guest can efficiently make forward progress. So it's not wrong to leave pv_unhalted as is, but it's odd to clear it in the DESTROY case but not CREATE_INIT case. It should be a moot point, as a sane implementation should make it impossible to get to CREATE with pv_unhalted=1. > >> + vcpu->arch.mp_state = KVM_MP_STATE_UNINITIALIZED; > > > > What happens if userspace calls kvm_arch_vcpu_ioctl_set_mpstate, or even worse > > the guest sends INIT-SIPI? Unless I'm mistaken, either case will cause KVM to > > run the vCPU with vmcb->control.vmsa_pa==0. > > Using the INVALID_PAGE value now (and even when it was 0), you'll get a > VMRUN failure. > > The AP CREATE_ON_INIT is meant to be used with INIT-SIPI, so if the guest > hasn't done the right thing, then it will fail on VMRUN. > > > > > My initial reaction is that the "offline" case needs a new mp_state, or maybe > > just use KVM_MP_STATE_STOPPED. > > I'll look at KVM_MP_STATE_STOPPED. Qemu doesn't reference that state at > all in the i386 support, though, which is why I went with UNINITIALIZED. Ya, it'd effectively be a new feature. My concern with UNINITIALIZED is that it be impossible for KVM to differentiate between "never run" and "destroyed and may have an invalid VMSA" without looking at the VMSA. > >> + mutex_lock(&target_svm->snp_vmsa_mutex); > > > > This seems like it's missing a big pile of sanity checks. E.g. KVM should reject > > SVM_VMGEXIT_AP_CREATE if the target vCPU is already "created", including the case > > where it was "created_on_init" but hasn't yet received INIT-SIPI. > > Why? If the guest wants to call it multiple times I guess I don't see a > reason that it would need to call DESTROY first and then CREATE. I don't > know why a guest would want to do that, but I don't think we should > prevent it. Because "creating" a vCPU that already exists is non-sensical. Ditto for onlining a vCPU that is already onlined. E.g. from the guest's perspective, I would fully expect a SVM_VMGEXIT_AP_CREATE to fail, not effectively send the vCPU to an arbitrary state. Any ambiguity as to the legality of CREATE/DESTROY absolutely needs to be clarified in the GHCB. > >> + > >> + target_svm->snp_vmsa_gpa = 0; > >> + target_svm->snp_vmsa_update_on_init = false; > >> + > >> + /* Interrupt injection mode shouldn't change for AP creation */ > >> + if (request < SVM_VMGEXIT_AP_DESTROY) { > >> + u64 sev_features; > >> + > >> + sev_features = vcpu->arch.regs[VCPU_REGS_RAX]; > >> + sev_features ^= sev->sev_features; > >> + if (sev_features & SVM_SEV_FEATURES_INT_INJ_MODES) { > > > > Why is only INT_INJ_MODES checked? The new comment in sev_es_sync_vmsa() explicitly > > states that sev_features are the same for all vCPUs, but that's not enforced here. > > At a bare minimum I would expect this to sanity check SVM_SEV_FEATURES_SNP_ACTIVE. > > That's because we can't really enforce it. The SEV_FEATURES value is part > of the VMSA, of which the hypervisor has no insight into (its encrypted). > > The interrupt injection mechanism was specifically requested as a sanity > check type of thing during the GHCB review, and as there were no > objections, it was added (see the end of section 4.1.9). > > I can definitely add the check for the SNP_ACTIVE bit, but it isn't required. I'm confused. If we've no insight into what the guest is actually using, what's the point of the INT_INJ_MODES check?