Re: [PATCH v5 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation

[Andrew Cooper <andrew.cooper3@citrix.com>]

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 <jbeulich@suse.com>

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 <andrew.cooper3@citrix.com>