Track all possibilities for the optimized APIC map's logical modes
instead of overloading the pseudo-bitmap and treating any "unknown" value
as "invalid".
As documented by the now-stale comment above the mode values, the values
did have meaning when the optimized map was originally added. That
dependent logical was removed by commit e45115b62f9a ("KVM: x86: use
physical LAPIC array for logical x2APIC"), but the obfuscated behavior
and its comment were left behind.
Opportunistically rename "mode" to "logical_mode", partly to make it
clear that the "disabled" case applies only to the logical map, but also
to prove that there is no lurking code that expects "mode" to be a bitmap.
Functionally, this is a glorified nop.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 21 +++++++++++---------
arch/x86/kvm/lapic.c | 35 +++++++++++++++++++++++++--------
2 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 062758135c86..ac28bbfbf0e3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -962,19 +962,22 @@ struct kvm_arch_memory_slot {
};
/*
- * We use as the mode the number of bits allocated in the LDR for the
- * logical processor ID. It happens that these are all powers of two.
- * This makes it is very easy to detect cases where the APICs are
- * configured for multiple modes; in that case, we cannot use the map and
- * hence cannot use kvm_irq_delivery_to_apic_fast either.
+ * Track the mode of the optimized logical map, as the rules for decoding the
+ * destination vary per mode. Enabling the optimized logical map requires all
+ * software-enabled local APIs to be in the same mode, each addressable APIC to
+ * be mapped to only one MDA, and each MDA to map to at most one APIC.
*/
-#define KVM_APIC_MODE_XAPIC_CLUSTER 4
-#define KVM_APIC_MODE_XAPIC_FLAT 8
-#define KVM_APIC_MODE_X2APIC 16
+enum kvm_apic_logical_mode {
+ KVM_APIC_MODE_SW_DISABLED,
+ KVM_APIC_MODE_XAPIC_CLUSTER,
+ KVM_APIC_MODE_XAPIC_FLAT,
+ KVM_APIC_MODE_X2APIC,
+ KVM_APIC_MODE_MAP_DISABLED,
+};
struct kvm_apic_map {
struct rcu_head rcu;
- u8 mode;
+ enum kvm_apic_logical_mode logical_mode;
u32 max_apic_id;
union {
struct kvm_lapic *xapic_flat_map[8];
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index cef8b202490b..9989893fef69 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -168,7 +168,12 @@ static bool kvm_use_posted_timer_interrupt(struct kvm_vcpu *vcpu)
static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
- switch (map->mode) {
+ switch (map->logical_mode) {
+ case KVM_APIC_MODE_SW_DISABLED:
+ /* Arbitrarily use the flat map so that @cluster isn't NULL. */
+ *cluster = map->xapic_flat_map;
+ *mask = 0;
+ return true;
case KVM_APIC_MODE_X2APIC: {
u32 offset = (dest_id >> 16) * 16;
u32 max_apic_id = map->max_apic_id;
@@ -193,8 +198,10 @@ static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
*cluster = map->xapic_cluster_map[(dest_id >> 4) & 0xf];
*mask = dest_id & 0xf;
return true;
+ case KVM_APIC_MODE_MAP_DISABLED:
+ return false;
default:
- /* Not optimized. */
+ WARN_ON_ONCE(1);
return false;
}
}
@@ -256,10 +263,12 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
goto out;
new->max_apic_id = max_id;
+ new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
kvm_for_each_vcpu(i, vcpu, kvm) {
struct kvm_lapic *apic = vcpu->arch.apic;
struct kvm_lapic **cluster;
+ enum kvm_apic_logical_mode logical_mode;
u16 mask;
u32 ldr;
u8 xapic_id;
@@ -282,7 +291,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
new->phys_map[xapic_id] = apic;
- if (!kvm_apic_sw_enabled(apic))
+ if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED ||
+ !kvm_apic_sw_enabled(apic))
continue;
ldr = kvm_lapic_get_reg(apic, APIC_LDR);
@@ -290,17 +300,26 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
continue;
if (apic_x2apic_mode(apic)) {
- new->mode |= KVM_APIC_MODE_X2APIC;
+ logical_mode = KVM_APIC_MODE_X2APIC;
} else {
ldr = GET_APIC_LOGICAL_ID(ldr);
if (kvm_lapic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
- new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
+ logical_mode = KVM_APIC_MODE_XAPIC_FLAT;
else
- new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
+ logical_mode = KVM_APIC_MODE_XAPIC_CLUSTER;
}
+ if (new->logical_mode != KVM_APIC_MODE_SW_DISABLED &&
+ new->logical_mode != logical_mode) {
+ new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
+ continue;
+ }
+ new->logical_mode = logical_mode;
- if (!kvm_apic_map_get_logical_dest(new, ldr, &cluster, &mask))
+ if (WARN_ON_ONCE(!kvm_apic_map_get_logical_dest(new, ldr,
+ &cluster, &mask))) {
+ new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
continue;
+ }
if (mask)
cluster[ffs(mask) - 1] = apic;
@@ -953,7 +972,7 @@ static bool kvm_apic_is_broadcast_dest(struct kvm *kvm, struct kvm_lapic **src,
{
if (kvm->arch.x2apic_broadcast_quirk_disabled) {
if ((irq->dest_id == APIC_BROADCAST &&
- map->mode != KVM_APIC_MODE_X2APIC))
+ map->logical_mode != KVM_APIC_MODE_X2APIC))
return true;
if (irq->dest_id == X2APIC_BROADCAST)
return true;
--
2.38.0.rc1.362.ged0d419d3c-goog
On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> Track all possibilities for the optimized APIC map's logical modes
> instead of overloading the pseudo-bitmap and treating any "unknown" value
> as "invalid".
>
> As documented by the now-stale comment above the mode values, the values
> did have meaning when the optimized map was originally added. That
> dependent logical was removed by commit e45115b62f9a ("KVM: x86: use
> physical LAPIC array for logical x2APIC"), but the obfuscated behavior
> and its comment were left behind.
>
> Opportunistically rename "mode" to "logical_mode", partly to make it
> clear that the "disabled" case applies only to the logical map, but also
> to prove that there is no lurking code that expects "mode" to be a bitmap.
>
> Functionally, this is a glorified nop.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 21 +++++++++++---------
> arch/x86/kvm/lapic.c | 35 +++++++++++++++++++++++++--------
> 2 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 062758135c86..ac28bbfbf0e3 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -962,19 +962,22 @@ struct kvm_arch_memory_slot {
> };
>
> /*
> - * We use as the mode the number of bits allocated in the LDR for the
> - * logical processor ID. It happens that these are all powers of two.
> - * This makes it is very easy to detect cases where the APICs are
> - * configured for multiple modes; in that case, we cannot use the map and
> - * hence cannot use kvm_irq_delivery_to_apic_fast either.
> + * Track the mode of the optimized logical map, as the rules for decoding the
> + * destination vary per mode. Enabling the optimized logical map requires all
> + * software-enabled local APIs to be in the same mode, each addressable APIC to
> + * be mapped to only one MDA, and each MDA to map to at most one APIC.
> */
> -#define KVM_APIC_MODE_XAPIC_CLUSTER 4
> -#define KVM_APIC_MODE_XAPIC_FLAT 8
> -#define KVM_APIC_MODE_X2APIC 16
> +enum kvm_apic_logical_mode {
It would be nice to have short comment about each of the modes, like
/* All local APICs are disabled */
> + KVM_APIC_MODE_SW_DISABLED,
/* All enabled local APICs are in XAPIC mode using cluster logical addresssing */
> + KVM_APIC_MODE_XAPIC_CLUSTER,
/* All enabled local APICs are in XAPIC mode using flat logical addresssing */
> + KVM_APIC_MODE_XAPIC_FLAT,
/* All enabled local APICs are in X2APIC mode */
> + KVM_APIC_MODE_X2APIC,
/*
Due to differencies in mode between enabled local APICs and/or other corner cases,
the optimized logical map disabled
*/
> + KVM_APIC_MODE_MAP_DISABLED,
> +};
>
> struct kvm_apic_map {
> struct rcu_head rcu;
> - u8 mode;
> + enum kvm_apic_logical_mode logical_mode;
> u32 max_apic_id;
> union {
> struct kvm_lapic *xapic_flat_map[8];
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index cef8b202490b..9989893fef69 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -168,7 +168,12 @@ static bool kvm_use_posted_timer_interrupt(struct kvm_vcpu *vcpu)
>
> static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
> u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
> - switch (map->mode) {
> + switch (map->logical_mode) {
> + case KVM_APIC_MODE_SW_DISABLED:
> + /* Arbitrarily use the flat map so that @cluster isn't NULL. */
> + *cluster = map->xapic_flat_map;
> + *mask = 0;
> + return true;
> case KVM_APIC_MODE_X2APIC: {
> u32 offset = (dest_id >> 16) * 16;
> u32 max_apic_id = map->max_apic_id;
> @@ -193,8 +198,10 @@ static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
> *cluster = map->xapic_cluster_map[(dest_id >> 4) & 0xf];
> *mask = dest_id & 0xf;
> return true;
> + case KVM_APIC_MODE_MAP_DISABLED:
> + return false;
> default:
> - /* Not optimized. */
> + WARN_ON_ONCE(1);
> return false;
> }
> }
> @@ -256,10 +263,12 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> goto out;
>
> new->max_apic_id = max_id;
> + new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
>
> kvm_for_each_vcpu(i, vcpu, kvm) {
> struct kvm_lapic *apic = vcpu->arch.apic;
> struct kvm_lapic **cluster;
> + enum kvm_apic_logical_mode logical_mode;
> u16 mask;
> u32 ldr;
> u8 xapic_id;
> @@ -282,7 +291,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
> new->phys_map[xapic_id] = apic;
>
> - if (!kvm_apic_sw_enabled(apic))
> + if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED ||
> + !kvm_apic_sw_enabled(apic))
> continue;
Very minor nitpick: it feels to me that code that updates the logical mode of the
map, might be better to be in a function, or in 'if', like
if (new->logical_mode != KVM_APIC_MODE_MAP_DISABLED) {
/* Disabled local APICs don't affect the logical map */
if (!kvm_apic_sw_enabled(apic))
continue;
....
}
>
> ldr = kvm_lapic_get_reg(apic, APIC_LDR);
> @@ -290,17 +300,26 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> continue;
>
> if (apic_x2apic_mode(apic)) {
> - new->mode |= KVM_APIC_MODE_X2APIC;
> + logical_mode = KVM_APIC_MODE_X2APIC;
> } else {
> ldr = GET_APIC_LOGICAL_ID(ldr);
> if (kvm_lapic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
> - new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
> + logical_mode = KVM_APIC_MODE_XAPIC_FLAT;
> else
> - new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
> + logical_mode = KVM_APIC_MODE_XAPIC_CLUSTER;
> }
> + if (new->logical_mode != KVM_APIC_MODE_SW_DISABLED &&
> + new->logical_mode != logical_mode) {
> + new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
> + continue;
> + }
> + new->logical_mode = logical_mode;
How about:
if (new->logical_mode == KVM_APIC_MODE_SW_DISABLED)
new->logical_mode = logical_mode;
if (new->logical_mode != logical_mode) {
new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
continue;
}
>
> - if (!kvm_apic_map_get_logical_dest(new, ldr, &cluster, &mask))
> + if (WARN_ON_ONCE(!kvm_apic_map_get_logical_dest(new, ldr,
> + &cluster, &mask))) {
> + new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
> continue;
> + }
>
> if (mask)
> cluster[ffs(mask) - 1] = apic;
> @@ -953,7 +972,7 @@ static bool kvm_apic_is_broadcast_dest(struct kvm *kvm, struct kvm_lapic **src,
> {
> if (kvm->arch.x2apic_broadcast_quirk_disabled) {
> if ((irq->dest_id == APIC_BROADCAST &&
> - map->mode != KVM_APIC_MODE_X2APIC))
> + map->logical_mode != KVM_APIC_MODE_X2APIC))
> return true;
> if (irq->dest_id == X2APIC_BROADCAST)
> return true;
Functionality wise the code looks OK to me.
Best regards,
Maxim Levitsky
On Thu, Dec 08, 2022, Maxim Levitsky wrote:
> On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> > -#define KVM_APIC_MODE_XAPIC_CLUSTER 4
> > -#define KVM_APIC_MODE_XAPIC_FLAT 8
> > -#define KVM_APIC_MODE_X2APIC 16
> > +enum kvm_apic_logical_mode {
>
> It would be nice to have short comment about each of the modes, like
>
> /* All local APICs are disabled */
> > + KVM_APIC_MODE_SW_DISABLED,
> /* All enabled local APICs are in XAPIC mode using cluster logical addresssing */
> > + KVM_APIC_MODE_XAPIC_CLUSTER,
> /* All enabled local APICs are in XAPIC mode using flat logical addresssing */
> > + KVM_APIC_MODE_XAPIC_FLAT,
> /* All enabled local APICs are in X2APIC mode */
> > + KVM_APIC_MODE_X2APIC,
> /*
> Due to differencies in mode between enabled local APICs and/or other corner cases,
> the optimized logical map disabled
> */
I'll add that.
> > + KVM_APIC_MODE_MAP_DISABLED,
> > +};
> >
> > struct kvm_apic_map {
> > struct rcu_head rcu;
> > - u8 mode;
> > + enum kvm_apic_logical_mode logical_mode;
> > u32 max_apic_id;
> > union {
> > struct kvm_lapic *xapic_flat_map[8];
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index cef8b202490b..9989893fef69 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -168,7 +168,12 @@ static bool kvm_use_posted_timer_interrupt(struct kvm_vcpu *vcpu)
> >
> > static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
> > u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
> > - switch (map->mode) {
> > + switch (map->logical_mode) {
> > + case KVM_APIC_MODE_SW_DISABLED:
> > + /* Arbitrarily use the flat map so that @cluster isn't NULL. */
> > + *cluster = map->xapic_flat_map;
> > + *mask = 0;
> > + return true;
> > case KVM_APIC_MODE_X2APIC: {
> > u32 offset = (dest_id >> 16) * 16;
> > u32 max_apic_id = map->max_apic_id;
> > @@ -193,8 +198,10 @@ static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
> > *cluster = map->xapic_cluster_map[(dest_id >> 4) & 0xf];
> > *mask = dest_id & 0xf;
> > return true;
> > + case KVM_APIC_MODE_MAP_DISABLED:
> > + return false;
> > default:
> > - /* Not optimized. */
> > + WARN_ON_ONCE(1);
> > return false;
> > }
> > }
> > @@ -256,10 +263,12 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> > goto out;
> >
> > new->max_apic_id = max_id;
> > + new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
> >
> > kvm_for_each_vcpu(i, vcpu, kvm) {
> > struct kvm_lapic *apic = vcpu->arch.apic;
> > struct kvm_lapic **cluster;
> > + enum kvm_apic_logical_mode logical_mode;
> > u16 mask;
> > u32 ldr;
> > u8 xapic_id;
> > @@ -282,7 +291,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> > if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
> > new->phys_map[xapic_id] = apic;
> >
> > - if (!kvm_apic_sw_enabled(apic))
> > + if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED ||
> > + !kvm_apic_sw_enabled(apic))
> > continue;
> Very minor nitpick: it feels to me that code that updates the logical mode of the
> map, might be better to be in a function, or in 'if', like
An if-statement would be rough due to the indentation. A function works well
though, especially if both the physical and logical chunks are put into helpers.
E.g. the patch at the bottom (with other fixup for this patch) yields:
new->max_apic_id = max_id;
new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
kvm_for_each_vcpu(i, vcpu, kvm) {
if (!kvm_apic_present(vcpu))
continue;
if (kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch)) {
kvfree(new);
new = NULL;
goto out;
}
kvm_recalculate_logical_map(new, vcpu);
}
I'll tack that patch on at the end of the series if it looks ok.
> if (new->logical_mode != KVM_APIC_MODE_MAP_DISABLED) {
>
> /* Disabled local APICs don't affect the logical map */
> if (!kvm_apic_sw_enabled(apic))
> continue;
> ....
> }
> >
> > ldr = kvm_lapic_get_reg(apic, APIC_LDR);
> > @@ -290,17 +300,26 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> > continue;
> >
> > if (apic_x2apic_mode(apic)) {
> > - new->mode |= KVM_APIC_MODE_X2APIC;
> > + logical_mode = KVM_APIC_MODE_X2APIC;
> > } else {
> > ldr = GET_APIC_LOGICAL_ID(ldr);
> > if (kvm_lapic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
> > - new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
> > + logical_mode = KVM_APIC_MODE_XAPIC_FLAT;
> > else
> > - new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
> > + logical_mode = KVM_APIC_MODE_XAPIC_CLUSTER;
> > }
> > + if (new->logical_mode != KVM_APIC_MODE_SW_DISABLED &&
> > + new->logical_mode != logical_mode) {
> > + new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
> > + continue;
> > + }
> > + new->logical_mode = logical_mode;
>
> How about:
>
> if (new->logical_mode == KVM_APIC_MODE_SW_DISABLED)
> new->logical_mode = logical_mode;
>
> if (new->logical_mode != logical_mode) {
> new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
> continue;
> }
This instead? So that it's a bit more obvious that the SW_DISABLED case is
exclusive with the mismatched mode path.
if (new->logical_mode == KVM_APIC_MODE_SW_DISABLED) {
new->logical_mode = logical_mode;
} else if (new->logical_mode != logical_mode) {
new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
continue;
}
---
arch/x86/kvm/lapic.c | 245 +++++++++++++++++++++++--------------------
1 file changed, 133 insertions(+), 112 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 5224d73cd84a..d017f49c5048 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -218,6 +218,134 @@ static void kvm_apic_map_free(struct rcu_head *rcu)
kvfree(map);
}
+static int kvm_recalculate_phys_map(struct kvm_apic_map *new,
+ struct kvm_vcpu *vcpu,
+ bool *xapic_id_mismatch)
+{
+ struct kvm_lapic *apic = vcpu->arch.apic;
+ u32 x2apic_id = kvm_x2apic_id(apic);
+ u32 xapic_id = kvm_xapic_id(apic);
+ u32 physical_id;
+
+ /*
+ * Deliberately truncate the vCPU ID when detecting a mismatched APIC
+ * ID to avoid false positives if the vCPU ID, i.e. x2APIC ID, is a
+ * 32-bit value. Any unwanted aliasing due to truncation results will
+ * be detected below.
+ */
+ if (!apic_x2apic_mode(apic) && xapic_id != (u8)vcpu->vcpu_id)
+ *xapic_id_mismatch = true;
+
+ /*
+ * Apply KVM's hotplug hack if userspace has enable 32-bit APIC IDs.
+ * Allow sending events to vCPUs by their x2APIC ID even if the target
+ * vCPU is in legacy xAPIC mode, and silently ignore aliased xAPIC IDs
+ * (the x2APIC ID is truncated to 8 bits, causing IDs > 0xff to wrap
+ * and collide).
+ *
+ * Honor the architectural (and KVM's non-optimized) behavior if
+ * userspace has not enabled 32-bit x2APIC IDs. Each APIC is supposed
+ * to process messages independently. If multiple vCPUs have the same
+ * effective APIC ID, e.g. due to the x2APIC wrap or because the guest
+ * manually modified its xAPIC IDs, events targeting that ID are
+ * supposed to be recognized by all vCPUs with said ID.
+ */
+ if (vcpu->kvm->arch.x2apic_format) {
+ /* See also kvm_apic_match_physical_addr(). */
+ if ((apic_x2apic_mode(apic) || x2apic_id > 0xff) &&
+ x2apic_id <= new->max_apic_id)
+ new->phys_map[x2apic_id] = apic;
+
+ if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
+ new->phys_map[xapic_id] = apic;
+ } else {
+ /*
+ * Disable the optimized map if the physical APIC ID is already
+ * mapped, i.e. is aliased to multiple vCPUs. The optimized
+ * map requires a strict 1:1 mapping between IDs and vCPUs.
+ */
+ if (apic_x2apic_mode(apic))
+ physical_id = x2apic_id;
+ else
+ physical_id = xapic_id;
+
+ if (new->phys_map[physical_id])
+ return -EINVAL;
+
+ new->phys_map[physical_id] = apic;
+ }
+
+ return 0;
+}
+
+static void kvm_recalculate_logical_map(struct kvm_apic_map *new,
+ struct kvm_vcpu *vcpu)
+{
+ struct kvm_lapic *apic = vcpu->arch.apic;
+ enum kvm_apic_logical_mode logical_mode;
+ struct kvm_lapic **cluster;
+ u16 mask;
+ u32 ldr;
+
+ if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED)
+ return;
+
+ if (kvm_apic_sw_enabled(apic))
+ return;
+
+ ldr = kvm_lapic_get_reg(apic, APIC_LDR);
+ if (!ldr)
+ return;
+
+ if (apic_x2apic_mode(apic)) {
+ logical_mode = KVM_APIC_MODE_X2APIC;
+ } else {
+ ldr = GET_APIC_LOGICAL_ID(ldr);
+ if (kvm_lapic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
+ logical_mode = KVM_APIC_MODE_XAPIC_FLAT;
+ else
+ logical_mode = KVM_APIC_MODE_XAPIC_CLUSTER;
+ }
+
+ /*
+ * To optimize logical mode delivery, all software-enabled APICs must
+ * be configured for the same mode.
+ */
+ if (new->logical_mode == KVM_APIC_MODE_SW_DISABLED) {
+ new->logical_mode = logical_mode;
+ } else if (new->logical_mode != logical_mode) {
+ new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
+ return;
+ }
+
+ /*
+ * In x2APIC mode, the LDR is read-only and derived directly from the
+ * x2APIC ID, thus is guaranteed to be addressable. KVM reuses
+ * kvm_apic_map.phys_map to optimize logical mode x2APIC interrupts by
+ * reversing the LDR calculation to get cluster of APICs, i.e. no
+ * additional work is required.
+ */
+ if (apic_x2apic_mode(apic)) {
+ WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(kvm_x2apic_id(apic)));
+ return;
+ }
+
+ if (WARN_ON_ONCE(!kvm_apic_map_get_logical_dest(new, ldr,
+ &cluster, &mask))) {
+ new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
+ return;
+ }
+
+ if (!mask)
+ return;
+
+ ldr = ffs(mask) - 1;
+ if (!is_power_of_2(mask) || cluster[ldr])
+ new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
+ else
+ cluster[ldr] = apic;
+}
+
/*
* CLEAN -> DIRTY and UPDATE_IN_PROGRESS -> DIRTY changes happen without a lock.
*
@@ -272,123 +400,16 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
kvm_for_each_vcpu(i, vcpu, kvm) {
- struct kvm_lapic *apic = vcpu->arch.apic;
- struct kvm_lapic **cluster;
- enum kvm_apic_logical_mode logical_mode;
- u32 x2apic_id, physical_id;
- u16 mask;
- u32 ldr;
- u8 xapic_id;
-
if (!kvm_apic_present(vcpu))
continue;
- xapic_id = kvm_xapic_id(apic);
- x2apic_id = kvm_x2apic_id(apic);
-
- /*
- * Deliberately truncate the vCPU ID when detecting a mismatched
- * APIC ID to avoid false positives if the vCPU ID, i.e. x2APIC
- * ID, is a 32-bit value. Any unwanted aliasing due to
- * truncation results will be detected below.
- */
- if (!apic_x2apic_mode(apic) && xapic_id != (u8)vcpu->vcpu_id)
- xapic_id_mismatch = true;
-
- /*
- * Apply KVM's hotplug hack if userspace has enable 32-bit APIC
- * IDs. Allow sending events to vCPUs by their x2APIC ID even
- * if the target vCPU is in legacy xAPIC mode, and silently
- * ignore aliased xAPIC IDs (the x2APIC ID is truncated to 8
- * bits, causing IDs > 0xff to wrap and collide).
- *
- * Honor the architectural (and KVM's non-optimized) behavior
- * if userspace has not enabled 32-bit x2APIC IDs. Each APIC
- * is supposed to process messages independently. If multiple
- * vCPUs have the same effective APIC ID, e.g. due to the
- * x2APIC wrap or because the guest manually modified its xAPIC
- * IDs, events targeting that ID are supposed to be recognized
- * by all vCPUs with said ID.
- */
- if (kvm->arch.x2apic_format) {
- /* See also kvm_apic_match_physical_addr(). */
- if ((apic_x2apic_mode(apic) || x2apic_id > 0xff) &&
- x2apic_id <= new->max_apic_id)
- new->phys_map[x2apic_id] = apic;
-
- if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
- new->phys_map[xapic_id] = apic;
- } else {
- /*
- * Disable the optimized map if the physical APIC ID is
- * already mapped, i.e. is aliased to multiple vCPUs.
- * The optimized map requires a strict 1:1 mapping
- * between IDs and vCPUs.
- */
- if (apic_x2apic_mode(apic))
- physical_id = x2apic_id;
- else
- physical_id = xapic_id;
-
- if (new->phys_map[physical_id]) {
- kvfree(new);
- new = NULL;
- goto out;
- }
- new->phys_map[physical_id] = apic;
+ if (kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch)) {
+ kvfree(new);
+ new = NULL;
+ goto out;
}
- if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED ||
- !kvm_apic_sw_enabled(apic))
- continue;
-
- ldr = kvm_lapic_get_reg(apic, APIC_LDR);
- if (!ldr)
- continue;
-
- if (apic_x2apic_mode(apic)) {
- logical_mode = KVM_APIC_MODE_X2APIC;
- } else {
- ldr = GET_APIC_LOGICAL_ID(ldr);
- if (kvm_lapic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
- logical_mode = KVM_APIC_MODE_XAPIC_FLAT;
- else
- logical_mode = KVM_APIC_MODE_XAPIC_CLUSTER;
- }
- if (new->logical_mode != KVM_APIC_MODE_SW_DISABLED &&
- new->logical_mode != logical_mode) {
- new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
- continue;
- }
- new->logical_mode = logical_mode;
-
- /*
- * In x2APIC mode, the LDR is read-only and derived directly
- * from the x2APIC ID, thus is guaranteed to be addressable.
- * KVM reuses kvm_apic_map.phys_map to optimize logical mode
- * x2APIC interrupts by reversing the LDR calculation to get
- * cluster of APICs, i.e. no additional work is required.
- */
- if (apic_x2apic_mode(apic)) {
- WARN_ON_ONCE(ldr != kvm_apic_calc_x2apic_ldr(x2apic_id));
- continue;
- }
-
- if (WARN_ON_ONCE(!kvm_apic_map_get_logical_dest(new, ldr,
- &cluster, &mask))) {
- new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
- continue;
- }
-
- if (!mask)
- continue;
-
- ldr = ffs(mask) - 1;
- if (!is_power_of_2(mask) || cluster[ldr]) {
- new->logical_mode = KVM_APIC_MODE_MAP_DISABLED;
- continue;
- }
- cluster[ldr] = apic;
+ kvm_recalculate_logical_map(new, vcpu);
}
out:
/*
base-commit: 46d887a39567c4d1517518e4bdfcc10f34b5d6ce
--
On Fri, Dec 16, 2022, Sean Christopherson wrote:
> On Thu, Dec 08, 2022, Maxim Levitsky wrote:
...
> > > @@ -282,7 +291,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> > > if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
> > > new->phys_map[xapic_id] = apic;
> > >
> > > - if (!kvm_apic_sw_enabled(apic))
> > > + if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED ||
> > > + !kvm_apic_sw_enabled(apic))
> > > continue;
> > Very minor nitpick: it feels to me that code that updates the logical mode of the
> > map, might be better to be in a function, or in 'if', like
>
> An if-statement would be rough due to the indentation. A function works well
> though, especially if both the physical and logical chunks are put into helpers.
> E.g. the patch at the bottom (with other fixup for this patch) yields:
>
> new->max_apic_id = max_id;
> new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
>
> kvm_for_each_vcpu(i, vcpu, kvm) {
> if (!kvm_apic_present(vcpu))
> continue;
>
> if (kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch)) {
> kvfree(new);
> new = NULL;
> goto out;
> }
>
> kvm_recalculate_logical_map(new, vcpu);
> }
>
> I'll tack that patch on at the end of the series if it looks ok.
...
> +static void kvm_recalculate_logical_map(struct kvm_apic_map *new,
> + struct kvm_vcpu *vcpu)
> +{
> + struct kvm_lapic *apic = vcpu->arch.apic;
> + enum kvm_apic_logical_mode logical_mode;
> + struct kvm_lapic **cluster;
> + u16 mask;
> + u32 ldr;
> +
> + if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED)
> + return;
> +
> + if (kvm_apic_sw_enabled(apic))
"minor" detail, this is inverted.
On 12/17/22 00:34, Sean Christopherson wrote:
> On Fri, Dec 16, 2022, Sean Christopherson wrote:
>> On Thu, Dec 08, 2022, Maxim Levitsky wrote:
>
> ...
>
>>>> @@ -282,7 +291,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>>>> if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
>>>> new->phys_map[xapic_id] = apic;
>>>>
>>>> - if (!kvm_apic_sw_enabled(apic))
>>>> + if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED ||
>>>> + !kvm_apic_sw_enabled(apic))
>>>> continue;
>>> Very minor nitpick: it feels to me that code that updates the logical mode of the
>>> map, might be better to be in a function, or in 'if', like
>>
>> An if-statement would be rough due to the indentation. A function works well
>> though, especially if both the physical and logical chunks are put into helpers.
>> E.g. the patch at the bottom (with other fixup for this patch) yields:
>>
>> new->max_apic_id = max_id;
>> new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
>>
>> kvm_for_each_vcpu(i, vcpu, kvm) {
>> if (!kvm_apic_present(vcpu))
>> continue;
>>
>> if (kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch)) {
>> kvfree(new);
>> new = NULL;
>> goto out;
>> }
>>
>> kvm_recalculate_logical_map(new, vcpu);
>> }
>>
>> I'll tack that patch on at the end of the series if it looks ok.
Yes, please send as a follow up.
Paolo