| Message ID | 20220427200314.276673-4-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: nested AVIC | expand |
On Wed, Apr 27, 2022, Maxim Levitsky wrote: > AVIC is now inhibited if the guest changes apic id, thus remove > that broken code. Can you explicitly call out what's broken? Just something short on the code not handling the scenario where APIC ID is changed back to vcpu_id to help future archaeologists. I forget if there are other bugs...
On Thu, 2022-05-19 at 16:10 +0000, Sean Christopherson wrote: > On Wed, Apr 27, 2022, Maxim Levitsky wrote: > > AVIC is now inhibited if the guest changes apic id, thus remove > > that broken code. > > Can you explicitly call out what's broken? Just something short on the code not > handling the scenario where APIC ID is changed back to vcpu_id to help future > archaeologists. I forget if there are other bugs... > Well, the avic_handle_apic_id_update is called each time the AVIC is uninhibited, because while it is inhibited, the AVIC code doesn't track changes to APIC ID and such. Also there are many ways it is broken for example 1. a CPU can't move its APIC ID to a free slot due to (!new) check 2. If APIC ID is moved to a used slot, then the CPU that used that overwritten slot can't correctly move it, since its now not its slot, not to mention races. BTW, if you see a value in it, I can fix this code instead - a lock + going over all the apic ids, should be quite easy to implement. In case of two vCPUs using the same APIC ID, I can write non present entry to the table, so none will be able to be addressed, hoping that the situation is only temporary. Same can be done for IPIv. Best regards, Maxim Levitsky
On Sun, May 22, 2022, Maxim Levitsky wrote: > On Thu, 2022-05-19 at 16:10 +0000, Sean Christopherson wrote: > > On Wed, Apr 27, 2022, Maxim Levitsky wrote: > > > AVIC is now inhibited if the guest changes apic id, thus remove > > > that broken code. > > > > Can you explicitly call out what's broken? Just something short on the code not > > handling the scenario where APIC ID is changed back to vcpu_id to help future > > archaeologists. I forget if there are other bugs... > > > > > Well, the avic_handle_apic_id_update is called each time the AVIC is uninhibited, > because while it is inhibited, the AVIC code doesn't track changes to APIC ID and such. > > Also there are many ways it is broken for example > > 1. a CPU can't move its APIC ID to a free slot due to (!new) check > > 2. If APIC ID is moved to a used slot, then the CPU that used that overwritten > slot can't correctly move it, since its now not its slot, not to mention races. The more the merrier :-) Any/all of those examples are great, just so long as it's obvious to future readers that the code truly is busted. > BTW, if you see a value in it, I can fix this code instead - a lock + going over all the apic ids, > should be quite easy to implement. In case of two vCPUs using the same APIC ID, > I can write non present entry to the table, so none will be able to be addressed, > hoping that the situation is only temporary. Very strong "no", let's keep this as simple as possible without outright killing the guest or breaking ABI. Disabling APICv/AVIC is perfect.
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index 54fe03714f8a6..1102421668a11 100644 --- a/arch/x86/kvm/svm/avic.c +++ b/arch/x86/kvm/svm/avic.c @@ -508,35 +508,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); @@ -555,10 +526,6 @@ static int avic_unaccel_trap_write(struct kvm_vcpu *vcpu) AVIC_UNACCEL_ACCESS_OFFSET_MASK; switch (offset) { - case APIC_ID: - if (avic_handle_apic_id_update(vcpu)) - return 0; - break; case APIC_LDR: if (avic_handle_ldr_update(vcpu)) return 0; @@ -650,8 +617,6 @@ int avic_init_vcpu(struct vcpu_svm *svm) void avic_apicv_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); }
AVIC is now inhibited if the guest changes apic id, thus remove that broken code. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/avic.c | 35 ----------------------------------- 1 file changed, 35 deletions(-)