The first half or so patches fix semi-urgent, real-world relevant APICv
and AVIC bugs.
The second half fixes a variety of AVIC and optimized APIC map bugs
where KVM doesn't play nice with various edge cases that are
architecturally legal(ish), but are unlikely to occur in most real world
scenarios
I have tested this heavily with KUT, but I haven't booted Windows and
don't have access to x2AVIC, so as usual, additional testing would be
much appreciated.
v4:
- Fix more bugs! [Alejandro]
- Delete APIC memslot to inhibit xAVIC acceleration when x2APIC is
enabled on AMD/SVM instead of using a "partial" inihbit. [Maxim]
v3:
- https://lore.kernel.org/all/[email protected]
- Collect reviews. [Paolo]
- Drop "partial" x2APIC inhibit and instead delete the memslot.
[Maxim, Suravee]
- Skip logical mode updates for x2APIC, which just reuses the
phys_map with some clever logic. [Suravee]
- Add a fix for "nodecode write" traps. [Alejandro]
v2:
- https://lore.kernel.org/all/[email protected]
- Collect reviews. [Li, Maxim]
- Disable only MMIO access when x2APIC is enabled (instead of disabling
all of AVIC). [Maxim]
- Inhibit AVIC when logical IDs are aliased. [Maxim]
- Tweak name of set_virtual_apic_mode() hook. [Maxim]
- Straight up revert logical ID fastpath mess. [Maxim]
- Reword changelog about skipping vCPU during logical setup. [Maxim]
- Fix LDR updates on AVIC. [Maxim?]
- Fix a nasty ISR caching bug.
- Flush TLB when activating AVIC.
v1: https://lore.kernel.org/all/[email protected]
Sean Christopherson (31):
KVM: x86: Blindly get current x2APIC reg value on "nodecode write"
traps
KVM: x86: Purge "highest ISR" cache when updating APICv state
KVM: SVM: Flush the "current" TLB when activating AVIC
KVM: SVM: Process ICR on AVIC IPI delivery failure due to invalid
target
KVM: x86: Don't inhibit APICv/AVIC on xAPIC ID "change" if APIC is
disabled
KVM: x86: Track xAPIC ID only on userspace SET, _after_ vAPIC is
updated
KVM: x86: Don't inhibit APICv/AVIC if xAPIC ID mismatch is due to
32-bit ID
KVM: SVM: Don't put/load AVIC when setting virtual APIC mode
KVM: x86: Handle APICv updates for APIC "mode" changes via request
KVM: x86: Move APIC access page helper to common x86 code
KVM: x86: Inhibit APIC memslot if x2APIC and AVIC are enabled
KVM: SVM: Replace "avic_mode" enum with "x2avic_enabled" boolean
KVM: SVM: Compute dest based on sender's x2APIC status for AVIC kick
Revert "KVM: SVM: Use target APIC ID to complete x2AVIC IRQs when
possible"
KVM: SVM: Document that vCPU ID == APIC ID in AVIC kick fastpatch
KVM: SVM: Add helper to perform final AVIC "kick" of single vCPU
KVM: x86: Explicitly skip optimized logical map setup if vCPU's LDR==0
KVM: x86: Explicitly track all possibilities for APIC map's logical
modes
KVM: x86: Skip redundant x2APIC logical mode optimized cluster setup
KVM: x86: Disable APIC logical map if logical ID covers multiple MDAs
KVM: x86: Disable APIC logical map if vCPUs are aliased in logical
mode
KVM: x86: Honor architectural behavior for aliased 8-bit APIC IDs
KVM: x86: Inhibit APICv/AVIC if the optimized physical map is disabled
KVM: SVM: Inhibit AVIC if vCPUs are aliased in logical mode
KVM: SVM: Always update local APIC on writes to logical dest register
KVM: SVM: Update svm->ldr_reg cache even if LDR is "bad"
KVM: SVM: Require logical ID to be power-of-2 for AVIC entry
KVM: SVM: Handle multiple logical targets in AVIC kick fastpath
KVM: SVM: Ignore writes to Remote Read Data on AVIC write traps
Revert "KVM: SVM: Do not throw warning when calling avic_vcpu_load on
a running vcpu"
KVM: x86: Track required APICv inhibits with variable, not callback
Suravee Suthikulpanit (1):
KVM: SVM: Fix x2APIC Logical ID calculation for
avic_kick_target_vcpus_fast
Documentation/virt/kvm/x86/errata.rst | 11 +
arch/x86/include/asm/kvm-x86-ops.h | 1 -
arch/x86/include/asm/kvm_host.h | 55 +++-
arch/x86/kvm/lapic.c | 238 +++++++++++++---
arch/x86/kvm/lapic.h | 2 +
arch/x86/kvm/svm/avic.c | 375 ++++++++++++--------------
arch/x86/kvm/svm/nested.c | 2 +-
arch/x86/kvm/svm/svm.c | 6 +-
arch/x86/kvm/svm/svm.h | 28 +-
arch/x86/kvm/vmx/vmx.c | 58 +---
arch/x86/kvm/x86.c | 23 +-
11 files changed, 477 insertions(+), 322 deletions(-)
base-commit: c59fb127583869350256656b7ed848c398bef879
--
2.38.0.rc1.362.ged0d419d3c-goog
Inhibit APICv/AVIC if the optimized physical map is disabled so that KVM
KVM provides consistent APIC behavior if xAPIC IDs are aliased due to
vcpu_id being truncated and the x2APIC hotplug hack isn't enabled. If
the hotplug hack is disabled, events that are emulated by KVM will follow
architectural behavior (all matching vCPUs receive events, even if the
"match" is due to truncation), whereas APICv and AVIC will deliver events
only to the first matching vCPU, i.e. the vCPU that matches without
truncation.
Note, the "extra" inhibit is needed because KVM deliberately ignores
mismatches due to truncation when applying the APIC_ID_MODIFIED inhibit
so that large VMs (>255 vCPUs) can run with APICv/AVIC.
Fixes: TDB
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 6 ++++++
arch/x86/kvm/lapic.c | 13 ++++++++++++-
arch/x86/kvm/svm/avic.c | 1 +
arch/x86/kvm/vmx/vmx.c | 1 +
4 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ac28bbfbf0e3..171e38b94c89 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1104,6 +1104,12 @@ enum kvm_apicv_inhibit {
*/
APICV_INHIBIT_REASON_BLOCKIRQ,
+ /*
+ * APICv is disabled because not all vCPUs have a 1:1 mapping between
+ * APIC ID and vCPU, _and_ KVM is not applying its x2APIC hotplug hack.
+ */
+ APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED,
+
/*
* For simplicity, the APIC acceleration is inhibited
* first time either APIC ID or APIC base are changed by the guest
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 340c2d3e781b..f6f328d36ae2 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -381,6 +381,16 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
cluster[ldr] = apic;
}
out:
+ /*
+ * The optimized map is effectively KVM's internal version of APICv,
+ * and all unwanted aliasing that results in disabling the optimized
+ * map also applies to APICv.
+ */
+ if (!new)
+ kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
+ else
+ kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
+
old = rcu_dereference_protected(kvm->arch.apic_map,
lockdep_is_held(&kvm->arch.apic_map_lock));
rcu_assign_pointer(kvm->arch.apic_map, new);
@@ -2153,7 +2163,8 @@ static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
/*
* Deliberately truncate the vCPU ID when detecting a modified APIC ID
* to avoid false positives if the vCPU ID, i.e. x2APIC ID, is a 32-bit
- * value.
+ * value. If the wrap/truncation results in unwatned aliasing, APICv
+ * will be inhibited as part of updating KVM's optimized APIC maps.
*/
if (kvm_xapic_id(apic) == (u8)apic->vcpu->vcpu_id)
return;
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index dd0e41d454a7..2908adc79ea6 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -965,6 +965,7 @@ bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
BIT(APICV_INHIBIT_REASON_PIT_REINJ) |
BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |
BIT(APICV_INHIBIT_REASON_SEV) |
+ BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |
BIT(APICV_INHIBIT_REASON_X2APIC);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 974d9a366d5d..5920166d7260 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7955,6 +7955,7 @@ static bool vmx_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
BIT(APICV_INHIBIT_REASON_ABSENT) |
BIT(APICV_INHIBIT_REASON_HYPERV) |
BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |
+ BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED);
--
2.38.0.rc1.362.ged0d419d3c-goog
Move the VMCB updates from avic_refresh_apicv_exec_ctrl() into
avic_set_virtual_apic_mode() and invert the dependency being said
functions to avoid calling avic_vcpu_{load,put}() and
avic_set_pi_irte_mode() when "only" setting the virtual APIC mode.
avic_set_virtual_apic_mode() is invoked from common x86 with preemption
enabled, which makes avic_vcpu_{load,put}() unhappy. Luckily, calling
those and updating IRTE stuff is unnecessary as the only reason
avic_set_virtual_apic_mode() is called is to handle transitions between
xAPIC and x2APIC that don't also toggle APICv activation. And if
activation doesn't change, there's no need to fiddle with the physical
APIC ID table or update IRTE.
The "full" refresh is guaranteed to be called if activation changes in
this case as the only call to the "set" path is:
kvm_vcpu_update_apicv(vcpu);
static_call_cond(kvm_x86_set_virtual_apic_mode)(vcpu);
and kvm_vcpu_update_apicv() invokes the refresh if activation changes:
if (apic->apicv_active == activate)
goto out;
apic->apicv_active = activate;
kvm_apic_update_apicv(vcpu);
static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu);
Rename the helper to reflect that it is also called during "refresh".
WARNING: CPU: 183 PID: 49186 at arch/x86/kvm/svm/avic.c:1081 avic_vcpu_put+0xde/0xf0 [kvm_amd]
CPU: 183 PID: 49186 Comm: stable Tainted: G O 6.0.0-smp--fcddbca45f0a-sink #34
Hardware name: Google, Inc. Arcadia_IT_80/Arcadia_IT_80, BIOS 10.48.0 01/27/2022
RIP: 0010:avic_vcpu_put+0xde/0xf0 [kvm_amd]
avic_refresh_apicv_exec_ctrl+0x142/0x1c0 [kvm_amd]
avic_set_virtual_apic_mode+0x5a/0x70 [kvm_amd]
kvm_lapic_set_base+0x149/0x1a0 [kvm]
kvm_set_apic_base+0x8f/0xd0 [kvm]
kvm_set_msr_common+0xa3a/0xdc0 [kvm]
svm_set_msr+0x364/0x6b0 [kvm_amd]
__kvm_set_msr+0xb8/0x1c0 [kvm]
kvm_emulate_wrmsr+0x58/0x1d0 [kvm]
msr_interception+0x1c/0x30 [kvm_amd]
svm_invoke_exit_handler+0x31/0x100 [kvm_amd]
svm_handle_exit+0xfc/0x160 [kvm_amd]
vcpu_enter_guest+0x21bb/0x23e0 [kvm]
vcpu_run+0x92/0x450 [kvm]
kvm_arch_vcpu_ioctl_run+0x43e/0x6e0 [kvm]
kvm_vcpu_ioctl+0x559/0x620 [kvm]
Fixes: 05c4fe8c1bd9 ("KVM: SVM: Refresh AVIC configuration when changing APIC mode")
Cc: [email protected]
Cc: Suravee Suthikulpanit <[email protected]>
Cc: Maxim Levitsky <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm/avic.c | 31 +++++++++++++++----------------
arch/x86/kvm/svm/svm.c | 2 +-
arch/x86/kvm/svm/svm.h | 2 +-
3 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 3b2c88b168ba..97ad0661f963 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -747,18 +747,6 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
avic_handle_ldr_update(vcpu);
}
-void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
-{
- if (!lapic_in_kernel(vcpu) || avic_mode == AVIC_MODE_NONE)
- return;
-
- if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
- WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
- return;
- }
- avic_refresh_apicv_exec_ctrl(vcpu);
-}
-
static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
{
int ret = 0;
@@ -1100,17 +1088,18 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
}
-
-void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
+void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
struct vmcb *vmcb = svm->vmcb01.ptr;
- bool activated = kvm_vcpu_apicv_active(vcpu);
+
+ if (!lapic_in_kernel(vcpu) || avic_mode == AVIC_MODE_NONE)
+ return;
if (!enable_apicv)
return;
- if (activated) {
+ if (kvm_vcpu_apicv_active(vcpu)) {
/**
* During AVIC temporary deactivation, guest could update
* APIC ID, DFR and LDR registers, which would not be trapped
@@ -1124,6 +1113,16 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
avic_deactivate_vmcb(svm);
}
vmcb_mark_dirty(vmcb, VMCB_AVIC);
+}
+
+void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
+{
+ bool activated = kvm_vcpu_apicv_active(vcpu);
+
+ if (!enable_apicv)
+ return;
+
+ avic_refresh_virtual_apic_mode(vcpu);
if (activated)
avic_vcpu_load(vcpu, vcpu->cpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 58f0077d9357..afae97ea9a06 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4798,7 +4798,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
.enable_nmi_window = svm_enable_nmi_window,
.enable_irq_window = svm_enable_irq_window,
.update_cr8_intercept = svm_update_cr8_intercept,
- .set_virtual_apic_mode = avic_set_virtual_apic_mode,
+ .set_virtual_apic_mode = avic_refresh_virtual_apic_mode,
.refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
.apicv_post_state_restore = avic_apicv_post_state_restore,
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 6a7686bf6900..7a95f50e80e7 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -646,7 +646,7 @@ void avic_vcpu_blocking(struct kvm_vcpu *vcpu);
void avic_vcpu_unblocking(struct kvm_vcpu *vcpu);
void avic_ring_doorbell(struct kvm_vcpu *vcpu);
unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu);
-void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
+void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
/* sev.c */
--
2.38.0.rc1.362.ged0d419d3c-goog
Add a helper to perform the final kick, two instances of the ICR decoding
is one too many.
No functional change intended.
Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Maxim Levitsky <[email protected]>
---
arch/x86/kvm/svm/avic.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 40a1ea21074d..dd0e41d454a7 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -317,6 +317,16 @@ void avic_ring_doorbell(struct kvm_vcpu *vcpu)
put_cpu();
}
+
+static void avic_kick_vcpu(struct kvm_vcpu *vcpu, u32 icrl)
+{
+ vcpu->arch.apic->irr_pending = true;
+ svm_complete_interrupt_delivery(vcpu,
+ icrl & APIC_MODE_MASK,
+ icrl & APIC_INT_LEVELTRIG,
+ icrl & APIC_VECTOR_MASK);
+}
+
/*
* A fast-path version of avic_kick_target_vcpus(), which attempts to match
* destination APIC ID to vCPU without looping through all vCPUs.
@@ -415,11 +425,7 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
if (unlikely(!target_vcpu))
return 0;
- target_vcpu->arch.apic->irr_pending = true;
- svm_complete_interrupt_delivery(target_vcpu,
- icrl & APIC_MODE_MASK,
- icrl & APIC_INT_LEVELTRIG,
- icrl & APIC_VECTOR_MASK);
+ avic_kick_vcpu(target_vcpu, icrl);
return 0;
}
@@ -443,13 +449,8 @@ static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
*/
kvm_for_each_vcpu(i, vcpu, kvm) {
if (kvm_apic_match_dest(vcpu, source, icrl & APIC_SHORT_MASK,
- dest, icrl & APIC_DEST_MASK)) {
- vcpu->arch.apic->irr_pending = true;
- svm_complete_interrupt_delivery(vcpu,
- icrl & APIC_MODE_MASK,
- icrl & APIC_INT_LEVELTRIG,
- icrl & APIC_VECTOR_MASK);
- }
+ dest, icrl & APIC_DEST_MASK))
+ avic_kick_vcpu(vcpu, icrl);
}
}
--
2.38.0.rc1.362.ged0d419d3c-goog
Inhibit SVM's AVIC if multiple vCPUs are aliased to the same logical ID.
Architecturally, all CPUs whose logical ID matches the MDA are supposed
to receive the interrupt; overwriting existing entries in AVIC's
logical=>physical map can result in missed IPIs.
Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 6 ++++++
arch/x86/kvm/lapic.c | 5 +++++
arch/x86/kvm/svm/avic.c | 3 ++-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 171e38b94c89..4fd06965c773 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1159,6 +1159,12 @@ enum kvm_apicv_inhibit {
* APIC base. For simplicity, this is sticky.
*/
APICV_INHIBIT_REASON_X2APIC,
+
+ /*
+ * AVIC is disabled because not all vCPUs with a valid LDR have a 1:1
+ * mapping between logical ID and vCPU.
+ */
+ APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
};
struct kvm_arch {
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index f6f328d36ae2..9b3af49d2524 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -391,6 +391,11 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
else
kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
+ if (!new || new->logical_mode == KVM_APIC_MODE_MAP_DISABLED)
+ kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
+ else
+ kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
+
old = rcu_dereference_protected(kvm->arch.apic_map,
lockdep_is_held(&kvm->arch.apic_map_lock));
rcu_assign_pointer(kvm->arch.apic_map, new);
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 2908adc79ea6..27d5abc15a91 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -968,7 +968,8 @@ bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |
- BIT(APICV_INHIBIT_REASON_X2APIC);
+ BIT(APICV_INHIBIT_REASON_X2APIC) |
+ BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
return supported & BIT(reason);
}
--
2.38.0.rc1.362.ged0d419d3c-goog
On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> Inhibit SVM's AVIC if multiple vCPUs are aliased to the same logical ID.
> Architecturally, all CPUs whose logical ID matches the MDA are supposed
> to receive the interrupt; overwriting existing entries in AVIC's
> logical=>physical map can result in missed IPIs.
>
> Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 6 ++++++
> arch/x86/kvm/lapic.c | 5 +++++
> arch/x86/kvm/svm/avic.c | 3 ++-
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 171e38b94c89..4fd06965c773 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1159,6 +1159,12 @@ enum kvm_apicv_inhibit {
> * APIC base. For simplicity, this is sticky.
> */
> APICV_INHIBIT_REASON_X2APIC,
> +
> + /*
> + * AVIC is disabled because not all vCPUs with a valid LDR have a 1:1
> + * mapping between logical ID and vCPU.
> + */
> + APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
> };
>
> struct kvm_arch {
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index f6f328d36ae2..9b3af49d2524 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -391,6 +391,11 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> else
> kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
>
> + if (!new || new->logical_mode == KVM_APIC_MODE_MAP_DISABLED)
> + kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
> + else
> + kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
> +
> old = rcu_dereference_protected(kvm->arch.apic_map,
> lockdep_is_held(&kvm->arch.apic_map_lock));
> rcu_assign_pointer(kvm->arch.apic_map, new);
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 2908adc79ea6..27d5abc15a91 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -968,7 +968,8 @@ bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
> BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
> BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
> BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |
> - BIT(APICV_INHIBIT_REASON_X2APIC);
> + BIT(APICV_INHIBIT_REASON_X2APIC) |
> + BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
>
> return supported & BIT(reason);
> }
Reviewed-by: Maxim Levitsky <[email protected]>
Best regards,
Maxim Levitsky
On Sat, 2022-10-01 at 00:58 +0000, Sean Christopherson wrote:
> Move the VMCB updates from avic_refresh_apicv_exec_ctrl() into
> avic_set_virtual_apic_mode() and invert the dependency being said
> functions to avoid calling avic_vcpu_{load,put}() and
> avic_set_pi_irte_mode() when "only" setting the virtual APIC mode.
>
> avic_set_virtual_apic_mode() is invoked from common x86 with preemption
> enabled, which makes avic_vcpu_{load,put}() unhappy. Luckily, calling
> those and updating IRTE stuff is unnecessary as the only reason
> avic_set_virtual_apic_mode() is called is to handle transitions between
> xAPIC and x2APIC that don't also toggle APICv activation. And if
> activation doesn't change, there's no need to fiddle with the physical
> APIC ID table or update IRTE.
>
> The "full" refresh is guaranteed to be called if activation changes in
> this case as the only call to the "set" path is:
>
> kvm_vcpu_update_apicv(vcpu);
> static_call_cond(kvm_x86_set_virtual_apic_mode)(vcpu);
>
> and kvm_vcpu_update_apicv() invokes the refresh if activation changes:
>
> if (apic->apicv_active == activate)
> goto out;
>
> apic->apicv_active = activate;
> kvm_apic_update_apicv(vcpu);
> static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu);
>
> Rename the helper to reflect that it is also called during "refresh".
>
> WARNING: CPU: 183 PID: 49186 at arch/x86/kvm/svm/avic.c:1081 avic_vcpu_put+0xde/0xf0 [kvm_amd]
> CPU: 183 PID: 49186 Comm: stable Tainted: G O 6.0.0-smp--fcddbca45f0a-sink #34
> Hardware name: Google, Inc. Arcadia_IT_80/Arcadia_IT_80, BIOS 10.48.0 01/27/2022
> RIP: 0010:avic_vcpu_put+0xde/0xf0 [kvm_amd]
> avic_refresh_apicv_exec_ctrl+0x142/0x1c0 [kvm_amd]
> avic_set_virtual_apic_mode+0x5a/0x70 [kvm_amd]
> kvm_lapic_set_base+0x149/0x1a0 [kvm]
> kvm_set_apic_base+0x8f/0xd0 [kvm]
> kvm_set_msr_common+0xa3a/0xdc0 [kvm]
> svm_set_msr+0x364/0x6b0 [kvm_amd]
> __kvm_set_msr+0xb8/0x1c0 [kvm]
> kvm_emulate_wrmsr+0x58/0x1d0 [kvm]
> msr_interception+0x1c/0x30 [kvm_amd]
> svm_invoke_exit_handler+0x31/0x100 [kvm_amd]
> svm_handle_exit+0xfc/0x160 [kvm_amd]
> vcpu_enter_guest+0x21bb/0x23e0 [kvm]
> vcpu_run+0x92/0x450 [kvm]
> kvm_arch_vcpu_ioctl_run+0x43e/0x6e0 [kvm]
> kvm_vcpu_ioctl+0x559/0x620 [kvm]
>
> Fixes: 05c4fe8c1bd9 ("KVM: SVM: Refresh AVIC configuration when changing APIC mode")
> Cc: [email protected]
> Cc: Suravee Suthikulpanit <[email protected]>
> Cc: Maxim Levitsky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/svm/avic.c | 31 +++++++++++++++----------------
> arch/x86/kvm/svm/svm.c | 2 +-
> arch/x86/kvm/svm/svm.h | 2 +-
> 3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3b2c88b168ba..97ad0661f963 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -747,18 +747,6 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
> avic_handle_ldr_update(vcpu);
> }
>
> -void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> -{
> - if (!lapic_in_kernel(vcpu) || avic_mode == AVIC_MODE_NONE)
> - return;
> -
> - if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
> - WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
> - return;
> - }
> - avic_refresh_apicv_exec_ctrl(vcpu);
> -}
> -
> static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
> {
> int ret = 0;
> @@ -1100,17 +1088,18 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
> WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
> }
>
> -
> -void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> +void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> struct vmcb *vmcb = svm->vmcb01.ptr;
> - bool activated = kvm_vcpu_apicv_active(vcpu);
> +
> + if (!lapic_in_kernel(vcpu) || avic_mode == AVIC_MODE_NONE)
> + return;
>
> if (!enable_apicv)
> return;
>
> - if (activated) {
> + if (kvm_vcpu_apicv_active(vcpu)) {
> /**
> * During AVIC temporary deactivation, guest could update
> * APIC ID, DFR and LDR registers, which would not be trapped
> @@ -1124,6 +1113,16 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> avic_deactivate_vmcb(svm);
> }
> vmcb_mark_dirty(vmcb, VMCB_AVIC);
> +}
> +
> +void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> +{
> + bool activated = kvm_vcpu_apicv_active(vcpu);
> +
> + if (!enable_apicv)
> + return;
> +
> + avic_refresh_virtual_apic_mode(vcpu);
>
> if (activated)
> avic_vcpu_load(vcpu, vcpu->cpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 58f0077d9357..afae97ea9a06 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4798,7 +4798,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
> .enable_nmi_window = svm_enable_nmi_window,
> .enable_irq_window = svm_enable_irq_window,
> .update_cr8_intercept = svm_update_cr8_intercept,
> - .set_virtual_apic_mode = avic_set_virtual_apic_mode,
> + .set_virtual_apic_mode = avic_refresh_virtual_apic_mode,
> .refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
> .check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
> .apicv_post_state_restore = avic_apicv_post_state_restore,
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 6a7686bf6900..7a95f50e80e7 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -646,7 +646,7 @@ void avic_vcpu_blocking(struct kvm_vcpu *vcpu);
> void avic_vcpu_unblocking(struct kvm_vcpu *vcpu);
> void avic_ring_doorbell(struct kvm_vcpu *vcpu);
> unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu);
> -void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
> +void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
>
>
> /* sev.c */
Makes sense. I missed that during x2avic review. That warning about preemption is a good one.
Reviewed-by: Maxim Levitsky <[email protected]>
Best regards,
Maxim Levitsky
On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> Inhibit APICv/AVIC if the optimized physical map is disabled so that KVM
> KVM provides consistent APIC behavior if xAPIC IDs are aliased due to
> vcpu_id being truncated and the x2APIC hotplug hack isn't enabled. If
> the hotplug hack is disabled, events that are emulated by KVM will follow
> architectural behavior (all matching vCPUs receive events, even if the
> "match" is due to truncation), whereas APICv and AVIC will deliver events
> only to the first matching vCPU, i.e. the vCPU that matches without
> truncation.
>
> Note, the "extra" inhibit is needed because KVM deliberately ignores
> mismatches due to truncation when applying the APIC_ID_MODIFIED inhibit
> so that large VMs (>255 vCPUs) can run with APICv/AVIC.
>
> Fixes: TDB
Do you mean Trade & Development Bank of Mongolia ? ;-)
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 6 ++++++
> arch/x86/kvm/lapic.c | 13 ++++++++++++-
> arch/x86/kvm/svm/avic.c | 1 +
> arch/x86/kvm/vmx/vmx.c | 1 +
> 4 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ac28bbfbf0e3..171e38b94c89 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1104,6 +1104,12 @@ enum kvm_apicv_inhibit {
> */
> APICV_INHIBIT_REASON_BLOCKIRQ,
>
> + /*
> + * APICv is disabled because not all vCPUs have a 1:1 mapping between
> + * APIC ID and vCPU, _and_ KVM is not applying its x2APIC hotplug hack.
> + */
> + APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED,
> +
> /*
> * For simplicity, the APIC acceleration is inhibited
> * first time either APIC ID or APIC base are changed by the guest
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 340c2d3e781b..f6f328d36ae2 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -381,6 +381,16 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> cluster[ldr] = apic;
> }
> out:
> + /*
> + * The optimized map is effectively KVM's internal version of APICv,
Nitpick: APICv/AVIC?
> + * and all unwanted aliasing that results in disabling the optimized
> + * map also applies to APICv.
> + */
> + if (!new)
> + kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
> + else
> + kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED);
> +
> old = rcu_dereference_protected(kvm->arch.apic_map,
> lockdep_is_held(&kvm->arch.apic_map_lock));
> rcu_assign_pointer(kvm->arch.apic_map, new);
> @@ -2153,7 +2163,8 @@ static void kvm_lapic_xapic_id_updated(struct kvm_lapic *apic)
> /*
> * Deliberately truncate the vCPU ID when detecting a modified APIC ID
> * to avoid false positives if the vCPU ID, i.e. x2APIC ID, is a 32-bit
> - * value.
> + * value. If the wrap/truncation results in unwatned aliasing, APICv
^^ typo
> + * will be inhibited as part of updating KVM's optimized APIC maps.
> */
> if (kvm_xapic_id(apic) == (u8)apic->vcpu->vcpu_id)
> return;
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index dd0e41d454a7..2908adc79ea6 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -965,6 +965,7 @@ bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
> BIT(APICV_INHIBIT_REASON_PIT_REINJ) |
> BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |
> BIT(APICV_INHIBIT_REASON_SEV) |
> + BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
> BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
> BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |
> BIT(APICV_INHIBIT_REASON_X2APIC);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 974d9a366d5d..5920166d7260 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7955,6 +7955,7 @@ static bool vmx_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
> BIT(APICV_INHIBIT_REASON_ABSENT) |
> BIT(APICV_INHIBIT_REASON_HYPERV) |
> BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |
> + BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
> BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
> BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED);
>
Reviewed-by: Maxim Levitsky <[email protected]>
Best regards,
Maxim Levitsky
On Thu, Dec 08, 2022, Maxim Levitsky wrote:
> On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> > Inhibit APICv/AVIC if the optimized physical map is disabled so that KVM
> > KVM provides consistent APIC behavior if xAPIC IDs are aliased due to
> > vcpu_id being truncated and the x2APIC hotplug hack isn't enabled. If
> > the hotplug hack is disabled, events that are emulated by KVM will follow
> > architectural behavior (all matching vCPUs receive events, even if the
> > "match" is due to truncation), whereas APICv and AVIC will deliver events
> > only to the first matching vCPU, i.e. the vCPU that matches without
> > truncation.
> >
> > Note, the "extra" inhibit is needed because KVM deliberately ignores
> > mismatches due to truncation when applying the APIC_ID_MODIFIED inhibit
> > so that large VMs (>255 vCPUs) can run with APICv/AVIC.
> >
> > Fixes: TDB
> Do you mean Trade & Development Bank of Mongolia ? ;-)
Heh, this fixes a patch/commit earlier in the series, hence the placeholder.
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/x86/include/asm/kvm_host.h | 6 ++++++
> > arch/x86/kvm/lapic.c | 13 ++++++++++++-
> > arch/x86/kvm/svm/avic.c | 1 +
> > arch/x86/kvm/vmx/vmx.c | 1 +
> > 4 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index ac28bbfbf0e3..171e38b94c89 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1104,6 +1104,12 @@ enum kvm_apicv_inhibit {
> > */
> > APICV_INHIBIT_REASON_BLOCKIRQ,
> >
> > + /*
> > + * APICv is disabled because not all vCPUs have a 1:1 mapping between
> > + * APIC ID and vCPU, _and_ KVM is not applying its x2APIC hotplug hack.
> > + */
> > + APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED,
> > +
> > /*
> > * For simplicity, the APIC acceleration is inhibited
> > * first time either APIC ID or APIC base are changed by the guest
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 340c2d3e781b..f6f328d36ae2 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -381,6 +381,16 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> > cluster[ldr] = apic;
> > }
> > out:
> > + /*
> > + * The optimized map is effectively KVM's internal version of APICv,
> Nitpick: APICv/AVIC?
Hmm. I think my preference for common code that doesn't need to differentiate
between Intel and AMD (or AVIC vs. x2AVIC) is to use just APICv. "APICv" is a
mostly a KVM term, e.g. Intel uses "APIC-virtualization" as an umbrella term for
a massive pile of features, so I think bending the term to cover Intel+AMD is ok?
Typing APICv/AVIC everywhere will get tedious and creates its own flavor of
confusion, e.g. the inhibits all use APICV.
Queued, thanks. I think I'll do a pass on the commit messages, but
I'm technically on vacation and I'm not sure when I will have time
so I am pushing everything to kvm/queue in the meanwhile.
Paolo