Re: [Qemu-devel] [RESEND 1/2] kvmclock: use a light weight interface to update env->tsc.

[Paolo Bonzini <pbonzini@redhat.com>]

On 29/10/2015 09:22, Liang Li wrote:
> +int kvm_get_tsc(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +    struct {
> +        struct kvm_msrs info;
> +        struct kvm_msr_entry entries[1];
> +    } msr_data;
> +    struct kvm_msr_entry *msrs = msr_data.entries;
> +    int ret, i, n;
> +
> +    n = 0;
> +
> +    if (!env->tsc_valid) {
> +        msrs[n++].index = MSR_IA32_TSC;
> +        env->tsc_valid = !runstate_is_running();
> +    }
> +
> +    if (n == 0) {
> +        return 0;
> +    }
> +
> +    msr_data.info = (struct kvm_msrs) {
> +        .nmsrs = n,
> +    };
> +
> +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    for (i = 0; i < ret; i++) {
> +        uint32_t index = msrs[i].index;
> +        switch (index) {
> +        case MSR_IA32_TSC:
> +            env->tsc = msrs[i].data;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +

This can be simplified a bit:

int kvm_get_tsc(CPUState *cs)
{
    X86CPU *cpu = X86_CPU(cs);
    CPUX86State *env = &cpu->env;
    struct {
        struct kvm_msrs info;
        struct kvm_msr_entry entries[1];
    } msr_data;
    int ret;

    if (env->tsc_valid) {
        return 0;
    }

    msr_data.info.nmsrs = 1;
    msr_data.entries[0].index = MSR_IA32_TSC;
    env->tsc_valid = !runstate_is_running();

    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
    if (ret < 0) {
        return ret;
    }

    env->tsc = msr_data.entries[0].data;
    return 0;
}

> 
> +        CPU_FOREACH(cpu) {
> +            ret = kvm_get_tsc(cpu);
> +            if (ret < 0) {
> +                fprintf(stderr, "KVM_GET_MSRS failed: %s\n", strerror(ret));
> +                abort();
> +                return;
> +            }
> +        }


This should be run in the appropriate thread using run_on_cpu.  VCPU
ioctls should only be invoked from the VCPU thread.  So you should
introduce a new function kvm_synchronize_all_tsc() or something like that.

Otherwise, the idea behind the patches is fine.  Thanks!

Paolo