From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f202.google.com (mail-yb1-f202.google.com [209.85.219.202]) (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 1F1055102B for ; Mon, 8 Jan 2024 15:53:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="Q2a8cxQl" Received: by mail-yb1-f202.google.com with SMTP id 3f1490d57ef6-dbed7ba6545so2247007276.1 for ; Mon, 08 Jan 2024 07:53:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1704729184; x=1705333984; darn=lists.linux.dev; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=LXpfvDdR25mi61ZRv+hUGYfziJrbv6+q0uR9W3pdtgw=; b=Q2a8cxQl+Jp3G6KgwpoKQ66HsbWHjJ7PgsN05bBDcCZb1Ug6PAYmvmFHR+35GyDlBJ sJLFdxYcwQrQVQbzeb0jw8nzBXcyXQcmR98sAANJMzbVh6JH8Vd2UXHus67gEyDBUs1f hkDo9BoVbw7jpOnaagGGkmVjVouqxKUEP7CU0oRr6l2dyhSpnuj6CmpHhYtOPEO96R5X CeyeTrXAqXO7iMkQDsGS+Jww+D+yT/xqBmSLT27qd2X8Tp+vyqhe8AB6u2Fyts5/PDvy kSBwW2Ie+YXmXYz0+STZJZJXtso7kWx6/TJIZi85DVOcvORWMJzd5sd/3v9mR4Ht+93C uVPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704729184; x=1705333984; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=LXpfvDdR25mi61ZRv+hUGYfziJrbv6+q0uR9W3pdtgw=; b=xI95ADAwA8FkiR41UirKQbBPHit8/OZ+JGhj1cuwUqsc7iBZ5zzhl8J/NZmcAiEvdt K6y7TwEFdojnmAL/9ja8fTZqy19HVTe7tDR4PEIev8Ws6XZau3iZMFeVVTDuxT1MiZTM RV6POOggzDZR+6tM6L7fJ2xGWzHvDXglrrSGuH/7W/lPwRSFtgBOZwzIvHQYQJO3BNdu dHqoSDJoCyNJCpUhxOfLc8bmwwgnr2Z76ul8ZMzQk3Cx28Akp6LOYhLDjD+xcw1tgzSr QfRDyRNYujzfN8kKkyySPdYNj+saQYXcIE6Fdu4+/3HSHPQwbmSkBArhc3OuXad1RAt+ +SwQ== X-Gm-Message-State: AOJu0Yw4GQ5dLTurLubimiJfOUu1WHj7eiZc/uTgRv5HqN/uOPJBSd0o SypbUSez9RZL1JLYXU4XdHKfmzoeRGyeK2sDOw== X-Google-Smtp-Source: AGHT+IH6tgug6GZXuthKAIt2m3rWLQls/sRxvnIhe9rTO176GAtStoSNma4aHNycx8stxCAgF2wzux11TeA= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a25:83d0:0:b0:dbe:a20a:6330 with SMTP id v16-20020a2583d0000000b00dbea20a6330mr1497318ybm.9.1704729184054; Mon, 08 Jan 2024 07:53:04 -0800 (PST) Date: Mon, 8 Jan 2024 07:53:02 -0800 In-Reply-To: Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20231230172351.574091-1-michael.roth@amd.com> <20231230172351.574091-27-michael.roth@amd.com> Message-ID: Subject: Re: [PATCH v11 26/35] KVM: SEV: Support SEV-SNP AP Creation NAE event From: Sean Christopherson To: Jacob Xu Cc: Michael Roth , 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, 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, sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com, jarkko@kernel.org, ashish.kalra@amd.com, nikunj.dadhania@amd.com, pankaj.gupta@amd.com, liam.merwick@oracle.com, zhi.a.wang@intel.com, Brijesh Singh , Adam Dunlap Content-Type: text/plain; charset="us-ascii" On Fri, Jan 05, 2024, Jacob Xu wrote: > > + if (kick) { > > + if (target_vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED) > > + target_vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE; > > + > > + kvm_make_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, target_vcpu); > > I think we should switch the order of these two statements for > setting mp_state and for making the request for > KVM_REQ_UPDATE_PROTECTED_GUEST_STATE. > There is a race condition I observed when booting with SVSM where: > 1. BSP sets target vcpu to KVM_MP_STATE_RUNNABLE > 2. AP thread within the loop of arch/x86/kvm.c:vcpu_run() checks > vm_vcpu_running() > 3. AP enters the guest without having updated the VMSA state from > KVM_REQ_UPDATE_PROTECTED_GUEST_STATE > > This results in the AP executing on a bad RIP and then crashing. > If we set the request first, then we avoid the race condition. That just introducs a different race, e.g. if this task gets delayed and the target vCPU processes KVM_REQ_UPDATE_PROTECTED_GUEST_STATE before its marked RUNNABLE, then the target vCPU could end up stuck in the UNINITIALIZED loop. Reading and writing arch.mp_state across vCPUs is simply not safe. There's a reason why KVM atomically manages INITs and SIPIs and only modifies mp_state when processing events on the target vCPU. > > + kvm_vcpu_kick(target_vcpu); ... > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 87b78d63e81d..df9ec357d538 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -10858,6 +10858,14 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > > > if (kvm_check_request(KVM_REQ_UPDATE_CPU_DIRTY_LOGGING, vcpu)) > > static_call(kvm_x86_update_cpu_dirty_logging)(vcpu); > > + > > + if (kvm_check_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, vcpu)) { > > + kvm_vcpu_reset(vcpu, true); > > + if (vcpu->arch.mp_state != KVM_MP_STATE_RUNNABLE) { > > + r = 1; > > + goto out; > > + } > > + } > > } > > > > if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win || > > @@ -13072,6 +13080,9 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) > > if (kvm_test_request(KVM_REQ_PMI, vcpu)) > > return true; > > > > + if (kvm_test_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, vcpu)) > > + return true; > > + > > if (kvm_arch_interrupt_allowed(vcpu) && > > (kvm_cpu_has_interrupt(vcpu) || > > kvm_guest_apic_has_interrupt(vcpu))) > > -- > > 2.25.1 > > > >