Re: [PATCH 2/2] KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN

[Igor Mammedov]

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.