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: Thu, 25 Sep 2014 12:26:31 +0100 Message-ID: <5423FBE7.6050600@citrix.com> References: <5422FF2E0200007800038693@mail.emea.novell.com> <542300AF02000078000386B2@mail.emea.novell.com> <5422E9FA.5050606@citrix.com> <5423F17F0200007800038C59@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 1XX7C6-0004vu-Jx for xen-devel@lists.xenproject.org; Thu, 25 Sep 2014 11:26:42 +0000 In-Reply-To: <5423F17F0200007800038C59@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 List-Id: xen-devel@lists.xenproject.org On 25/09/14 09:42, Jan Beulich wrote: >>>> On 24.09.14 at 17:57, wrote: >> On 24/09/14 16:34, Jan Beulich wrote: >>> @@ -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? > Ah, one more case I failed to convert. > >>> @@ -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. > Not sure what you mean or what you're thinking here: & has higher > precedence than even &&, not to speak of ?:, so the expression is > quite fine without further parentheses. And yes, the whole ?: is > intended to be a compile time constant, just being prepared for > directed-EOI support to be added (and as this doesn't seem to need > any kind of conditional code, I'm expecting that to get added > directly to the VLAPIC_VERSION #define). > > Jan > Ah - I had logically inverted the condition I thought I had spotted. Reviewed-by: Andrew Cooper