From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH v2 04/13] KVM: x86: dynamic kvm_apic_map Date: Mon, 11 Jul 2016 17:52:29 +0200 Message-ID: <20160711155229.GE21976@potion> References: <20160707171550.14675-1-rkrcmar@redhat.com> <20160707171550.14675-5-rkrcmar@redhat.com> <963b542a-1111-db83-8338-c32d44f98874@gmail.com> <3a5d86b6-9f1a-a6cf-8af4-ef6bf3936996@gmail.com> <20160711134837.GD21976@potion> <6dda9370-e538-2c7c-e95e-b6a6f1518bbf@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Yang Zhang , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Lan, Tianyu" , Igor Mammedov , Jan Kiszka , Peter Xu To: Paolo Bonzini Return-path: Content-Disposition: inline In-Reply-To: <6dda9370-e538-2c7c-e95e-b6a6f1518bbf@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2016-07-11 16:14+0200, Paolo Bonzini: > On 11/07/2016 15:48, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >>>> I guess the easiest solution is to replace kvm_apic_id with a fiel= d in >>>> struct kvm_lapic, which is already shifted right by 24 in xAPIC mo= de. >>=20 >> (I guess the fewest LOC is to look at vcpu->vcpu_id, which is equal = to >> x2apic id. xapic id cannot be greater than 255 and all of those ar= e >> covered by the initial value of max_id.) >=20 > Yes, this would work too. Or even better perhaps, look at vcpu->vcpu= _id > in kvm_apic_id? APIC ID is writeable in xAPIC mode, which would make the implementation weird without an extra variable. Always read-only APIC ID would be best, IMO. >>> Or we can just simply put the assignment of apic_base to the end. >>=20 >> Yes, this would work, I'd also remove recalculates from >> kvm_apic_set_*apic_id() and add a compiler barrier with comment for = good >> measure, even though set_virtual_x2apic_mode() serves as one. >=20 > Why a compiler barrier? True, it should be a proper pair of smp_wmb() and smp_rmb() in recalculate ... and current kvm_apic_id() reads in a wrong order, so changing the apic_base alone update wouldn't get rid of this race. >> (What makes a bit wary is that it doesn't avoid the same problem if = we >> changed KVM to reset apic id to xapic id first when disabling apic.= ) >=20 > Yes, this is why I prefer it fixed once and for all in kvm_apic_id... Seems most reasonable. We'll need to be careful to have a correct valu= e in the apic page, but there shouldn't be any races there. >> Races in recalculation and APIC ID changes also lead to invalid phys= ical >> maps, which haven't been taken care of properly ... >=20 > Hmm, true, but can be fixed separately. Probably the mutex should be > renamed so that it can be taken outside recalculate_apic_map... Good point, it'll make reasoning easier and shouldn't introduce any extra scalability issues.