| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YoZrG3n5fgMp4LQl@google.com>
Date: Thu, 19 May 2022 16:06:51 +0000
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: kvm@...r.kernel.org, Wanpeng Li <wanpengli@...cent.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Zhenyu Wang <zhenyuw@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Tom Lendacky <thomas.lendacky@....com>,
Ingo Molnar <mingo@...hat.com>,
David Airlie <airlied@...ux.ie>,
Thomas Gleixner <tglx@...utronix.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
intel-gfx@...ts.freedesktop.org, Daniel Vetter <daniel@...ll.ch>,
Borislav Petkov <bp@...en8.de>, Joerg Roedel <joro@...tes.org>,
linux-kernel@...r.kernel.org, Jim Mattson <jmattson@...gle.com>,
Zhi Wang <zhi.a.wang@...el.com>,
Brijesh Singh <brijesh.singh@....com>,
"H. Peter Anvin" <hpa@...or.com>,
intel-gvt-dev@...ts.freedesktop.org,
dri-devel@...ts.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.
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 <mlevitsk@...hat.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 */
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.
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.
> +{
> + 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.
> + 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.
> + 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.
> kvm_apic_set_xapic_id(apic, val >> 24);
> - else
> + kvm_lapic_check_initial_apic_id(apic);
> + } else
Needs curly braces for both paths.
> 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
--
Powered by blists - more mailing lists