On Fri, 14 Jan 2022 10:31:50 +0100 Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote: > Igor Mammedov <imammedo@xxxxxxxxxx> writes: > > > On Thu, 13 Jan 2022 22:33:38 +0000 > > Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > >> On Mon, Jan 03, 2022, Igor Mammedov wrote: > >> > On Mon, 03 Jan 2022 09:04:29 +0100 > >> > Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote: > >> > > >> > > Paolo Bonzini <pbonzini@xxxxxxxxxx> writes: > >> > > > >> > > > On 12/27/21 18:32, Igor Mammedov wrote: > >> > > >>> Tweaked and queued nevertheless, thanks. > >> > > >> it seems this patch breaks VCPU hotplug, in scenario: > >> > > >> > >> > > >> 1. hotunplug existing VCPU (QEMU stores VCPU file descriptor in parked cpus list) > >> > > >> 2. hotplug it again (unsuspecting QEMU reuses stored file descriptor when recreating VCPU) > >> > > >> > >> > > >> RHBZ:https://bugzilla.redhat.com/show_bug.cgi?id=2028337#c11 > >> > > >> > >> > > > > >> > > > The fix here would be (in QEMU) to not call KVM_SET_CPUID2 again. > >> > > > However, we need to work around it in KVM, and allow KVM_SET_CPUID2 if > >> > > > the data passed to the ioctl is the same that was set before. > >> > > > >> > > Are we sure the data is going to be *exactly* the same? In particular, > >> > > when using vCPU fds from the parked list, do we keep the same > >> > > APIC/x2APIC id when hotplugging? Or can we actually hotplug with a > >> > > different id? > >> > > >> > If I recall it right, it can be a different ID easily. > >> > >> No, it cannot. KVM doesn't provide a way for userspace to change the APIC ID of > >> a vCPU after the vCPU is created. x2APIC flat out disallows changing the APIC ID, > >> and unless there's magic I'm missing, apic_mmio_write() => kvm_lapic_reg_write() > >> is not reachable from userspace. > >> > >> The only way for userspace to set the APIC ID is to change vcpu->vcpu_id, and that > >> can only be done at KVM_VCPU_CREATE. > >> > >> So, reusing a parked vCPU for hotplug must reuse the same APIC ID. QEMU handles > >> this by stashing the vcpu_id, a.k.a. APIC ID, when parking a vCPU, and reuses a > >> parked vCPU if and only if it has the same APIC ID. And because QEMU derives the > >> APIC ID from topology, that means all the topology CPUID leafs must remain the > >> same, otherwise the guest is hosed because it will send IPIs to the wrong vCPUs. > > > > Indeed, I was wrong. > > I just checked all cpu unplug history in qemu. It was introduced in qemu-2.7 > > and from the very beginning it did stash vcpu_id, > > so there is no old QEMU that would re-plug VCPU with different apic_id. > > Though tells us nothing about what other userspace implementations might do. > > > > The genie is out of the bottle already, 5.16 is released with the change > (which was promissed for some time, KVM was complaining with > pr_warn_ratelimited()). I'd be brave and say that if QEMU doesn't need > it then nobody else does (out of curiosity, are there KVM VMMs besides > QEMU which support CPU hotplug out there?). > > > However, a problem of failing KVM_SET_CPUID2 during VCPU re-plug > > is still there and re-plug will fail if KVM rejects repeated KVM_SET_CPUID2 > > even if ioctl called with exactly the same CPUID leafs as the 1st call. > > > > Assuming APIC id change doesn not need to be supported, I can send v2 > here with an empty allowlist. As you mentioned in another thread black list would be better to address Sean's concerns or just revert problematic commit.
