Message-ID: <1ff69ed503faa4c5df3ad1b5abe8979d570ef2b8.camel@redhat.com>
Date:   Tue, 11 Jan 2022 00:24:09 +0200
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Chao Gao <chao.gao@...el.com>
Cc:     Zeng Guang <guang.zeng@...el.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Kim Phillips <kim.phillips@....com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Jethro Beekman <jethro@...tanix.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Hu, Robert" <robert.hu@...el.com>
Subject: Re: [PATCH v5 7/8] KVM: VMX: Update PID-pointer table entry when
 APIC ID is changed

On Mon, 2022-01-10 at 15:45 +0800, Chao Gao wrote:
> On Fri, Jan 07, 2022 at 10:31:59AM +0200, Maxim Levitsky wrote:
> > On Fri, 2022-01-07 at 16:05 +0800, Zeng Guang wrote:
> > > On 1/6/2022 10:06 PM, Tom Lendacky wrote:
> > > > On 1/5/22 7:44 PM, Zeng Guang wrote:
> > > > > On 1/6/2022 3:13 AM, Tom Lendacky wrote:
> > > > > > On 12/31/21 8:28 AM, Zeng Guang wrote:
> > > > > > Won't this blow up on AMD since there is no corresponding SVM op?
> > > > > > 
> > > > > > Thanks,
> > > > > > Tom
> > > > > Right, need check ops validness to avoid ruining AMD system. Same
> > > > > consideration on ops "update_ipiv_pid_table" in patch8.
> > > > Not necessarily for patch8. That is "protected" by the
> > > > kvm_check_request(KVM_REQ_PID_TABLE_UPDATE, vcpu) test, but it couldn't hurt.
> > > 
> > > OK, make sense. Thanks.
> > 
> > I haven't fully reviewed this patch series yet,
> > and I will soon.
> > 
> > I just want to point out few things:
> 
> Thanks for pointing them out.
> 
> > 1. AMD's AVIC also has a PID table (its calle AVIC physical ID table). 
> > It stores addressses of vCPUs apic backing pages,
> > and thier real APIC IDs.
> > 
> > avic_init_backing_page initializes the entry (assuming apic_id == vcpu_id) 
> > (which is double confusing)
> > 
> > 2. For some reason KVM supports writable APIC IDs. Does anyone use these?
> > Even Intel's PRM strongly discourages users from using them and in X2APIC mode,
> > the APIC ID is read only.
> > 
> > Because of this we have quite some bookkeeping in lapic.c, 
> > (things like kvm_recalculate_apic_map and such)
> > 
> > Also AVIC has its own handling for writes to APIC_ID,APIC_LDR,APIC_DFR
> > which tries to update its physical and logical ID tables.
> 
> Intel's IPI virtualization doesn't handle logical-addressing IPIs. They cause
> APIC-write vm-exit as usual. So, this series doesn't handle APIC_LDR/DFR.
> 
> > (it used also to handle apic base and I removed this as apic base otherwise
> > was always hardcoded to the default vaule)
> > 
> > Note that avic_handle_apic_id_update is broken - it always copies the entry
> > from the default (apicid == vcpu_id) location to new location and zeros
> > the old location, which will fail in many cases, like even if the guest
> > were to swap few apic ids.
> 
> This series differs from avic_handle_apic_id_update slightly:
> 
> If a vCPU's APIC ID is changed, this series zeros the old entry in PID-pointer
> table and programs the vCPU's PID to the new entry (rather than copy from the
> old entry).

Yes. The AVIC code is pretty much totaly busted in this regard which I noticed recently. 
It will fail the 2nd time it is called because it zeroes the entry it copies, 
and even if the guest changes the APIC ID once, this code will still fail because, 
it is called after each AVIC inhibition.

> 
> But this series is also problematic if guest swaps two vCPU's APIC ID without
> using another free APIC ID; it would end up one of them having no valid entry.

Yes, exactly. I wanted to fix the AVIC's code and also noticed that.
Plus, the guest can assign the same APIC ID to two vCPUs in theory and keep it this
way which complicates things further, from the point of view of what malicious guests can do.
 

> 
> One solution in my mind is:
> 
> when a vCPU's APIC ID is changed, KVM traverses all vCPUs to count vCPUs using
> the old APIC ID and the new APIC ID, programs corrsponding entries following
> below rules:
> 1. populate an entry with a vCPU's PID if the corrsponding APIC ID is
> exclusively used by that vCPU.
> 2. zero an entry for other cases.

Yes, that what I was thinking as well - but zeroing *both* entries when they are duplicate,
is not what I was thinkging and it is a very good idea IMHO.


> 
> Proper locking is needed in this process to prevent changes to vCPUs' APIC IDs.
Yes.

> 
> Or if it doesn't worth it, we can disable IPI virtualization for a guest on its
> first attempt to change xAPIC ID.

Yes, and this brings the main question. Are there any OSes that actually change the APIC ID?
 
I tested winxp, and win10 32 bit, and they seem to work just fine when I don't allow them to change apic id.
Older/more obscure oses like win98/95/dos and such don't use APIC at all.
That leaves only modern OSes like *BSD and such, I'll try to check a few of them soon.
 
I just don't see any reason whatsoever to change APIC ID. It seems that it was initially writable,
just because Intel' forgot to make it read-only, and then they even made it read-only with x2apic.
 
Both Intel and AMD's PRM also state that changing APIC ID is implementation dependent.
 
I vote to forbid changing apic id, at least in the case any APIC acceleration is used, be that APICv or AVIC.
 
 

Best regards,
	Maxim Levitsky

> 
> Let us know which option is preferred.
> 

Powered by blists - more mailing lists