From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:51854)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1ZtGfF-00019n-QZ
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:04:54 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1ZtGfA-0007wQ-4u
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:04:53 -0500
Received: from mx1.redhat.com ([209.132.183.28]:57364)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1ZtGf9-0007wL-VT
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:04:48 -0500
References: <1446106927-15490-1-git-send-email-liang.z.li@intel.com>
	<1446106927-15490-2-git-send-email-liang.z.li@intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <56377B89.1050808@redhat.com>
Date: Mon, 2 Nov 2015 16:04:41 +0100
MIME-Version: 1.0
In-Reply-To: <1446106927-15490-2-git-send-email-liang.z.li@intel.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RESEND 1/2] kvmclock: use a light weight
 interface to update env->tsc.
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Liang Li <liang.z.li@intel.com>, qemu-devel@nongnu.org
Cc: mtosatti@redhat.com, m.gibula@beyond.pl



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