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 11:42:51 +0100 Message-ID: <5422A02B.1070205@citrix.com> References: <541B09B402000078000363B4@mail.emea.novell.com> <541B0BEB02000078000363C3@mail.emea.novell.com> <54203298.7090700@citrix.com> <54205A3F020000780003723E@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XWk2C-0004Oc-RQ for xen-devel@lists.xenproject.org; Wed, 24 Sep 2014 10:42:56 +0000 In-Reply-To: <54205A3F020000780003723E@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 22/09/14 16:19, Jan Beulich wrote: >>>> On 22.09.14 at 16:30, wrote: >> On 18/09/14 15:44, Jan Beulich wrote: >>> @@ -656,6 +650,8 @@ static int vlapic_reg_write(struct vcpu >>> struct vlapic *vlapic = vcpu_vlapic(v); >>> int rc = X86EMUL_OKAY; >>> >>> + memset(&vlapic->loaded, 0, sizeof(vlapic->loaded)); >> This is slightly expensive to do for each register write. Why does this >> clobbering need to be here? Could it not be.... > No, clearing this state can't be done in the load functions themselves > (or else we wouldn't need the state in the first place. This, however, > is more obvious with the follow-up addition I mailed in reply to the > patch here, adding an "else" path to lapic_load_fixup(). That code > wouldn't work as intended if we cleared the state early. > > If writing 12 bytes to memory would really turn out expensive, we > may end up having to limit the clearing to certain register writes, > but I was really instead considering to also do the clearing on > register reads. > >>> +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); >> ... in here, where we have positively identified whether fixup is needed >> or not. > 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? 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. Furthermore, vlapic_x2apic_mode(vlapic) contradicts the use of {GET,SET}_xAPIC_ID(). ~Andrew