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 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.