Re: [PATCH 3/5] vtdt: Modify vlapic code to add vtdt support

[Keir Fraser <keir@xen.org>]

On 14/12/2010 03:27, "Wei, Gang" <gang.wei@intel.com> wrote:

> @@ -643,7 +669,11 @@ static int vlapic_write(struct vcpu *v,
>          break;
>  
>      case APIC_LVTT:         /* LVT Timer Reg */
> +        destroy_periodic_time(&vlapic->pt);
>          vlapic->pt.irq = val & APIC_VECTOR_MASK;
> +        vlapic_set_reg(vlapic, APIC_TMICT, 0);
> +        vlapic_set_reg(vlapic, APIC_TMCCT, 0);
> +        vlapic->hw.tdt_msr = 0;

Writing any value to LVTT zaps TMICT,TMCCT,MSR_TDT? That seems pretty
unlikely to me! This obviously has effects on behaviour outside TDT
emulation as it affects TMICT/TMCCT emulation. Looks dangerous, as well as
wrong.

Also I now notice that this patch is not against tip of xen-unstable, as
these changes should be to new function vlapic_reg_write().

 -- Keir

>      case APIC_LVTTHMR:      /* LVT Thermal Monitor */
>      case APIC_LVTPC:        /* LVT Performance Counter */
>      case APIC_LVT0:         /* LVT LINT0 Reg */
> @@ -666,6 +696,9 @@ static int vlapic_write(struct vcpu *v,
>      {
>          uint64_t period;
>  
> +        if ( !vlapic_lvtt_oneshot(vlapic) && !vlapic_lvtt_period(vlapic) )
> +            break;
> +
>          vlapic_set_reg(vlapic, APIC_TMICT, val);
>          if ( val == 0 )
>          {
> @@ -746,6 +779,73 @@ 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);
> +}
> +
> +uint64_t  vlapic_tdt_msr_get(struct vlapic *vlapic)
> +{
> +    if ( !vlapic_lvtt_tdt(vlapic) )
> +        return 0;
> +
> +    return vlapic->hw.tdt_msr;
> +}
> +
> +void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value)
> +{
> +    uint64_t guest_tsc;
> +    uint64_t guest_time;
> +    struct vcpu *v = vlapic_vcpu(vlapic);
> +
> +    /* may need to exclude some other conditions like vlapic->hw.disabled */
> +    if ( !vlapic_lvtt_tdt(vlapic) )
> +    {
> +        HVM_DBG_LOG(DBG_LEVEL_VLAPIC_TIMER, "ignore tsc deadline msr write");
> +        return;
> +    }
> +    
> +    /* new_value = 0, >0 && <= now, > now */
> +    guest_tsc = hvm_get_guest_tsc(v);
> +    guest_time = hvm_get_guest_time(v);
> +    if ( value > guest_tsc )
> +    {
> +        uint64_t delta = value - v->arch.hvm_vcpu.cache_tsc_offset;
> +        delta = gtsc_to_gtime(v->domain, delta);
> +        delta = max_t(s64, delta - guest_time, 0);
> +
> +        HVM_DBG_LOG(DBG_LEVEL_VLAPIC_TIMER, "delta[0x%016"PRIx64"]", delta);
> +
> +        vlapic->hw.tdt_msr = value;
> +        /* .... reprogram tdt timer */
> +        create_periodic_time(v, &vlapic->pt, delta, 0,
> +                             vlapic->pt.irq, vlapic_tdt_pt_cb,
> +                             &vlapic->timer_last_update);
> +        vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
> +    }
> +    else
> +    {
> +        vlapic->hw.tdt_msr = 0;
> +
> +        /* trigger a timer event if needed */
> +        if ( value > 0 )
> +        {
> +            create_periodic_time(v, &vlapic->pt, 0, 0,
> +                                 vlapic->pt.irq, vlapic_tdt_pt_cb,
> +                                 &vlapic->timer_last_update);
> +            vlapic->timer_last_update = vlapic->pt.last_plt_gtime;
> +        }
> +        else
> +        {
> +            /* .... stop tdt timer */
> +            destroy_periodic_time(&vlapic->pt);
> +        }
> +
> +        HVM_DBG_LOG(DBG_LEVEL_VLAPIC_TIMER, "value[0x%016"PRIx64"]", value);
> +    }
> +
> +    HVM_DBG_LOG(DBG_LEVEL_VLAPIC_TIMER,
> +                "tdt_msr[0x%016"PRIx64"],"
> +                " gtsc[0x%016"PRIx64"],"
> +                " gtime[0x%016"PRIx64"]",
> +                vlapic->hw.tdt_msr, guest_tsc, guest_time);
>  }
>  
>  static int __vlapic_accept_pic_intr(struct vcpu *v)
> @@ -860,6 +960,17 @@ void vlapic_reset(struct vlapic *vlapic)
>      destroy_periodic_time(&vlapic->pt);
>  }
>  
> +static void lapic_tdt_rearm(struct vlapic *s)
> +{
> +    uint64_t tdt_msr = vlapic_tdt_msr_get(s);
> +
> +    s->pt.irq = vlapic_get_reg(s, APIC_LVTT) & APIC_VECTOR_MASK;
> +    if ( tdt_msr == 0)
> +       return;
> +
> +    vlapic_tdt_msr_set(s, tdt_msr);
> +}
> +
>  /* rearm the actimer if needed, after a HVM restore */
>  static void lapic_rearm(struct vlapic *s)
>  {
> @@ -953,7 +1064,10 @@ static int lapic_load_regs(struct domain
>          return -EINVAL;
>  
>      vlapic_adjust_i8259_target(d);
> -    lapic_rearm(s);
> +    if ( vlapic_lvtt_tdt(s) )
> +        lapic_tdt_rearm(s);
> +    else
> +        lapic_rearm(s);
>      return 0;
>  }
>  
> diff -r d042ca4c6b68 xen/include/asm-x86/hvm/vlapic.h
> --- a/xen/include/asm-x86/hvm/vlapic.h Thu Dec 09 22:33:02 2010 +0800
> +++ b/xen/include/asm-x86/hvm/vlapic.h Thu Dec 09 22:33:06 2010 +0800
> @@ -90,6 +90,8 @@ void vlapic_reset(struct vlapic *vlapic)
>  void vlapic_reset(struct vlapic *vlapic);
>  
>  void 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);
>  
>  int vlapic_accept_pic_intr(struct vcpu *v);
>  
> diff -r d042ca4c6b68 xen/include/public/arch-x86/hvm/save.h
> --- a/xen/include/public/arch-x86/hvm/save.h Thu Dec 09 22:33:02 2010 +0800
> +++ b/xen/include/public/arch-x86/hvm/save.h Thu Dec 09 22:33:06 2010 +0800
> @@ -265,6 +265,7 @@ struct hvm_hw_lapic {
>      uint64_t             apic_base_msr;
>      uint32_t             disabled; /* VLAPIC_xx_DISABLED */
>      uint32_t             timer_divisor;
> +    uint64_t             tdt_msr;
>  };
>  
>  DECLARE_HVM_SAVE_TYPE(LAPIC, 5, struct hvm_hw_lapic);
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel