LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Avi Kivity <avi@redhat.com>
To: Glauber Costa <glommer@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Rik van Riel <riel@redhat.com>,
	Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v3 2/6] KVM-HV: KVM Steal time implementation
Date: Tue, 15 Feb 2011 16:34:19 +0200	[thread overview]
Message-ID: <4D5A8EEB.5030701@redhat.com> (raw)
In-Reply-To: <1297448364-14051-3-git-send-email-glommer@redhat.com>

On 02/11/2011 08:19 PM, Glauber Costa wrote:
> To implement steal time, we need the hypervisor to pass the guest information
> about how much time was spent running other processes outside the VM.
> This is per-vcpu, and using the kvmclock structure for that is an abuse
> we decided not to make.
>
> In this patchset, I am introducing a new msr, KVM_MSR_STEAL_TIME, that
> holds the memory area address containing information about steal time
>
> This patch contains the hypervisor part for it. I am keeping it separate from
> the headers to facilitate backports to people who wants to backport the kernel
> part but not the hypervisor, or the other way around.
>
>
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ffd7f8d..be6e0e2 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -377,6 +377,11 @@ struct kvm_vcpu_arch {
>   	unsigned int hw_tsc_khz;
>   	unsigned int time_offset;
>   	struct page *time_page;
> +
> +	gpa_t stime;
> +	struct kvm_steal_time steal;
> +	u64 this_time_out;
> +

Please put in a small sub-structure (or rename stime to something 
meaningful).

> @@ -1546,6 +1546,16 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>   		if (kvm_pv_enable_async_pf(vcpu, data))
>   			return 1;
>   		break;
> +	case MSR_KVM_STEAL_TIME:
> +
> +		if (!(data&  1)) {

Named constant.

> +			vcpu->arch.stime = 0;
> +			break;
> +		}
> +
> +		vcpu->arch.stime = data&  ~1;

I asked for 64-byte aligned structure, yes?  Need to fault if bits 1-5 
are set (to make sure the guest doesn't use an unadvertised feature and 
break itself in the future).

We might also want to fault if the cpuid bit isn't present.  We haven't 
done so in the past but it makes some sense.

> +		break;
> +
>   	case MSR_IA32_MCG_CTL:
>   	case MSR_IA32_MCG_STATUS:
>   	case MSR_IA32_MC0_CTL ... MSR_IA32_MC0_CTL + 4 * KVM_MAX_MCE_BANKS - 1:
> @@ -1831,6 +1841,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>   	case MSR_KVM_ASYNC_PF_EN:
>   		data = vcpu->arch.apf.msr_val;
>   		break;
> +	case MSR_KVM_STEAL_TIME:
> +		data = vcpu->arch.stime;
> +		break;

You are returning something other than the guest has written.

>   	case MSR_IA32_P5_MC_ADDR:
>   	case MSR_IA32_P5_MC_TYPE:
>   	case MSR_IA32_MCG_CAP:
>

> @@ -2108,6 +2122,9 @@ static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
>
>   void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   {
> +	struct kvm_steal_time *st;
> +	st = (struct kvm_steal_time *)vcpu->arch.stime;

You are converting a guest physical address into a host virtual 
address?  Truncating it in the process.

> +
>   	/* Address WBINVD may be executed by guest */
>   	if (need_emulate_wbinvd(vcpu)) {
>   		if (kvm_x86_ops->has_wbinvd_exit())
> @@ -2133,6 +2150,21 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   			kvm_migrate_timers(vcpu);
>   		vcpu->cpu = cpu;
>   	}
> +
> +	if (vcpu->arch.this_time_out) {
> +		u64 to = (get_kernel_ns() - vcpu->arch.this_time_out);
> +
> +		kvm_read_guest(vcpu->kvm, (gpa_t)st,&vcpu->arch.steal,
> +				sizeof(*st));

Now you are converting it back.  Also, you aren't checking the error 
return from kvm_read_guest().

> +
> +		vcpu->arch.steal.steal += to;
> +		vcpu->arch.steal.version += 2;
> +
> +		kvm_write_guest(vcpu->kvm, (gpa_t)st,&vcpu->arch.steal,
> +				sizeof(*st));

Error check.

> +		/* is it possible to have 2 loads in sequence? */

No.

> +		vcpu->arch.this_time_out = 0;
> +	}
>   }

kvm_arch_vcpu_put() is also executed when we return to userspace.  Do we 
want to account that as steal time?

Do we want to execute this code even if steal time isn't enabled?

Please put this into a separate function.

-- 
error compiling committee.c: too many arguments to function


  reply	other threads:[~2011-02-15 14:38 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-11 18:19 [PATCH v3 0/6] Steal time for KVM Glauber Costa
2011-02-11 18:19 ` [PATCH v3 1/6] KVM-HDR: KVM Steal time implementation Glauber Costa
2011-02-15 14:25   ` Avi Kivity
2011-02-11 18:19 ` [PATCH v3 2/6] KVM-HV: " Glauber Costa
2011-02-15 14:34   ` Avi Kivity [this message]
2011-02-11 18:19 ` [PATCH v3 3/6] KVM-GST: KVM Steal time accounting Glauber Costa
2011-02-11 19:05   ` Peter Zijlstra
2011-02-12 23:46     ` Glauber Costa
2011-02-15 14:35   ` Avi Kivity
2011-02-15 14:45     ` Peter Zijlstra
2011-02-15 15:17       ` Avi Kivity
2011-02-15 15:24         ` Rik van Riel
2011-02-15 15:26           ` Avi Kivity
2011-02-15 15:27         ` Peter Zijlstra
2011-02-11 18:19 ` [PATCH v3 4/6] KVM-GST: KVM Steal time registration Glauber Costa
2011-02-15 14:41   ` Avi Kivity
2011-02-15 15:48     ` Peter Zijlstra
2011-02-11 18:19 ` [PATCH v3 5/6] KVM-GST: adjust scheduler cpu power Glauber Costa
2011-02-11 19:05   ` Peter Zijlstra
2011-02-11 18:19 ` [PATCH v3 6/6] Describe KVM_MSR_STEAL_TIME Glauber Costa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D5A8EEB.5030701@redhat.com \
    --to=avi@redhat.com \
    --cc=glommer@redhat.com \
    --cc=jeremy.fitzhardinge@citrix.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=riel@redhat.com \
    --subject='Re: [PATCH v3 2/6] KVM-HV: KVM Steal time implementation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).