Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 18 May 2022 16:28:17 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     kvm@vger.kernel.org, Wanpeng Li <wanpengli@tencent.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Zhenyu Wang <zhenyuw@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Ingo Molnar <mingo@redhat.com>,
        David Airlie <airlied@linux.ie>,
        Thomas Gleixner <tglx@linutronix.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        intel-gfx@lists.freedesktop.org,
        Sean Christopherson <seanjc@google.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Borislav Petkov <bp@alien8.de>, Joerg Roedel <joro@8bytes.org>,
        linux-kernel@vger.kernel.org, Jim Mattson <jmattson@google.com>,
        Zhi Wang <zhi.a.wang@intel.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        intel-gvt-dev@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org
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.
Message-ID: <20220518082811.GA8765@gao-cwp>
References: <20220427200314.276673-1-mlevitsk@redhat.com>
 <20220427200314.276673-3-mlevitsk@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220427200314.276673-3-mlevitsk@redhat.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Wed, Apr 27, 2022 at 11:02:57PM +0300, 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 <mlevitsk@redhat.com>
>---
> 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 */
>+	APICV_INHIBIT_REASON_RO_SETTINGS,

You need to add it to check_apicv_inhibit_reasons as well.

> };
> 
> 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;

What's the value of this boolean? No one reads it.

> };
> 
> 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)
>+{
>+	if (kvm_apic_has_initial_apic_id(apic))
>+		return;
>+
>+	pr_warn_once("APIC ID change is unsupported by KVM");

It is misleading because changing xAPIC ID is supported by KVM; it just
isn't compatible with APICv. Probably this pr_warn_once() should be
removed.

>+
>+	kvm_set_apicv_inhibit(apic->vcpu->kvm,
>+			APICV_INHIBIT_REASON_RO_SETTINGS);

The indentation here looks incorrect to me.
	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)) {
>+
> 			kvm_apic_set_xapic_id(apic, val >> 24);
>-		else
>+			kvm_lapic_check_initial_apic_id(apic);
>+		} else
> 			ret = 1;
> 		break;
> 
>@@ -2335,8 +2350,11 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> 			     MSR_IA32_APICBASE_BASE;
> 
> 	if ((value & MSR_IA32_APICBASE_ENABLE) &&
>-	     apic->base_address != APIC_DEFAULT_PHYS_BASE)
>+	     apic->base_address != APIC_DEFAULT_PHYS_BASE) {
>+		kvm_set_apicv_inhibit(apic->vcpu->kvm,
>+				APICV_INHIBIT_REASON_RO_SETTINGS);
> 		pr_warn_once("APIC base relocation is unsupported by KVM");
>+	}
> }
> 
> void kvm_apic_update_apicv(struct kvm_vcpu *vcpu)
>@@ -2649,6 +2667,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
> 		}
> 	}
> 
>+	kvm_lapic_check_initial_apic_id(vcpu->arch.apic);
> 	return 0;
> }
> 
>diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>index 4e4f8a22754f9..b9c406d383080 100644
>--- a/arch/x86/kvm/lapic.h
>+++ b/arch/x86/kvm/lapic.h
>@@ -252,4 +252,12 @@ static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
> 	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
> }
> 
>+static inline bool kvm_apic_has_initial_apic_id(struct kvm_lapic *apic)
>+{
>+	if (apic_x2apic_mode(apic))
>+		return true;

I suggest warning of x2apic mode:
	if (WARN_ON_ONCE(apic_x2apic_mode(apic)))

Because it is weird that callers care about initial apic id when apic is
in x2apic mode.