From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation Date: Wed, 24 Sep 2014 16:57:46 +0100 Message-ID: <5422E9FA.5050606@citrix.com> References: <5422FF2E0200007800038693@mail.emea.novell.com> <542300AF02000078000386B2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XWoxL-0004IT-LN for xen-devel@lists.xenproject.org; Wed, 24 Sep 2014 15:58:15 +0000 In-Reply-To: <542300AF02000078000386B2@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 List-Id: xen-devel@lists.xenproject.org On 24/09/14 16:34, Jan Beulich wrote: > - generate #GP on invalid APIC base MSR transitions > - fail reads from the EOI and self-IPI registers (which are write-only) > - handle self-IPI writes and the ICR2 half of ICR writes largely in > hvm_x2apic_msr_write() and (for self-IPI only) vlapic_apicv_write() > - don't permit MMIO-based access in x2APIC mode > - filter writes to read-only registers in hvm_x2apic_msr_write(), > allowing conditionals to be dropped from vlapic_reg_write() > - don't ignore upper half of MSR-based write to ESR being non-zero > - don't ignore other writes to reserved bits > - VMX's EXIT_REASON_APIC_WRITE must not result in #GP (this exit being > trap-like, this exception would get raised on the wrong RIP) > - make hvm_x2apic_msr_read() produce X86EMUL_* return codes just like > hvm_x2apic_msr_write() does (benign to the only caller) > > Signed-off-by: Jan Beulich One nit and one bug. > @@ -877,8 +854,16 @@ static int vlapic_write(struct vcpu *v, > > int vlapic_apicv_write(struct vcpu *v, unsigned int offset) > { > - uint32_t val = vlapic_get_reg(vcpu_vlapic(v), offset); > - return vlapic_reg_write(v, offset, val); > + struct vlapic *vlapic = vcpu_vlapic(v); > + uint32_t val = vlapic_get_reg(vlapic, offset); > + > + if ( !vlapic_x2apic_mode(vlapic) ) > + return vlapic_reg_write(v, offset, val); > + > + if ( offset != APIC_SELF_IPI ) > + return X86EMUL_UNHANDLEABLE; > + > + return vlapic_reg_write(v, APIC_ICR, APIC_DEST_SELF | (uint8_t)val); (val & APIC_VECTOR_MASK) instead of a uint8_t cast? > } > > int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content) > @@ -891,16 +876,69 @@ int hvm_x2apic_msr_write(struct vcpu *v, > > switch ( offset ) > { > - int rc; > + case APIC_TASKPRI: > + if ( msr_content & ~APIC_TPRI_MASK ) > + return X86EMUL_UNHANDLEABLE; > + break; > + > + case APIC_SPIV: > + if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED | > + (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI Need brackets for the logical (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI) test to avoid making a constant expression with the precedence of the ternary operator. > + ? APIC_SPIV_DIRECTED_EOI : 0)) ) > + return X86EMUL_UNHANDLEABLE; > + break; > + Otherwise, Reviewed-by: Andrew Cooper