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=-9.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham 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 3C115C4363D for ; Thu, 24 Sep 2020 18:13:27 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 82DD9208E4 for ; Thu, 24 Sep 2020 18:13:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="uuUj72Zo" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 82DD9208E4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=FeXtjAtIIwvEMckokIyAkXv1wV56PI+vg/sB9A4GMrs=; b=uuUj72ZoWrlGntum+0g+YtxRg OQZMDcxjJrCSPF3wBnvxEC2P/Xk01YJLm2NR1oElwp6V+i6MzikslwWcui84cP7JeBJWi17bsf5c3 QXMMyKnNZc66AlKqGi23sirf1nQS8n+7MKV9JQmUbC9U0Jteyl6Ku145yQ2p1S7DVjpjy195EzYtp oVc8zN75zektBJBJVF+AUYiCoU3vscHIVaD83w+zTHpHMKiG/I0Ca1eMlquRkVxb9KaYxV0Ufhn/j ChEC1b9GQIP8pTvwYgzS3Hy8KjiRyaBYEDxbuUu6xRYgxKTcrQ0yv2gvULckixfoLLpl1+5s2uKCm 9LQdxk6Uw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kLViY-0004AR-DP; Thu, 24 Sep 2020 18:11:42 +0000 Received: from mga09.intel.com ([134.134.136.24]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kLViV-00049y-3G for linux-arm-kernel@lists.infradead.org; Thu, 24 Sep 2020 18:11:40 +0000 IronPort-SDR: LbbaPR/zqN9DLVXoGEfGUMyEEL/YWxLRPBYtSvaskALkOy8Z04lb7KW6CWRTaKfMcLWkXha9+8 PfVEPgdYfkFw== X-IronPort-AV: E=McAfee;i="6000,8403,9754"; a="162202845" X-IronPort-AV: E=Sophos;i="5.77,298,1596524400"; d="scan'208";a="162202845" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Sep 2020 11:11:36 -0700 IronPort-SDR: myztV3dd0B7gegLeRVc9tNiKIbZZpy2YW/CWgwel9h01a1mDMLsbCF6vAfL8cbAjhB26HhiYs8 cEc4R+1PFBzg== X-IronPort-AV: E=Sophos;i="5.77,298,1596524400"; d="scan'208";a="336095887" Received: from sjchrist-coffee.jf.intel.com (HELO linux.intel.com) ([10.54.74.160]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Sep 2020 11:11:35 -0700 Date: Thu, 24 Sep 2020 11:11:34 -0700 From: Sean Christopherson To: Vitaly Kuznetsov Subject: Re: [RFC PATCH 3/3] KVM: x86: Use KVM_BUG/KVM_BUG_ON to handle bugs that are fatal to the VM Message-ID: <20200924181134.GB9649@linux.intel.com> References: <20200923224530.17735-1-sean.j.christopherson@intel.com> <20200923224530.17735-4-sean.j.christopherson@intel.com> <878scze4l5.fsf@vitty.brq.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <878scze4l5.fsf@vitty.brq.redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200924_141139_280645_8F615DED X-CRM114-Status: GOOD ( 28.58 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Cornelia Huck , Wanpeng Li , Janosch Frank , kvm@vger.kernel.org, Suzuki K Poulose , Marc Zyngier , Joerg Roedel , David Hildenbrand , linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, linux-mips@vger.kernel.org, Paul Mackerras , Christian Borntraeger , Aleksandar Markovic , James Morse , linux-arm-kernel@lists.infradead.org, Huacai Chen , Paolo Bonzini , Claudio Imbrenda , Julien Thierry , Jim Mattson Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 24, 2020 at 02:34:14PM +0200, Vitaly Kuznetsov wrote: > Sean Christopherson writes: > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 6f9a0c6d5dc5..810d46ab0a47 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -2250,7 +2250,7 @@ static void vmx_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg) > > vcpu->arch.cr4 |= vmcs_readl(GUEST_CR4) & guest_owned_bits; > > break; > > default: > > - WARN_ON_ONCE(1); > > + KVM_BUG_ON(1, vcpu->kvm); > > break; > > } > > } > > @@ -4960,6 +4960,7 @@ static int handle_cr(struct kvm_vcpu *vcpu) > > return kvm_complete_insn_gp(vcpu, err); > > case 3: > > WARN_ON_ONCE(enable_unrestricted_guest); > > + > > err = kvm_set_cr3(vcpu, val); > > return kvm_complete_insn_gp(vcpu, err); > > case 4: > > @@ -4985,14 +4986,13 @@ static int handle_cr(struct kvm_vcpu *vcpu) > > } > > break; > > case 2: /* clts */ > > - WARN_ONCE(1, "Guest should always own CR0.TS"); > > - vmx_set_cr0(vcpu, kvm_read_cr0_bits(vcpu, ~X86_CR0_TS)); > > - trace_kvm_cr_write(0, kvm_read_cr0(vcpu)); > > - return kvm_skip_emulated_instruction(vcpu); > > + KVM_BUG(1, vcpu->kvm, "Guest always owns CR0.TS"); > > + return -EIO; > > case 1: /*mov from cr*/ > > switch (cr) { > > case 3: > > WARN_ON_ONCE(enable_unrestricted_guest); > > + > > Here, were you intended to replace WARN_ON_ONCE() with KVM_BUG_ON() or > this is just a stray newline added? I think it's just a stray newline. At one point I had converted this to a KVM_BUG_ON(), but then reversed direction because it's not fatal to the guest, i.e. KVM should continue to function even though it's spuriously intercepting CR3 loads. Which, rereading this patch, completely contradicts the KVM_BUG() for CLTS. That's probably something we should sort out in this RFC: is KVM_BUG() only to be used if the bug is fatal/dangerous, or should it be used any time the error is definitely a KVM (or hardware) bug. > > val = kvm_read_cr3(vcpu); > > kvm_register_write(vcpu, reg, val); > > trace_kvm_cr_read(cr, val); > > @@ -5330,7 +5330,9 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu) > > > > static int handle_nmi_window(struct kvm_vcpu *vcpu) > > { > > - WARN_ON_ONCE(!enable_vnmi); > > + if (KVM_BUG_ON(!enable_vnmi, vcpu->kvm)) > > + return -EIO; > > + > > exec_controls_clearbit(to_vmx(vcpu), CPU_BASED_NMI_WINDOW_EXITING); > > ++vcpu->stat.nmi_window_exits; > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > @@ -5908,7 +5910,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath) > > * below) should never happen as that means we incorrectly allowed a > > * nested VM-Enter with an invalid vmcs12. > > */ > > - WARN_ON_ONCE(vmx->nested.nested_run_pending); > > + if (KVM_BUG_ON(vmx->nested.nested_run_pending, vcpu->kvm)) > > + return -EIO; > > > > /* If guest state is invalid, start emulating */ > > if (vmx->emulation_required) > > @@ -6258,7 +6261,9 @@ static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) > > int max_irr; > > bool max_irr_updated; > > > > - WARN_ON(!vcpu->arch.apicv_active); > > + if (KVM_BUG_ON(!vcpu->arch.apicv_active, vcpu->kvm)) > > + return -EIO; > > + > > if (pi_test_on(&vmx->pi_desc)) { > > pi_clear_on(&vmx->pi_desc); > > /* > > @@ -6345,7 +6350,7 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu) > > gate_desc *desc; > > u32 intr_info = vmx_get_intr_info(vcpu); > > > > - if (WARN_ONCE(!is_external_intr(intr_info), > > + if (KVM_BUG(!is_external_intr(intr_info), vcpu->kvm, > > "KVM: unexpected VM-Exit interrupt info: 0x%x", intr_info)) > > return; > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 17f4995e80a7..672eb5142b34 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -8363,6 +8363,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > bool req_immediate_exit = false; > > > > if (kvm_request_pending(vcpu)) { > > + if (kvm_check_request(KVM_REQ_VM_BUGGED, vcpu)) { > > Do we want to allow userspace to continue executing the guest or should > we make KVM_REQ_VM_BUGGED permanent by replacing kvm_check_request() > with kvm_test_request()? In theory, it should be impossible to reach this again as "r = -EIO" will bounce this out to userspace, the common checks to deny all ioctls() will prevent reinvoking KVM_RUN. > > + r = -EIO; > > + goto out; > > + } > > if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu)) { > > if (unlikely(!kvm_x86_ops.nested_ops->get_vmcs12_pages(vcpu))) { > > r = 0; > > -- > Vitaly > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel