On Fri, Jan 14, 2022, Maxim Levitsky wrote: > On Thu, 2022-01-13 at 22:33 +0000, Sean Christopherson 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. > > So after all, it is true that vcpu_id == initial APIC_ID, > and if we don't let guest change it, it will be always like that? Except for kvm_apic_set_state(), which I forgot existed, yes. > You said that its not true in the other mail in the thread. I was wrong, I was thinking that userspace could reach kvm_lapic_reg_write(), but I forgot that there would be no connection without x2apic. But I forgot about kvm_apic_set_state()... > I haven't checked it in the code yet, as I never was much worried about > userspace changing, but I will check it soon. > > I did a quick look and I see that at least the userspace can call > 'kvm_apic_set_state' and it contains snapshot of all apic registers, > including apic id. However it would be very easy to add a check there and > fail if userspace attempts to set APIC_ID != vcpu_id. Yeah, hopefully that doesn't break any userspace. I can't imagine it would, because if the guest disabled and re-enabled the APIC, kvm_lapic_set_base() would restore the APIC ID to vcpu_id. With luck, that's the last hole we need to close...
