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

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

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 = 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 = APIC_ICR;
> +        msr_content = 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 = vcpu_vlapic(v);
>      unsigned long offset  = 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 = 83FH (self-IPI
MSR) can lead to an APIC-write VM exit
if the “virtual-interrupt delivery” 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