Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 14 Jan 2022 16:08:52 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     Igor Mammedov <imammedo@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN
Message-ID: <YeGgFP3VmMldszgh@google.com>
References: <20211122175818.608220-1-vkuznets@redhat.com>
 <20211122175818.608220-3-vkuznets@redhat.com>
 <16368a89-99ea-e52c-47b6-bd006933ec1f@redhat.com>
 <20211227183253.45a03ca2@redhat.com>
 <61325b2b-dc93-5db2-2d0a-dd0900d947f2@redhat.com>
 <87mtkdqm7m.fsf@redhat.com>
 <20220103104057.4dcf7948@redhat.com>
 <YeCowpPBEHC6GJ59@google.com>
 <5b516b51f81874fe7cafe8ce6846bc9936d83cc7.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5b516b51f81874fe7cafe8ce6846bc9936d83cc7.camel@redhat.com>
Precedence: bulk

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@redhat.com> wrote:
> > > 
> > > > Paolo Bonzini <pbonzini@redhat.com> 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...