Re: [PATCH v2 1/2] x86/HVM: fix miscellaneous aspects of x2APIC emulation

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

[-- Attachment #1.1: Type: text/plain, Size: 4957 bytes --]

On 10/09/14 14:43, Jan Beulich wrote:
> - generate #GP on invalid APIC base MSR transitions
> - fail reads from the self-IPI register (which is write-only)
> - handle self-IPI writes and the ICR2 half of ICR writes largely in
>   hvm_x2apic_msr_write()
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Split from main patch.
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4499,7 +4499,8 @@ int hvm_msr_write_intercept(unsigned int
>          break;
>  
>      case MSR_IA32_APICBASE:
> -        vlapic_msr_set(vcpu_vlapic(v), msr_content);
> +        if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
> +            goto gp_fault;
>          break;
>  
>      case MSR_IA32_TSC_DEADLINE:
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -602,6 +602,7 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
>          break;
>  
>      case APIC_ICR2:
> +    case APIC_SELF_IPI:

APIC_EOI is also write-only, generates #GP(0) on on rdmsr, and isn't
caught by vlapic_read_aligned().

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>          return 1;
>      }
>  
> @@ -692,9 +693,7 @@ static int vlapic_reg_write(struct vcpu 
>          break;
>  
>      case APIC_SELF_IPI:
> -        rc = vlapic_x2apic_mode(vlapic)
> -            ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
> -            : X86EMUL_UNHANDLEABLE;
> +        rc = X86EMUL_UNHANDLEABLE;
>          break;
>  
>      case APIC_ICR:
> @@ -704,9 +703,7 @@ static int vlapic_reg_write(struct vcpu 
>          break;
>  
>      case APIC_ICR2:
> -        if ( !vlapic_x2apic_mode(vlapic) )
> -            val &= 0xff000000;
> -        vlapic_set_reg(vlapic, APIC_ICR2, val);
> +        vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000);
>          break;
>  
>      case APIC_LVTT:         /* LVT Timer Reg */
> @@ -865,16 +862,17 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>  
>      switch ( offset )
>      {
> -        int rc;
> -
>      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;
>      }
>  
>      return vlapic_reg_write(v, offset, (uint32_t)msr_content);
> @@ -893,10 +891,12 @@ const struct hvm_mmio_handler vlapic_mmi
>      .write_handler = vlapic_write
>  };
>  
> -void vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
> +bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
>  {
>      if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
>      {
> +        if ( unlikely(value & MSR_IA32_APICBASE_EXTD) )
> +            return 0;
>          if ( value & MSR_IA32_APICBASE_ENABLE )
>          {
>              vlapic_reset(vlapic);
> @@ -905,10 +905,15 @@ void vlapic_msr_set(struct vlapic *vlapi
>          }
>          else
>          {
> +            if ( unlikely(vlapic->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
> +                return 0;
>              vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
>              pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
>          }
>      }
> +    else if ( !(value & MSR_IA32_APICBASE_ENABLE) &&
> +              unlikely(value & MSR_IA32_APICBASE_EXTD) )
> +        return 0;
>  
>      vlapic->hw.apic_base_msr = value;
>  
> @@ -923,6 +928,8 @@ void vlapic_msr_set(struct vlapic *vlapi
>  
>      HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
>                  "apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
> +
> +    return 1;
>  }
>  
>  uint64_t  vlapic_tdt_msr_get(struct vlapic *vlapic)
> @@ -1206,6 +1213,10 @@ static int lapic_load_hidden(struct doma
>      if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) 
>          return -EINVAL;
>  
> +    if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
> +         unlikely(s->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
> +        return -EINVAL;
> +
>      vmx_vlapic_msr_changed(v);
>  
>      return 0;
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -106,7 +106,7 @@ void vlapic_destroy(struct vcpu *v);
>  
>  void vlapic_reset(struct vlapic *vlapic);
>  
> -void vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
> +bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
>  void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value);
>  uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
>  
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 5579 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel