From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Date: Mon, 22 Sep 2014 14:14:42 +0100 Message-ID: <542020C2.9000304@citrix.com> References: <541B09B402000078000363B4@mail.emea.novell.com> <541B0BB402000078000363BF@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XW3S5-00025D-ML for xen-devel@lists.xenproject.org; Mon, 22 Sep 2014 13:14:49 +0000 In-Reply-To: <541B0BB402000078000363BF@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 , xen-devel Cc: Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 18/09/14 15:43, Jan Beulich wrote: > int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_= content) > @@ -891,16 +876,33 @@ int hvm_x2apic_msr_write(struct vcpu *v, > = > switch ( offset ) > { > - int rc; > + case APIC_TASKPRI: > + case APIC_EOI: > + case APIC_SPIV: > + case APIC_CMCI: > + case APIC_LVTT ... APIC_LVTERR: > + case APIC_TMICT: > + case APIC_TMCCT: > + case APIC_TDCR: Most (all?) of these MSRs have reserved bits, which should fail with a #GP(0) for attempts to set. vlapic_reg_read() masks most of the relevant bits, but doesn't appear to hit a misbehaving VM. > + break; > = > case APIC_ICR: > - rc =3D vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> = 32)); > - if ( rc ) > - return rc; > + vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32); > break; > = > - case APIC_ICR2: > - return X86EMUL_UNHANDLEABLE; > + case APIC_SELF_IPI: > + offset =3D APIC_ICR; > + msr_content =3D APIC_DEST_SELF | (uint8_t)msr_content; > + break; > + > + case APIC_ESR: > + if ( msr_content ) > + { > + printk(XENLOG_G_WARNING "%pv: non-zero (%lx) LAPIC ESR write= \n", > + v, msr_content); I know this is just moving an existing error message, but is it actually useful? ESR is no more special than the other registers with some/all bits reserved. > + default: > + return X86EMUL_UNHANDLEABLE; > + } > } > = > return vlapic_reg_write(v, offset, (uint32_t)msr_content); > @@ -910,7 +912,10 @@ static int vlapic_range(struct vcpu *v, = > { > struct vlapic *vlapic =3D vcpu_vlapic(v); > unsigned long offset =3D addr - vlapic_base_address(vlapic); > - return (!vlapic_hw_disabled(vlapic) && (offset < PAGE_SIZE)); > + > + return !vlapic_hw_disabled(vlapic) && > + !vlapic_x2apic_mode(vlapic) && > + (offset < PAGE_SIZE); This check is too restrictive, at least on Intel. From SDM Vol 3 29.4.3.3 "As noted in Section 29.5, execution of WRMSR with ECX =3D 83FH (self-IPI MSR) can lead to an APIC-write VM exit if the =93virtual-interrupt delivery=94 VM-execution control is 1. The exit qualification for such an APIC-write VM exit is 3F0H." So we can still end up wandering the vlapic MMIO codepaths even in x2apic mode. ~Andrew