Received: by 2002:a05:6602:18e:0:0:0:0 with SMTP id m14csp1646439ioo; Sun, 22 May 2022 22:53:03 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwAGebHshC4ZgDs0t/7itqg3k0u2pejkFtUIBGBE9g9u1satUuB4+RB3qMXTJg6Qe/ppbD/ X-Received: by 2002:a17:90b:1b41:b0:1e0:17f:d17 with SMTP id nv1-20020a17090b1b4100b001e0017f0d17mr15210284pjb.85.1653285183741; Sun, 22 May 2022 22:53:03 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1653285183; cv=none; d=google.com; s=arc-20160816; b=08M+wXgLPimfIOjElZXReA6MH/CGFPmqbyRdfCNpQ9mX9GB4Bp4UQuTdbwXXmdMzF0 aAqr7XTVuIJd+XJWzSMTvnkuD9FQ7p9l3ix9veWfjpIQpCQckxGFWLzrm3LwzJr4F+M/ vsO0dzj+u/EwRTDg3s+dioHXAl2PzaySHNWAqOjHIlPUc08eTJKskK/WGiHDVApwUWWB DBUCBYseHwCBt3qrL8CTRDJaWTARWxU5sCXn64LkP6JxLwtpMNYfu94GIBsksHRjGmza bN7LkZdaB8YtKAmJgXmf5w+eBEAaUfQTVBOZDEROYfldWzP4C0otBRNyQwyOk7JG9rD7 z0sA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:date:cc:to:from:subject :message-id:dkim-signature; bh=iZk3PNgU+m5QGZbYbM3Q/AYoB1xcu8VqIfJcE4Df1D8=; b=kEuf5ScWg5AP0vebm//1MRKLJ/eI8qpOeo2Er7H6LrrDD/HEdcFZF+eUERJxo4UFVF eL1uRQYTDLYGSzypWj007KmWWsHmylFg0PR19rZLr0Wb77PO/dNDtdoVNBhQ5hz1RHoq B307bkG/UD7ueOVZIVpK31BJQLxykUd3Sd9GW9qqh5aT77H1CJKmcbosyu9aQ7g8YV37 BWF9iDLrsxXUvgVIKKTjYrqTrod/6cw5c00j/WrjkX/WgTkeZHWzCeNYWmTMm8SjCqnC vK2h/ievT3Iorh92QE+jRAkfLTo9WuBNx7b6F+MR3QE6VElmlC/UMOk1Ncs5n5hg2J7z TFcg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RACHC0nP; spf=softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net. [23.128.96.19]) by mx.google.com with ESMTPS id k71-20020a63844a000000b003c281e1fd7csi8734574pgd.214.2022.05.22.22.53.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 22 May 2022 22:53:03 -0700 (PDT) Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RACHC0nP; spf=softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 6B152DE9F; Sun, 22 May 2022 22:50:43 -0700 (PDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242602AbiEVJD1 (ORCPT + 99 others); Sun, 22 May 2022 05:03:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54352 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240935AbiEVJDX (ORCPT ); Sun, 22 May 2022 05:03:23 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 8914F1CB14 for ; Sun, 22 May 2022 02:03:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1653210201; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=iZk3PNgU+m5QGZbYbM3Q/AYoB1xcu8VqIfJcE4Df1D8=; b=RACHC0nP6DPi3/uyZgDoCdVipe1XEQ/ClFRxrYiRl1+N4BcoUmDdLSyypeudkzVMkB307c smhqF+KEjtUo0leyUKNkquQBnoeKzv8RqX9ku3lj53kjvoLPBFop/Kg8U3U9qd5XuJqMdV mFuiQhesixax4tZ1QDSZglW2nlL+Qlk= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-619-lDNHqC-NPg6GqFVGjvEqjQ-1; Sun, 22 May 2022 05:03:18 -0400 X-MC-Unique: lDNHqC-NPg6GqFVGjvEqjQ-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 29ECD811E7A; Sun, 22 May 2022 09:03:17 +0000 (UTC) Received: from starship (unknown [10.40.192.55]) by smtp.corp.redhat.com (Postfix) with ESMTP id 97593492C14; Sun, 22 May 2022 09:03:11 +0000 (UTC) Message-ID: Subject: Re: [RFC PATCH v3 02/19] KVM: x86: inhibit APICv/AVIC when the guest and/or host changes apic id/base from the defaults. From: Maxim Levitsky To: Sean Christopherson Cc: kvm@vger.kernel.org, Wanpeng Li , Vitaly Kuznetsov , Jani Nikula , Paolo Bonzini , Tvrtko Ursulin , Rodrigo Vivi , Zhenyu Wang , Joonas Lahtinen , Tom Lendacky , Ingo Molnar , David Airlie , Thomas Gleixner , Dave Hansen , x86@kernel.org, intel-gfx@lists.freedesktop.org, Daniel Vetter , Borislav Petkov , Joerg Roedel , linux-kernel@vger.kernel.org, Jim Mattson , Zhi Wang , Brijesh Singh , "H. Peter Anvin" , intel-gvt-dev@lists.freedesktop.org, dri-devel@lists.freedesktop.org Date: Sun, 22 May 2022 12:03:10 +0300 In-Reply-To: References: <20220427200314.276673-1-mlevitsk@redhat.com> <20220427200314.276673-3-mlevitsk@redhat.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.85 on 10.11.54.10 X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE autolearn=unavailable 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: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2022-05-19 at 16:06 +0000, Sean Christopherson wrote: > On Wed, Apr 27, 2022, Maxim Levitsky wrote: > > Neither of these settings should be changed by the guest and it is > > a burden to support it in the acceleration code, so just inhibit > > it instead. > > > > Also add a boolean 'apic_id_changed' to indicate if apic id ever changed. > > > > Signed-off-by: Maxim Levitsky > > --- > > arch/x86/include/asm/kvm_host.h | 3 +++ > > arch/x86/kvm/lapic.c | 25 ++++++++++++++++++++++--- > > arch/x86/kvm/lapic.h | 8 ++++++++ > > 3 files changed, 33 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 63eae00625bda..636df87542555 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -1070,6 +1070,8 @@ enum kvm_apicv_inhibit { > > APICV_INHIBIT_REASON_ABSENT, > > /* AVIC is disabled because SEV doesn't support it */ > > APICV_INHIBIT_REASON_SEV, > > + /* APIC ID and/or APIC base was changed by the guest */ > > I don't see any reason to inhibit APICv if the APIC base is changed. KVM has > never supported that, and disabling APICv won't "fix" anything. I kind of tacked the APIC base on the thing just to be a good citezen. In theory currently if the guest changes the APIC base, neither APICv nor AVIC will even notice, so the guest will still be able to access the default APIC base and the new APIC base, which is kind of wrong. Inhibiting APICv/AVIC in this case makes it better and it is very cheap to do. If you still think that it shouln't be done, I'll remove it. > > Ignoring that is a minor simplification, but also allows for a more intuitive > name, e.g. > > APICV_INHIBIT_REASON_APIC_ID_MODIFIED, > > The inhibit also needs to be added avic_check_apicv_inhibit_reasons() and > vmx_check_apicv_inhibit_reasons(). > > > + APICV_INHIBIT_REASON_RO_SETTINGS, > > }; > > > > struct kvm_arch { > > @@ -1258,6 +1260,7 @@ struct kvm_arch { > > hpa_t hv_root_tdp; > > spinlock_t hv_root_tdp_lock; > > #endif > > + bool apic_id_changed; > > }; > > > > struct kvm_vm_stat { > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 66b0eb0bda94e..8996675b3ef4c 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -2038,6 +2038,19 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val) > > } > > } > > > > +static void kvm_lapic_check_initial_apic_id(struct kvm_lapic *apic) > > The "check" part is misleading/confusing. "check" helpers usually query and return > state. I assume you avoided "changed" because the ID may or may not actually be > changing. Maybe kvm_apic_id_updated()? Ah, better idea. What about > kvm_lapic_xapic_id_updated()? See below for reasoning. This is a very good idea! > > > +{ > > + if (kvm_apic_has_initial_apic_id(apic)) > > Rather than add a single-use helper, invoke the helper from kvm_apic_state_fixup() > in the !x2APIC path, then this can KVM_BUG_ON() x2APIC to help document that KVM > should never allow the ID to change for x2APIC. yes, but we do allow non default x2apic id via userspace api - I wasn't able to convience you to remove this :) > > > + return; > > + > > + pr_warn_once("APIC ID change is unsupported by KVM"); > > It's supported (modulo x2APIC shenanigans), otherwise KVM wouldn't need to disable > APICv. Here, as I said, it would be nice to see that warning if someone complains. Fact is that AVIC code was totally broken in this regard, and there are probably more, so it would be nice to see if anybody complains. If you insist, I'll remove this warning. > > > + kvm_set_apicv_inhibit(apic->vcpu->kvm, > > + APICV_INHIBIT_REASON_RO_SETTINGS); > > + > > + apic->vcpu->kvm->arch.apic_id_changed = true; > > +} > > + > > static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > > { > > int ret = 0; > > @@ -2046,9 +2059,11 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > > > > switch (reg) { > > case APIC_ID: /* Local APIC ID */ > > - if (!apic_x2apic_mode(apic)) > > + if (!apic_x2apic_mode(apic)) { > > + > > Spurious newline. Will fix. > > > kvm_apic_set_xapic_id(apic, val >> 24); > > - else > > + kvm_lapic_check_initial_apic_id(apic); > > + } else > > Needs curly braces for both paths. Will fix. > > > ret = 1; > > break; > > > > E.g. > > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/lapic.c | 21 +++++++++++++++++++-- > arch/x86/kvm/svm/avic.c | 3 ++- > arch/x86/kvm/vmx/vmx.c | 3 ++- > 4 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index d895d25c5b2f..d888fa1bae77 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1071,6 +1071,7 @@ enum kvm_apicv_inhibit { > APICV_INHIBIT_REASON_BLOCKIRQ, > APICV_INHIBIT_REASON_ABSENT, > APICV_INHIBIT_REASON_SEV, > + APICV_INHIBIT_REASON_APIC_ID_MODIFIED, > }; > > struct kvm_arch { > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 5fd678c90288..6fe8f20f03d8 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -2039,6 +2039,19 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val) > } > } > > +static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic) > +{ > + struct kvm *kvm = apic->vcpu->kvm; > + > + if (KVM_BUG_ON(apic_x2apic_mode(apic), kvm)) > + return; > + > + if (kvm_xapic_id(apic) == apic->vcpu->vcpu_id) > + return; > + > + kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED); > +} > + > static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > { > int ret = 0; > @@ -2047,10 +2060,12 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > > switch (reg) { > case APIC_ID: /* Local APIC ID */ > - if (!apic_x2apic_mode(apic)) > + if (!apic_x2apic_mode(apic)) { > kvm_apic_set_xapic_id(apic, val >> 24); > - else > + kvm_lapic_xapic_id_updated(apic); > + } else { > ret = 1; > + } > break; > > case APIC_TASKPRI: > @@ -2665,6 +2680,8 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, > icr = __kvm_lapic_get_reg64(s->regs, APIC_ICR); > __kvm_lapic_set_reg(s->regs, APIC_ICR2, icr >> 32); > } > + } else { > + kvm_lapic_xapic_id_updated(vcpu->arch.apic); > } > > return 0; > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > index 54fe03714f8a..239c3e8b1f3f 100644 > --- a/arch/x86/kvm/svm/avic.c > +++ b/arch/x86/kvm/svm/avic.c > @@ -910,7 +910,8 @@ bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason) > BIT(APICV_INHIBIT_REASON_PIT_REINJ) | > BIT(APICV_INHIBIT_REASON_X2APIC) | > BIT(APICV_INHIBIT_REASON_BLOCKIRQ) | > - BIT(APICV_INHIBIT_REASON_SEV); > + BIT(APICV_INHIBIT_REASON_SEV) | > + BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED); > > return supported & BIT(reason); > } > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index b06eafa5884d..941adade21ea 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -7818,7 +7818,8 @@ static bool vmx_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason) > ulong supported = BIT(APICV_INHIBIT_REASON_DISABLE) | > BIT(APICV_INHIBIT_REASON_ABSENT) | > BIT(APICV_INHIBIT_REASON_HYPERV) | > - BIT(APICV_INHIBIT_REASON_BLOCKIRQ); > + BIT(APICV_INHIBIT_REASON_BLOCKIRQ) | > + BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED); > > return supported & BIT(reason); > } > > base-commit: 6ab6e3842d18e4529fa524fb6c668ae8a8bf54f4 Best regards, Thanks for the review, Maxim Levitsky > -- >