On Wed, 2022-05-18 at 19:51 +0800, Chao Gao wrote:
On Wed, May 18, 2022 at 12:50:27PM +0300, Maxim Levitsky wrote:
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.
I use it in later patches to kill the guest during nested VM entry if it attempts to use nested AVIC after any vCPU changed APIC ID.
I mentioned this boolean in the commit description.
This boolean avoids the need to go over all vCPUs and checking if they still have the initial apic id.
Do you want to kill the guest if APIC base got changed? If yes, you can check if APICV_INHIBIT_REASON_RO_SETTINGS is set and save the boolean.
Yep, I thrown in the apic base just because I can. It doesn't matter to my nested AVIC logic at all, but since it is also something that guests don't change, I also don't care if this will lead to inhibit and killing the guest if it attempts to use nested AVIC.
That boolean should have the same value as the APICV_INHIBIT_REASON_RO_SETTINGS inhibit, so yes I can instead check if the inhibit is active.
I don't know if that is cleaner that this boolean though, individual inhibit value is currently not something that anybody uses in logic.
Best regards, Maxim Levitsky
In the future maybe we can introduce a more generic 'taint' bitmap with various flags like that, indicating that the guest did something unexpected.
BTW, the other option in regard to the nested AVIC is just to ignore this issue completely. The code itself always uses vcpu_id's, thus regardless of when/how often the guest changes its apic ids, my code would just use the initial APIC ID values consistently.
In this case I won't need this boolean.
};
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.
Honestly since nobody uses this feature, I am not sure if to call this supported, I am sure that KVM has more bugs in regard of using non standard APIC ID. This warning might hopefuly make someone complain about it if this feature is actually used somewhere.
Now I got you. It is fine to me.