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=-5.7 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FSL_HELO_FAKE,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 B20EFC12002 for ; Fri, 16 Jul 2021 21:01:41 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3EC00613FB for ; Fri, 16 Jul 2021 21:01:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3EC00613FB 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 956CC8D00F4; Fri, 16 Jul 2021 17:01:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 906478D00EC; Fri, 16 Jul 2021 17:01:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7596B8D00F4; Fri, 16 Jul 2021 17:01:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0128.hostedemail.com [216.40.44.128]) by kanga.kvack.org (Postfix) with ESMTP id 49E828D00EC for ; Fri, 16 Jul 2021 17:01:41 -0400 (EDT) Received: from smtpin34.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 1176D252DD for ; Fri, 16 Jul 2021 21:01:40 +0000 (UTC) X-FDA: 78369672360.34.5D7C2FB Received: from mail-pf1-f180.google.com (mail-pf1-f180.google.com [209.85.210.180]) by imf04.hostedemail.com (Postfix) with ESMTP id A33D850000A4 for ; Fri, 16 Jul 2021 21:00:39 +0000 (UTC) Received: by mail-pf1-f180.google.com with SMTP id 17so9926920pfz.4 for ; Fri, 16 Jul 2021 14:00:39 -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:content-transfer-encoding:in-reply-to; bh=32owgdGkkMeFKYJKP96aJ0cepPQZmV7ASPwAnVQdyQ8=; b=mWGfXWGNFAoNVa/XQEMEHBGq3nEvz+50vTLu166iLOmPRHDnbyzkms0w3ML3zNZeWP GVWuqECe5KirV8LFrcLji/GmzrQ7GVbVpBIE3e5EzaXDLr6WyUx5EmX+jj1MebxKkklO 4FV8/YtI4QEurHcBfFD436NAdnosWqA7ukGoXATVmBIhLQbTxj1nAy4LNBJ4Rs+/Y1gn frv72OoilRYqDRBvJQD2gN25MIilFA3t3XV9nKRiPei7WUH6uGHQRcrdLe3+TebOCdDS 53ONDWobK9c65N8sBOfetN8OnTEMw4ULi/OqXPosKkR+Pd3UuFCKTM65gtt0ZUlrb545 JcAA== 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:content-transfer-encoding :in-reply-to; bh=32owgdGkkMeFKYJKP96aJ0cepPQZmV7ASPwAnVQdyQ8=; b=A4SHiHkVpEmE2O+Cn1+4+1vWkW/Usb+UspSy/9jqtpdHlbdinhKfbcC3Hg+3+M7uE0 gVs8IhS91qsOIUT6X0isWu50LI7TKlXvcx6fgehUwoC2mAU7Ia9kBNq1D6kR20C11cFi 4TTjvL3IViTsc66dB9ErPAk7ZQ0+BfhOZAI8+1U7x7PlYvl/QKORVVbouj38vGVV6tB4 6iw3z9mPiNirSjfdOZ9cQDHqoN6vmp6DwiREU5RjGsRMtRnaM/DhKRm1ZKVlU51oMHxK wDhLGwM8+1AK2bfUYhzvFS6X9CiA/GTWe7PFm9lAD78SSFPgcKjfMlE/mumAmnqu4usf RunA== X-Gm-Message-State: AOAM533i+uzNf+JWN2DQWH5eMWjXPmG1LtwqiRNPgEgLI/fi3djpZGn2 fFrC3NPlkFrlQ9XLT6XKnVH1Xg== X-Google-Smtp-Source: ABdhPJwjG+4AKAUsolr4NhSH0onD6XOfm/OvTT2xz/g+OQ4KDyJFfI4r0VZGJsgBVJZPZ187YQk3wQ== X-Received: by 2002:a62:520e:0:b029:30b:fc21:975d with SMTP id g14-20020a62520e0000b029030bfc21975dmr12345822pfb.57.1626469238280; Fri, 16 Jul 2021 14:00:38 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id f18sm6484622pfe.25.2021.07.16.14.00.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 16 Jul 2021 14:00:37 -0700 (PDT) Date: Fri, 16 Jul 2021 21:00:34 +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 33/40] KVM: SVM: Add support to handle MSR based Page State Change VMGEXIT Message-ID: References: <20210707183616.5620-1-brijesh.singh@amd.com> <20210707183616.5620-34-brijesh.singh@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210707183616.5620-34-brijesh.singh@amd.com> X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: A33D850000A4 X-Stat-Signature: qzyep4dionojo5kx8qg1summ5bdi571x Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=mWGfXWGN; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf04.hostedemail.com: domain of seanjc@google.com designates 209.85.210.180 as permitted sender) smtp.mailfrom=seanjc@google.com X-HE-Tag: 1626469239-87925 Content-Transfer-Encoding: quoted-printable 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 07, 2021, Brijesh Singh wrote: > +static int __snp_handle_psc(struct kvm_vcpu *vcpu, int op, gpa_t gpa, = int level) I can live with e.g. GHCB_MSR_PSC_REQ, but I'd strongly prefer to spell t= his out, e.g. __snp_handle_page_state_change() or whatever. I had a hell of a tim= e figuring out what PSC was the first time I saw it in some random context. > +{ > + struct kvm *kvm =3D vcpu->kvm; > + int rc, tdp_level; > + kvm_pfn_t pfn; > + gpa_t gpa_end; > + > + gpa_end =3D gpa + page_level_size(level); > + > + while (gpa < gpa_end) { > + /* > + * Get the pfn and level for the gpa from the nested page table. > + * > + * If the TDP walk failed, then its safe to say that we don't have a= valid > + * mapping for the gpa in the nested page table. Create a fault to m= ap the > + * page is nested page table. > + */ > + if (!kvm_mmu_get_tdp_walk(vcpu, gpa, &pfn, &tdp_level)) { > + pfn =3D kvm_mmu_map_tdp_page(vcpu, gpa, PFERR_USER_MASK, level); > + if (is_error_noslot_pfn(pfn)) > + goto out; > + > + if (!kvm_mmu_get_tdp_walk(vcpu, gpa, &pfn, &tdp_level)) > + goto out; > + } > + > + /* Adjust the level so that we don't go higher than the backing page= level */ > + level =3D min_t(size_t, level, tdp_level); > + > + write_lock(&kvm->mmu_lock); Retrieving the PFN and level outside of mmu_lock is not correct. Because= the pages are pinned and the VMM is not malicious, it will function as intend= ed, but it is far from correct. The overall approach also feels wrong, e.g. a guest won't be able to conv= ert a 2mb chunk back to a 2mb large page if KVM mapped the GPA as a 4kb page in= the past (from a different conversion). I'd also strongly prefer to have a common flow between SNP and TDX for co= nverting between shared/prviate. I'll circle back to this next week, it'll probably take a few hours of st= aring to figure out a solution, if a common one for SNP+TDX is even possible. > + > + switch (op) { > + case SNP_PAGE_STATE_SHARED: > + rc =3D snp_make_page_shared(vcpu, gpa, pfn, level); > + break; > + case SNP_PAGE_STATE_PRIVATE: > + rc =3D snp_make_page_private(vcpu, gpa, pfn, level); > + break; > + default: > + rc =3D -EINVAL; > + break; > + } > + > + write_unlock(&kvm->mmu_lock); > + > + if (rc) { > + pr_err_ratelimited("Error op %d gpa %llx pfn %llx level %d rc %d\n"= , > + op, gpa, pfn, level, rc); > + goto out; > + } > + > + gpa =3D gpa + page_level_size(level); > + } > + > +out: > + return rc; > +} > + > static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) > { > struct vmcb_control_area *control =3D &svm->vmcb->control; > @@ -2941,6 +3063,25 @@ static int sev_handle_vmgexit_msr_protocol(struc= t vcpu_svm *svm) > GHCB_MSR_INFO_POS); > break; > } > + case GHCB_MSR_PSC_REQ: { > + gfn_t gfn; > + int ret; > + u8 op; > + > + gfn =3D get_ghcb_msr_bits(svm, GHCB_MSR_PSC_GFN_MASK, GHCB_MSR_PSC_G= FN_POS); > + op =3D get_ghcb_msr_bits(svm, GHCB_MSR_PSC_OP_MASK, GHCB_MSR_PSC_OP_= POS); > + > + ret =3D __snp_handle_psc(vcpu, op, gfn_to_gpa(gfn), PG_LEVEL_4K); > + > + /* If failed to change the state then spec requires to return all F'= s */ That doesn't mesh with what I could find: o 0x015 =E2=80=93 SNP Page State Change Response =E2=96=AA GHCBData[63:32] =E2=80=93 Error code =E2=96=AA GHCBData[31:12] =E2=80=93 Reserved, must be zero Written by the hypervisor in response to a Page State Change request. A= ny non- zero value for the error code indicates that the page state change was = not successful. And if "all Fs" is indeed the error code, 'int ret' probably only works b= y luck since the return value is a 64-bit value, where as ret is a 32-bit signed= int. > + if (ret) > + ret =3D -1; Uh, this is fubar. You've created a shadow of 'ret', i.e. the outer ret= is likely uninitialized. > + > + set_ghcb_msr_bits(svm, ret, GHCB_MSR_PSC_ERROR_MASK, GHCB_MSR_PSC_ER= ROR_POS); > + set_ghcb_msr_bits(svm, 0, GHCB_MSR_PSC_RSVD_MASK, GHCB_MSR_PSC_RSVD_= POS); > + set_ghcb_msr_bits(svm, GHCB_MSR_PSC_RESP, GHCB_MSR_INFO_MASK, GHCB_M= SR_INFO_POS); > + break; > + } > case GHCB_MSR_TERM_REQ: { > u64 reason_set, reason_code; > =20 > --=20 > 2.17.1 >=20