diff mbox

[3/3] x86/vlapic: Reset LAPIC Timer only on TSC Deadline mode change

Message ID 20170323114701.25207-4-anthony.perard@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Anthony PERARD March 23, 2017, 11:47 a.m. UTC
For the LAPIC timer, switching between periodic and one-shot does not
reset anything on real-hardward, but switching from TSC deadline or to
it does reset the timer, according to Intel manual.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
I'm not sure that TMICT should be reset, but the manuel said that in
tsc-deadline, write to TMICT are ignored.
---
 xen/arch/x86/hvm/vlapic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Jan Beulich March 24, 2017, 9:54 a.m. UTC | #1
>>> On 23.03.17 at 12:47, <anthony.perard@citrix.com> wrote:
> For the LAPIC timer, switching between periodic and one-shot does not
> reset anything on real-hardward, but switching from TSC deadline or to
> it does reset the timer, according to Intel manual.

Oh, I see that I've overlooked this when reviewing patch 1 (I
was looking for the acronyms only) - please ignore the respective
comment there then.

> I'm not sure that TMICT should be reset, but the manuel said that in
> tsc-deadline, write to TMICT are ignored.

Writes being ignored to me suggests that TMICT simply retains its
value while in this mode. But you should be able to verify this with
parts of the XTF test code of yours run on bare hardware, shouldn't
you?

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -839,9 +839,11 @@ static void vlapic_reg_write(struct vcpu *v,
>          break;
>  
>      case APIC_LVTT:         /* LVT Timer Reg */
> -        if ( (vlapic_get_reg(vlapic, offset) & APIC_TIMER_MODE_MASK) !=
> -             (val & APIC_TIMER_MODE_MASK) )
> +        /* Switching between tdt and periodic|one-shot reset the other mode */
> +        if ( vlapic_lvtt_tdt(vlapic) !=
> +             ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE))
>          {
> +            vlapic_set_reg(vlapic, APIC_TMICT, 0);
>              vlapic->hw.tdt_msr = 0;
>          }

The change is quite different from what I would expect based on
the SDM information: Did you verify that the MSR is being cleared
only when changing from/to TDT (but neither when changing
TDT->TDT nor when changing non-TDT->non-TDT)? And according
to the comment above, I'd rather expect TMICT to be left alone
here, but instead reads to not return zero but the last value
written before changing to TDT (writes already look to be ignored
as specified).

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index f70a25f5b9..18247bd8bb 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -839,9 +839,11 @@  static void vlapic_reg_write(struct vcpu *v,
         break;
 
     case APIC_LVTT:         /* LVT Timer Reg */
-        if ( (vlapic_get_reg(vlapic, offset) & APIC_TIMER_MODE_MASK) !=
-             (val & APIC_TIMER_MODE_MASK) )
+        /* Switching between tdt and periodic|one-shot reset the other mode */
+        if ( vlapic_lvtt_tdt(vlapic) !=
+             ((val & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_TSC_DEADLINE))
         {
+            vlapic_set_reg(vlapic, APIC_TMICT, 0);
             vlapic->hw.tdt_msr = 0;
         }
         vlapic->pt.irq = val & APIC_VECTOR_MASK;