Re: [Qemu-devel] [PATCH 1/2] KVM: emulate lapic tsc deadline timer for guest

[Liu, Jinsong]

Marcelo Tosatti wrote:
>> diff --git a/arch/x86/include/asm/apicdef.h
>> b/arch/x86/include/asm/apicdef.h 
>> index 34595d5..3925d80 100644
>> --- a/arch/x86/include/asm/apicdef.h
>> +++ b/arch/x86/include/asm/apicdef.h
>> @@ -100,7 +100,9 @@
>>  #define             APIC_TIMER_BASE_CLKIN           0x0
>>  #define             APIC_TIMER_BASE_TMBASE          0x1
>>  #define             APIC_TIMER_BASE_DIV             0x2
>> +#define             APIC_LVT_TIMER_ONESHOT          (0 << 17)
>>  #define             APIC_LVT_TIMER_PERIODIC         (1 << 17)
>> +#define             APIC_LVT_TIMER_TSCDEADLINE      (2 << 17)
>>  #define             APIC_LVT_MASKED                 (1 << 16)
>>  #define             APIC_LVT_LEVEL_TRIGGER          (1 << 15)
>>  #define             APIC_LVT_REMOTE_IRR             (1 << 14)
> 
> Please have a separate, introductory patch for definitions that are
> not KVM specific.
> 

OK, will present a separate patch. BTW, will the separate patch still be send 
to address@hidden

>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -671,6 +671,8 @@ u8 kvm_get_guest_memory_type(struct kvm_vcpu
>> *vcpu, gfn_t gfn); 
>> 
>>  extern bool tdp_enabled;
>> 
>> +extern u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu);
>> +
> 
> No need for extern.
> 

Any special concern, or, for coding style? a little curious :)

>> +    } else if (apic_lvtt_tscdeadline(apic)) {
>> +            /* lapic timer in tsc deadline mode */
>> +            u64 guest_tsc, guest_tsc_delta, ns = 0;
>> +            struct kvm_vcpu *vcpu = apic->vcpu;
>> +            unsigned long this_tsc_khz = vcpu_tsc_khz(vcpu); +              
>> unsigned long
>> flags; +
>> +            if (unlikely(!apic->lapic_timer.tscdeadline || !this_tsc_khz))
>> +                    return; +
>> +            local_irq_save(flags);
>> +
>> +            now = apic->lapic_timer.timer.base->get_time();
>> +            kvm_get_msr(vcpu, MSR_IA32_TSC, &guest_tsc);
> 
> Use kvm_x86_ops->read_l1_tsc(vcpu) instead of direct MSR read
> (to avoid reading L2 guest TSC in case of nested virt).
> 

Fine. I use some old version kvm (Jul 22), and didn't notice Nadav's patch 
checked in Aug 2 with read_l1_tsc hook.
Thanks for tell me.

>> +            guest_tsc_delta = apic->lapic_timer.tscdeadline - guest_tsc;
> 
> if (guest_tsc <= tscdeadline), the timer should start immediately.
> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 6cb353c..a73c059 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -610,6 +610,16 @@ static void update_cpuid(struct kvm_vcpu *vcpu)
>>              if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE))
>>                      best->ecx |= bit(X86_FEATURE_OSXSAVE);
>>      }
>> +
>> +    /*
>> +     * When cpu has tsc deadline timer capacibility, use bit 17/18
>> +     * as timer mode mask. Otherwise only use bit 17. +      */
>> +    if (cpu_has_tsc_deadline_timer && best->function == 0x1) {
>> +            best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
>> +            vcpu->arch.apic->lapic_timer.timer_mode_mask = (3 << 17); +     
>> } else
>> +            vcpu->arch.apic->lapic_timer.timer_mode_mask = (1 << 17);
>>  }
> 
> The deadline timer is entirely emulated, whether the host CPU supports
> it or not is irrelevant.
> 
> Why was this changed from previous submissions?

Hmm, will explain in next email.

Thanks,
Jinsong