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=-15.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,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 13553C12002 for ; Fri, 16 Jul 2021 19:31:38 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A3FBB613EB for ; Fri, 16 Jul 2021 19:31:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A3FBB613EB 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 BCA3B8D00F4; Fri, 16 Jul 2021 15:31:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B7AE78D00EC; Fri, 16 Jul 2021 15:31:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9F4358D00F4; Fri, 16 Jul 2021 15:31:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0019.hostedemail.com [216.40.44.19]) by kanga.kvack.org (Postfix) with ESMTP id 788E48D00EC for ; Fri, 16 Jul 2021 15:31:36 -0400 (EDT) Received: from smtpin31.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 28B2E18607147 for ; Fri, 16 Jul 2021 19:31:35 +0000 (UTC) X-FDA: 78369445350.31.50A1739 Received: from mail-pg1-f171.google.com (mail-pg1-f171.google.com [209.85.215.171]) by imf25.hostedemail.com (Postfix) with ESMTP id CF327B00008E for ; Fri, 16 Jul 2021 19:31:34 +0000 (UTC) Received: by mail-pg1-f171.google.com with SMTP id d12so10914974pgd.9 for ; Fri, 16 Jul 2021 12:31:34 -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=/hHZcYMza7NE4Psi5qcc23pedu+uGbkYbTmPoTjnedM=; b=BN12JV2QP6xeOYm+P7hOWRg91H97OO5pAZkKmHIYIuBIsE6PXqOVX7aRlVHXE+qBaj jJsjE1orHInfKykBM3svTQcefVtGB158y9NKaAtpTnV2FoHRr0MElSWAxuo65Dyp+b5T VSYqHWDHjILxcboknF11CscuZbazyHg4SrSNNazKexI2pjjaz7dlJLS9L3JomRvDK4DZ 4NChiXc+J8PrnRle1d+qfVMEaxEBnB08RcOA/+rXxoNxfQyOBCpaxai418Ls7VTz1/hZ yrp4XyPno2exnVEcQ4oP8W1q6dCyvwk7MDe+/6l2RkOTxbT5ZO/dKqe4MwDga+yEqmdl /P1g== 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=/hHZcYMza7NE4Psi5qcc23pedu+uGbkYbTmPoTjnedM=; b=MR0a7MyBj4jKhSzLkBe3eDW5XZs/5j69ZR5bUzBXMa+aE6rb6qTuoCx2pg9EYBbtyb yxQHAUkTu68ubSAMMJcDYAPgYS9dGnbt+Uws2pSWfbT3GStQPF2ljvKGqvegKfu+EzLX XlQrN9NAI7KOJUvo1aAQQVxPh7yOiwXUKaN3tMQCDZUX7s0JiFr0VJvlr7sevuis1bqR MOSt/0Q87Eys7Wk0nwf3sD+fkpTgC0WKa7/9IjToGoAOK3bVnSzpE3ZqhV/IGY15pG/r 4wDl75KIFHCf2e9UDy93QM/c1Qc/JnOgaJ1nOo40e9rhzoRj36i42SgahWs88PpJA12L 66+A== X-Gm-Message-State: AOAM533bA13ShJWQcIhpwMwbyTsgm0sW36tkgnigvtJ5rGwkMdWmFawX Zs5SNB123dYzUr9Yio+zLMACwQ== X-Google-Smtp-Source: ABdhPJw/Tl6imvxSRs9HdGQZlRO+Ko27yJBuT5jRtOwEVQDmy165qXol9Cy0ASigSmkGWdJ7jWp16w== X-Received: by 2002:a65:450d:: with SMTP id n13mr11562222pgq.13.1626463893304; Fri, 16 Jul 2021 12:31:33 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id v7sm3142968pjk.37.2021.07.16.12.31.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 16 Jul 2021 12:31:32 -0700 (PDT) Date: Fri, 16 Jul 2021 19:31:29 +0000 From: Sean Christopherson To: Brijesh Singh Cc: 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 , Tom Lendacky , "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 21/40] KVM: SVM: Add initial SEV-SNP support Message-ID: References: <20210707183616.5620-1-brijesh.singh@amd.com> <20210707183616.5620-22-brijesh.singh@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=BN12JV2Q; spf=pass (imf25.hostedemail.com: domain of seanjc@google.com designates 209.85.215.171 as permitted sender) smtp.mailfrom=seanjc@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam02 X-Stat-Signature: j3fa6giccau3j49fwdg3efm7jor4hdug X-Rspamd-Queue-Id: CF327B00008E X-HE-Tag: 1626463894-609181 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 Fri, Jul 16, 2021, Brijesh Singh wrote: > > On 7/16/21 1:00 PM, Sean Christopherson wrote: > > On Wed, Jul 07, 2021, Brijesh Singh wrote: > >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > >> index 411ed72f63af..abca2b9dee83 100644 > >> --- a/arch/x86/kvm/svm/sev.c > >> +++ b/arch/x86/kvm/svm/sev.c > >> @@ -52,9 +52,14 @@ module_param_named(sev, sev_enabled, bool, 0444); > >> /* enable/disable SEV-ES support */ > >> static bool sev_es_enabled = true; > >> module_param_named(sev_es, sev_es_enabled, bool, 0444); > >> + > >> +/* enable/disable SEV-SNP support */ > >> +static bool sev_snp_enabled = true; > > Is it safe to incrementally introduce SNP support? Or should the module param > > be hidden until all support is in place? E.g. what will happen when KVM allows > > userspace to create SNP guests but doesn't yet have the RMP management added? > > The SNP support depends on the RMP management. At least the patch > ordering in this series adds the RMP management first then updates > drivers to use the RMP specific APIs. Yep, got that. > If RMP is not initialized due to someone not picking the commits in the > order, then SNP guest creation will fail. That's not what I was asking. My question is if KVM will break/fail if someone runs a KVM build with SNP enabled halfway through the series. E.g. if I make a KVM build at patch 22, "KVM: SVM: Add KVM_SNP_INIT command", what will happen if I attempt to launch an SNP guest? Obviously it won't fully succeed, but will KVM fail gracefully and do all the proper cleanup? Repeat the question for all patches between this one and the final patch of the series. SNP simply not working is ok, but if KVM explodes or does weird things without "full" SNP support, then at minimum the module param should be off by default until it's safe to enable. E.g. for the TDP MMU, I believe the approach was to put all the machinery in place but not actually let userspace flip on the module param until the full implementation was ready. Bisecting and testing the individual commits is a bit painful because it requires modifying KVM code, but on the plus side unrelated bisects won't stumble into a half-baked state. > >> +module_param_named(sev_snp, sev_snp_enabled, bool, 0444); > >> #else > >> #define sev_enabled false > >> #define sev_es_enabled false > >> +#define sev_snp_enabled false > >> #endif /* CONFIG_KVM_AMD_SEV */ > >> > >> #define AP_RESET_HOLD_NONE 0 > >> @@ -1825,6 +1830,7 @@ void __init sev_hardware_setup(void) > >> { > >> #ifdef CONFIG_KVM_AMD_SEV > >> unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count; > >> + bool sev_snp_supported = false; > >> bool sev_es_supported = false; > >> bool sev_supported = false; > >> > >> @@ -1888,9 +1894,21 @@ void __init sev_hardware_setup(void) > >> pr_info("SEV-ES supported: %u ASIDs\n", sev_es_asid_count); > >> sev_es_supported = true; > >> > >> + /* SEV-SNP support requested? */ > >> + if (!sev_snp_enabled) > >> + goto out; > >> + > >> + /* Is SEV-SNP enabled? */ > >> + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) > > Random question, why use cpu_feature_enabled? Did something change in cpufeatures > > that prevents using boot_cpu_has() here? > > > During the boot the kernel initialize the RMP table. If RMP table > initialization fail, then X86_FEATURE_SEV_SNP is cleared. In that case, > the cpu_feature_enabled() should return false. The idea is, > cpu_feature_enabled() will be set only when the RMP table is > successfully initialized and SYSCFG.SNP is set. Ya, got that, but again not what I was asking :-) Why use cpu_feature_enabled() instead of boot_cpu_has()? As a random developer, I would fully expect that boot_cpu_has(X86_FEATURE_SEV_SNP) is true iff SNP is fully enabled by the kernel. > >> + goto out; > >> + > >> + pr_info("SEV-SNP supported: %u ASIDs\n", min_sev_asid - 1); > > Use sev_es_asid_count instead of manually recomputing the same; the latter > > obfuscates the fact that ES and SNP share the same ASID pool. > > > > Even better would be to report ES+SNP together, otherwise the user could easily > > interpret ES and SNP having separate ASID pools. And IMO the gotos for SNP are > > overkill, e.g. > > > > sev_es_supported = true; > > sev_snp_supported = sev_snp_enabled && > > cpu_feature_enabled(X86_FEATURE_SEV_SNP); > > > > pr_info("SEV-ES %ssupported: %u ASIDs\n", > > sev_snp_supported ? "and SEV-SNP " : "", sev_es_asid_count); > > > >> +static inline bool sev_snp_guest(struct kvm *kvm) > >> +{ > >> +#ifdef CONFIG_KVM_AMD_SEV > >> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > >> + > >> + return sev_es_guest(kvm) && sev->snp_active; > > Can't this be reduced to: > > > > return to_kvm_svm(kvm)->sev_info.snp_active; > > > > KVM should never set snp_active without also setting es_active. > > > The approach here is similar to SEV/ES. IIRC, it was done mainly to > avoid adding dead code when CONFIG_KVM_AMD_SEV is disabled. But this is already in an #ifdef, checking sev_es_guest() is pointless.