2016-12-14 17:15+0100, David Hildenbrand: >> kvm_for_each_vcpu(i, vcpu, kvm) >> if (kvm_apic_present(vcpu)) >> - max_id = max(max_id, kvm_apic_id(vcpu->arch.apic)); >> + max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic)); >> >> new = kvm_kvzalloc(sizeof(struct kvm_apic_map) + >> sizeof(struct kvm_lapic *) * ((u64)max_id + 1)); >> @@ -179,16 +189,23 @@ static void recalculate_apic_map(struct kvm *kvm) >> struct kvm_lapic *apic = vcpu->arch.apic; >> struct kvm_lapic **cluster; >> u16 mask; >> - u32 ldr, aid; >> + u32 ldr; >> + u8 xapic_id; >> + u32 x2apic_id; >> >> if (!kvm_apic_present(vcpu)) >> continue; >> >> - aid = kvm_apic_id(apic); > > think I'd even prefer here a simple > > aid = kvm_xapic_id(apic); > if (apic_x2apic_mode(apic)) > aid = kvm_x2apic_id(apic); > > that would keep changes minimal and I don't really see any benefit in the > code when splitting handling up. It is neccesassary to write an entry for both IDs and I wanted to split it before [4/4], because doing both changes at once seemed hard to grasp. Putting it here didn't work well either ... is a separate patch for the hunk below better, or would you prefer to have it in [4/4]? > Patch 4 then simply can fixup setting code > > if (aid <= new->max_apic_id && !new->phys_map[aid]) > new->phys_map[aid] = apic; > > (if I am not missing some important corner case here) The trick is that we want to do the following even in xAPIC mode: new->phys_map[kvm_x2apic_id(apic)] = apic; This is the main idea of the hotplug hack -- to allow unique addressing of processors that were reset in xAPIC mode. (And I add a disgusting "x2apic_id > 0xff" condition in [4/4], because we still allow guests to change xAPIC IDs, which wouldn't play nice with this.) Hardware does a superset of this, because it only looks at lower 8 bits of the desination ID when delivering to xAPIC. When kvm_x2apic_id(apic) != kvm_xapic_id(apic), then the APIC is in xAPIC mode so we definitely want to keep xAPIC working, hence if (!apic_x2apic_mode(apic)) new->phys_map[kvm_xapic_id(apic)] = apic; Two writes are necessary. And there can already be another_apic "kvm_x2apic_id(another_apic) == kvm_xapic_id(apic)" so we prevent hotplug from breaking existing x2APIC setups by doing "!new->phys_map[aid]" when setting xAPIC ID. I hope we get a better solution in the future, but it would have to be done at hardware (QEMU) level, because even firmware (seabios) doesn't have standard ways to deal with this situation ... >> - ldr = kvm_lapic_get_reg(apic, APIC_LDR); >> + xapic_id = kvm_xapic_id(apic); >> + x2apic_id = kvm_x2apic_id(apic); >> >> - if (aid <= new->max_apic_id) >> - new->phys_map[aid] = apic; >> + if (apic_x2apic_mode(apic) && >> + x2apic_id <= new->max_apic_id) >> + new->phys_map[x2apic_id] = apic; >> + else if (!apic_x2apic_mode(apic)) > > > This looks good to me. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
