Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date:   Tue, 4 Apr 2023 17:48:02 -0500
From:   Michael Roth <michael.roth@amd.com>
To:     Alexander Graf <graf@amazon.com>
CC:     <kvm@vger.kernel.org>, <linux-coco@lists.linux.dev>,
        <linux-mm@kvack.org>, <linux-crypto@vger.kernel.org>,
        <x86@kernel.org>, <linux-kernel@vger.kernel.org>,
        <tglx@linutronix.de>, <mingo@redhat.com>, <jroedel@suse.de>,
        <thomas.lendacky@amd.com>, <hpa@zytor.com>, <ardb@kernel.org>,
        <pbonzini@redhat.com>, <seanjc@google.com>, <vkuznets@redhat.com>,
        <jmattson@google.com>, <luto@kernel.org>,
        <dave.hansen@linux.intel.com>, <slp@redhat.com>,
        <pgonda@google.com>, <peterz@infradead.org>,
        <srinivas.pandruvada@linux.intel.com>, <rientjes@google.com>,
        <dovmurik@linux.ibm.com>, <tobin@ibm.com>, <bp@alien8.de>,
        <vbabka@suse.cz>, <kirill@shutemov.name>, <ak@linux.intel.com>,
        <tony.luck@intel.com>, <marcorr@google.com>,
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        <alpergun@google.com>, <dgilbert@redhat.com>, <jarkko@kernel.org>,
        <ashish.kalra@amd.com>, <nikunj.dadhania@amd.com>,
        Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [PATCH RFC v8 47/56] KVM: SVM: Support SEV-SNP AP Creation NAE
 event
Message-ID: <20230404224802.23fbe7kxa3h7tjps@amd.com>
References: <20230220183847.59159-1-michael.roth@amd.com>
 <20230220183847.59159-48-michael.roth@amd.com>
 <09696af0-b72d-29e7-862b-22ae4a630299@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <09696af0-b72d-29e7-862b-22ae4a630299@amazon.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2023 01:32:46.5797
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: eec5707b-1eb4-4223-1314-08db3575a90b
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT071.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB7094
X-Spam-Status: No, score=0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE autolearn=no
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On Fri, Feb 24, 2023 at 01:37:48PM +0100, Alexander Graf wrote:
> 
> On 20.02.23 19:38, Michael Roth wrote:
> > From: Tom Lendacky <thomas.lendacky@amd.com>
> > 
> > Add support for the SEV-SNP AP Creation NAE event. This allows SEV-SNP
> > guests to alter the register state of the APs on their own. This allows
> > the guest a way of simulating INIT-SIPI.
> > 
> > A new event, KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, is created and used
> > so as to avoid updating the VMSA pointer while the vCPU is running.
> > 
> > For CREATE
> >    The guest supplies the GPA of the VMSA to be used for the vCPU with
> >    the specified APIC ID. The GPA is saved in the svm struct of the
> >    target vCPU, the KVM_REQ_UPDATE_PROTECTED_GUEST_STATE event is added
> >    to the vCPU and then the vCPU is kicked.
> > 
> > For CREATE_ON_INIT:
> >    The guest supplies the GPA of the VMSA to be used for the vCPU with
> >    the specified APIC ID the next time an INIT is performed. The GPA is
> >    saved in the svm struct of the target vCPU.
> > 
> > For DESTROY:
> >    The guest indicates it wishes to stop the vCPU. The GPA is cleared
> >    from the svm struct, the KVM_REQ_UPDATE_PROTECTED_GUEST_STATE event is
> >    added to vCPU and then the vCPU is kicked.
> > 
> > The KVM_REQ_UPDATE_PROTECTED_GUEST_STATE event handler will be invoked
> > as a result of the event or as a result of an INIT. The handler sets the
> > vCPU to the KVM_MP_STATE_UNINITIALIZED state, so that any errors will
> > leave the vCPU as not runnable. Any previous VMSA pages that were
> > installed as part of an SEV-SNP AP Creation NAE event are un-pinned. If
> > a new VMSA is to be installed, the VMSA guest page is pinned and set as
> > the VMSA in the vCPU VMCB and the vCPU state is set to
> > KVM_MP_STATE_RUNNABLE. If a new VMSA is not to be installed, the VMSA is
> > cleared in the vCPU VMCB and the vCPU state is left as
> > KVM_MP_STATE_UNINITIALIZED to prevent it from being run.
> > 
> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > [mdr: add handling for restrictedmem]
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> 
> 
> What is the intended boot sequence for SEV-SNP guests? FWIW with this
> interface in place, guests will typically use in-guest VMSA pages to hold
> secondary vcpu state. But that means we're now allocating 4kb of memory for
> every vcpu that we create that will be for most of the guest's lifetime
> superfluous.
> 
> Wouldn't it make more sense to have a model where we only allocate the VMSA
> for the boot CPU and leave secondary allocation to the guest? We already
> need firmware changes for SEV-SNP - may as well make this one more.

I don't think we'd necessarily need a firmware change. We could just
free original VMSA back to the hypervisor as soon as those APs come
online. The down-side to that versus deferring cleaning till guest
shutdown is there is some flushing activity (see:
sev_flush_encrypted_page()) that would now likely be occuring during
guest boot up where the overhead might be more noticeable. But for SNP
the host likely supports X86_FEATURE_SME_COHERENT so the overhead
probably isn't that bad.

> 
> [...]
> 
> > +
> > +static int sev_snp_ap_creation(struct vcpu_svm *svm)
> > +{
> > +       struct kvm_sev_info *sev = &to_kvm_svm(svm->vcpu.kvm)->sev_info;
> > +       struct kvm_vcpu *vcpu = &svm->vcpu;
> > +       struct kvm_vcpu *target_vcpu;
> > +       struct vcpu_svm *target_svm;
> > +       unsigned int request;
> > +       unsigned int apic_id;
> > +       bool kick;
> > +       int ret;
> > +
> > +       request = lower_32_bits(svm->vmcb->control.exit_info_1);
> > +       apic_id = upper_32_bits(svm->vmcb->control.exit_info_1);
> > +
> > +       /* Validate the APIC ID */
> > +       target_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, apic_id);
> 
> 
> Out of curiosity: The target CPU can be my own vCPU, right?

I don't think that would be the normal behavior, but maybe with some
care it's possible for a guest to do things that way. I haven't seen
anything strictly prohibiting this in the relevant specs.

> 
> 
> > +       if (!target_vcpu) {
> > +               vcpu_unimpl(vcpu, "vmgexit: invalid AP APIC ID [%#x] from guest\n",
> > +                           apic_id);
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = 0;
> > +
> > +       target_svm = to_svm(target_vcpu);
> > +
> > +       /*
> > +        * The target vCPU is valid, so the vCPU will be kicked unless the
> > +        * request is for CREATE_ON_INIT. For any errors at this stage, the
> > +        * kick will place the vCPU in an non-runnable state.
> > +        */
> > +       kick = true;
> > +
> > +       mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
> > +
> > +       target_svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
> > +       target_svm->sev_es.snp_ap_create = true;
> > +
> > +       /* 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_FEAT_INT_INJ_MODES) {
> > +                       vcpu_unimpl(vcpu, "vmgexit: invalid AP injection mode [%#lx] from guest\n",
> > +                                   vcpu->arch.regs[VCPU_REGS_RAX]);
> > +                       ret = -EINVAL;
> > +                       goto out;
> > +               }
> > +       }
> > +
> > +       switch (request) {
> > +       case SVM_VMGEXIT_AP_CREATE_ON_INIT:
> > +               kick = false;
> > +               fallthrough;
> > +       case SVM_VMGEXIT_AP_CREATE:
> > +               if (!page_address_valid(vcpu, svm->vmcb->control.exit_info_2)) {
> > +                       vcpu_unimpl(vcpu, "vmgexit: invalid AP VMSA address [%#llx] from guest\n",
> > +                                   svm->vmcb->control.exit_info_2);
> > +                       ret = -EINVAL;
> > +                       goto out;
> > +               }
> > +
> > +               /*
> > +                * Malicious guest can RMPADJUST a large page into VMSA which
> > +                * will hit the SNP erratum where the CPU will incorrectly signal
> > +                * an RMP violation #PF if a hugepage collides with the RMP entry
> > +                * of VMSA page, reject the AP CREATE request if VMSA address from
> > +                * guest is 2M aligned.
> 
> 
> This will break genuine current Linux kernels that just happen to allocate a
> guest page, no? In fact, given enough vCPUs you're almost guaranteed to hit
> an aligned structure somewhere. What is the guest supposed to do in that
> situation?

The initial SNP support for guest kernels already made use of
snp_alloc_vmsa_page() to do the appropriate workaround to avoid allocating
2MB-aligned VMSA pages.

-Mike

> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
>