On Fri, 2022-01-14 at 10:58 +0800, Chao Gao wrote:
> On Thu, Jan 13, 2022 at 10:19:21PM +0000, Sean Christopherson wrote:
> > On Tue, Jan 11, 2022, Maxim Levitsky wrote:
> > > Both Intel and AMD's PRM also state that changing APIC ID is implementation
> > > dependent.
> > >
> > > I vote to forbid changing apic id, at least in the case any APIC acceleration
> > > is used, be that APICv or AVIC.
> >
> > That has my vote as well. For IPIv in particular there's not much concern with
> > backwards compability, i.e. we can tie the behavior to enable_ipiv.
Great!
>
> Hi Sean and Levitsky,
>
> Let's align on the implementation.
>
> To disable changes for xAPIC ID when IPIv/AVIC is enabled:
>
> 1. introduce a variable (forbid_apicid_change) for this behavior in kvm.ko
> and export it so that kvm-intel, kvm-amd can set it when IPIv/AVIC is
> enabled. To reduce complexity, this variable is a module level setting.
>
> 2. when guest attempts to change xAPIC ID but it is forbidden, KVM prints
> a warning on host and injects a #GP to guest.
>
> 3. remove AVIC code that deals with changes to xAPIC ID.
>
I have a patch for both, I attached them.
I haven't tested either of these patches that much other than a smoke test,
but I did test all of the guests I have and none broke in regard to boot.
I will send those patches as part of larger patch series that implements
nesting for AVIC. I hope to do this next week.
Best regards,
Maxim Levitsky
From 4a70416b98c4725dc28608152b66ec42a233b2e8 Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
Date: Sun, 9 Jan 2022 18:09:08 +0200
Subject: [PATCH 1/8] KVM: x86: lapic: don't allow to change APIC ID when apic
acceleration is enabled
No sane guest would change physical APIC IDs, and allowing this introduces bugs
into APIC acceleration code.
Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
---
arch/x86/kvm/lapic.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6e1fbbf4c508b..56bc494cadd3e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2007,10 +2007,16 @@ 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))
- kvm_apic_set_xapic_id(apic, val >> 24);
- else
+ if (!apic_x2apic_mode(apic) ||
+ /*
+ * Don't allow setting APIC ID with any APIC acceleration
+ * enabled to avoid unexpected issues
+ */
+ (enable_apicv && ((val >> 24) != apic->vcpu->vcpu_id))) {
ret = 1;
+ break;
+ }
+ kvm_apic_set_xapic_id(apic, val >> 24);
break;
case APIC_TASKPRI:
--
2.26.3
From 3200924ed056efe58b3d1d12675c194bea98c0fc Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
Date: Sun, 9 Jan 2022 18:14:12 +0200
Subject: [PATCH 2/8] KVM: x86: AVIC: remove broken code that updated APIC ID
Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
---
arch/x86/kvm/svm/avic.c | 37 ++++---------------------------------
1 file changed, 4 insertions(+), 33 deletions(-)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index f3ab00f407d5b..8655b35043134 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -480,35 +480,6 @@ static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
return ret;
}
-static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
-{
- u64 *old, *new;
- struct vcpu_svm *svm = to_svm(vcpu);
- u32 id = kvm_xapic_id(vcpu->arch.apic);
-
- if (vcpu->vcpu_id == id)
- return 0;
-
- old = avic_get_physical_id_entry(vcpu, vcpu->vcpu_id);
- new = avic_get_physical_id_entry(vcpu, id);
- if (!new || !old)
- return 1;
-
- /* We need to move physical_id_entry to new offset */
- *new = *old;
- *old = 0ULL;
- to_svm(vcpu)->avic_physical_id_cache = new;
-
- /*
- * Also update the guest physical APIC ID in the logical
- * APIC ID table entry if already setup the LDR.
- */
- if (svm->ldr_reg)
- avic_handle_ldr_update(vcpu);
-
- return 0;
-}
-
static void avic_handle_dfr_update(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
@@ -529,8 +500,10 @@ static int avic_unaccel_trap_write(struct vcpu_svm *svm)
switch (offset) {
case APIC_ID:
- if (avic_handle_apic_id_update(&svm->vcpu))
- return 0;
+ /* restore the value that we had, we don't support APIC ID
+ * changes, but due to trap VM exit, the value was
+ * already written*/
+ kvm_lapic_reg_write(apic, offset, svm->vcpu.vcpu_id << 24);
break;
case APIC_LDR:
if (avic_handle_ldr_update(&svm->vcpu))
@@ -624,8 +597,6 @@ int avic_init_vcpu(struct vcpu_svm *svm)
void avic_post_state_restore(struct kvm_vcpu *vcpu)
{
- if (avic_handle_apic_id_update(vcpu) != 0)
- return;
avic_handle_dfr_update(vcpu);
avic_handle_ldr_update(vcpu);
}
--
2.26.3
