From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation Date: Wed, 24 Sep 2014 13:10:19 +0100 Message-ID: <5422B4AB.8040603@citrix.com> References: <541B09B402000078000363B4@mail.emea.novell.com> <541B0BEB02000078000363C3@mail.emea.novell.com> <54203298.7090700@citrix.com> <54205A3F020000780003723E@mail.emea.novell.com> <5422A02B.1070205@citrix.com> <5422CA1C0200007800038315@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XWlOs-0007KM-Nb for xen-devel@lists.xenproject.org; Wed, 24 Sep 2014 12:10:26 +0000 In-Reply-To: <5422CA1C0200007800038315@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 24/09/14 12:41, Jan Beulich wrote: >>>> On 24.09.14 at 12:42, wrote: >> On 22/09/14 16:19, Jan Beulich wrote: >>> For full context, here's the full intended function again: >>> >>> +static void lapic_load_fixup(struct vlapic *vlapic) >>> +{ >>> + uint32_t id = vlapic->loaded.id; >>> + >>> + if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 && >>> + /* Further checks are optional: ID != 0 contradicts LDR == 1. */ >>> + GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 && >>> + id == SET_xAPIC_ID(GET_xAPIC_ID(id)) ) >>> + set_x2apic_id(vlapic); >>> + else /* Undo an eventual earlier fixup. */ >>> + { >>> + vlapic_set_reg(vlapic, APIC_ID, id); >>> + vlapic_set_reg(vlapic, APIC_LDR, vlapic->loaded.ldr); >>> + } >>> +} >> How about dropping the optional checks, as "id && vlapic->loaded.ldr == >> 1" covers the broken hypervisor case? > I'd like to keep them for a while - after all that's why I added the > comment saying they're optional. The moment they start conflicting > with something else, they could be dropped. > > The alternative would be to make them WARN_ON()s inside the if(). Making them WARN_ON()s would make it more obvious if underlying assumption/implementations change which subsequently invalidate the checks. > >> The "id = vcpu_id * 2" is a broken assumption which I do need to fix as >> part of the cpuid infrastructure improvements, which would then break >> this check for a broken Xen. > For one it's not a broken assumption imo: The APIC ID gets set up > this way. Sorry - it is one which is expected to change in future development work. > And then I don't see why altering the APIC ID setting > would break this check here: If altering how the ID gets established > would get backported, I'd surely expect the change to the ID > handling here to also be. This check gets applied equally to migrations from the same version of Xen as to those from older versions. In this case I suppose the "id && vlapic->loaded.ldr" will short circuit the vcpu_id*2 check, in makes it ok (assuming no backports). > >> Furthermore, vlapic_x2apic_mode(vlapic) contradicts the use of >> {GET,SET}_xAPIC_ID(). > So it does, but intentionally. Remember - we're checking whether some > fixup to what came in is necessary, and part of the brokenness was > that the ID was left set in a legacy APIC manner. Right, which again due to short circuiting ok given that we have never supported x2apic with ids greater than 255. ~Andrew