Re: [PATCH] Added call parameter to track whether invocation originated with guest or elsewhere
On Tue, Oct 23, 2012 at 07:56:54PM +, Auld, Will wrote: Having looked closer at the tacked of changing out the index and data fields in some function calls for a struct parameter with these and a originator field (host or guest) it is less attractive than I thought it would be. The only place where we need to know the initiator is in kvm_write_tsc() which has an implicit index. At the moment yes, but it might have other uses in the future. I have been trying to determine whether there is a possibility for taking a context switch while a guest initiated set_msr() is in progress whereby the new thread might invoke the set_msr()/kvm_write_tsc() routines. It looks to me like this is not possible but I can't be sure. It is not possible. If it is not possible we can set a variable for the vcpu when a guest call is in progress and this would be sufficient. What do you think? Thanks, The struct parameter seems the preferred choice as there might be other uses to this information in the future. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Added call parameter to track whether invocation originated with guest or elsewhere
Having looked closer at the tacked of changing out the index and data fields in
some
function calls for a struct parameter with these and a originator field (host
or guest)
it is less attractive than I thought it would be. The only place where we need
to know the initiator is in kvm_write_tsc() which has an implicit index.
I have been trying to determine whether there is a possibility for taking a
context switch while a guest initiated set_msr() is in progress whereby the new
thread might invoke the set_msr()/kvm_write_tsc() routines. It looks to me like
this is not possible but I can't be sure. If it is not possible we can set a
variable for the vcpu when a guest call is in progress and this would be
sufficient.
What do you think?
Thanks,
Will
-Original Message-
From: Will Auld [mailto:will.auld.in...@gmail.com]
Sent: Monday, October 22, 2012 2:58 PM
To: Avi Kivity
Cc: Marcelo Tosatti; Auld, Will; kvm@vger.kernel.org; Zhang, Xiantao;
Liu, Jinsong
Subject: Re: [PATCH] Added call parameter to track whether invocation
originated with guest or elsewhere
On Wed, 2012-10-17 at 16:28 +0200, Avi Kivity wrote:
On 10/17/2012 04:09 PM, Marcelo Tosatti wrote:
On Wed, Oct 17, 2012 at 12:35:33PM +0200, Avi Kivity wrote:
On 10/17/2012 04:10 AM, Will Auld wrote:
Signed-off-by: Will Auld will.a...@intel.com
---
Resending to full list
Marcelo,
This patch is what I believe you ask for as foundational for
later patches to address IA32_TSC_ADJUST.
Please write a changelog to reflect the motivation.
All those bool parameters scattered all over the place aren't very
pretty. Usually we solve this with helpers that embed the
parameter name (kvm_set_msr() vs. kvm_set_msr_host()) but there
are
too many functions for this to work here.
Marcelo, any ideas?
Its easier to read
kvm_x86_ops-kvm_set_msr()
kvm_x86_ops-kvm_set_msr_host()
then
kvm_x86_ops-kvm_set_msr(,false)
kvm_x86_ops-kvm_set_msr(,true)
So you're right.
Yes, but we have a million functions for setting MSRs.
Maybe
struct msr {
bool host_requested;
u32 index;
u64 data;
};
and change all the APIs to use that.
I was working on a different solution and then saw this suggestion just
now. I like this tact and will look at it closer.
Thanks,
Will
Re: [PATCH] Added call parameter to track whether invocation originated with guest or elsewhere
On 10/17/2012 04:10 AM, Will Auld wrote:
Signed-off-by: Will Auld will.a...@intel.com
---
Resending to full list
Marcelo,
This patch is what I believe you ask for as foundational for later
patches to address IA32_TSC_ADJUST.
Please write a changelog to reflect the motivation.
All those bool parameters scattered all over the place aren't very
pretty. Usually we solve this with helpers that embed the parameter
name (kvm_set_msr() vs. kvm_set_msr_host()) but there are too many
functions for this to work here.
Marcelo, any ideas?
Thanks,
Will
arch/x86/include/asm/kvm_host.h | 8
arch/x86/kvm/svm.c | 18 ++
arch/x86/kvm/vmx.c | 18 ++
arch/x86/kvm/x86.c | 18 ++
arch/x86/kvm/x86.h | 2 +-
5 files changed, 35 insertions(+), 29 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 09155d6..c06f0d1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -621,7 +621,7 @@ struct kvm_x86_ops {
void (*set_guest_debug)(struct kvm_vcpu *vcpu,
struct kvm_guest_debug *dbg);
int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata);
- int (*set_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
+ int (*set_msr)(struct kvm_vcpu *vcpu, u32 msr_index, u64 data, bool
guest_initiated);
u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);
void (*get_segment)(struct kvm_vcpu *vcpu,
struct kvm_segment *var, int seg);
@@ -684,7 +684,7 @@ struct kvm_x86_ops {
bool (*has_wbinvd_exit)(void);
void (*set_tsc_khz)(struct kvm_vcpu *vcpu, u32 user_tsc_khz, bool
scale);
- void (*write_tsc_offset)(struct kvm_vcpu *vcpu, u64 offset);
+ void (*write_tsc_offset)(struct kvm_vcpu *vcpu, u64 offset, bool
guest_initiated);
u64 (*compute_tsc_offset)(struct kvm_vcpu *vcpu, u64 target_tsc);
u64 (*read_l1_tsc)(struct kvm_vcpu *vcpu);
@@ -772,7 +772,7 @@ static inline int emulate_instruction(struct kvm_vcpu
*vcpu,
void kvm_enable_efer_bits(u64);
int kvm_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *data);
-int kvm_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
+int kvm_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data, bool
guest_initiated);
struct x86_emulate_ctxt;
@@ -799,7 +799,7 @@ void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db,
int *l);
int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata);
-int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data);
+int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool
guest_initiated);
unsigned long kvm_get_rflags(struct kvm_vcpu *vcpu);
void kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index baead95..424be27 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1012,7 +1012,8 @@ static void svm_set_tsc_khz(struct kvm_vcpu *vcpu, u32
user_tsc_khz, bool scale)
svm-tsc_ratio = ratio;
}
-static void svm_write_tsc_offset(struct kvm_vcpu *vcpu, u64 offset)
+static void svm_write_tsc_offset(struct kvm_vcpu *vcpu, u64 offset,
+ bool guest_initiated)
{
struct vcpu_svm *svm = to_svm(vcpu);
u64 g_tsc_offset = 0;
@@ -1255,7 +1256,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm
*kvm, unsigned int id)
svm-vmcb_pa = page_to_pfn(page) PAGE_SHIFT;
svm-asid_generation = 0;
init_vmcb(svm);
- kvm_write_tsc(svm-vcpu, 0);
+ kvm_write_tsc(svm-vcpu, 0, false /*Not Guest Initiated*/);
err = fx_init(svm-vcpu);
if (err)
@@ -3147,13 +3148,14 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64
data)
return 0;
}
-static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)
+static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data,
+ bool guest_initiated)
{
struct vcpu_svm *svm = to_svm(vcpu);
switch (ecx) {
case MSR_IA32_TSC:
- kvm_write_tsc(vcpu, data);
+ kvm_write_tsc(vcpu, data, guest_initiated);
break;
case MSR_STAR:
svm-vmcb-save.star = data;
@@ -3208,12 +3210,12 @@ static int svm_set_msr(struct kvm_vcpu *vcpu,
unsigned ecx, u64 data)
vcpu_unimpl(vcpu, unimplemented wrmsr: 0x%x data 0x%llx\n,
ecx, data);
break;
default:
- return kvm_set_msr_common(vcpu, ecx, data);
+ return kvm_set_msr_common(vcpu, ecx, data, guest_initiated);
}
return 0;
}
-static int wrmsr_interception(struct vcpu_svm *svm)
+static int wrmsr_interception(struct vcpu_svm *svm,
Re: [PATCH] Added call parameter to track whether invocation originated with guest or elsewhere
On Wed, Oct 17, 2012 at 12:35:33PM +0200, Avi Kivity wrote: On 10/17/2012 04:10 AM, Will Auld wrote: Signed-off-by: Will Auld will.a...@intel.com --- Resending to full list Marcelo, This patch is what I believe you ask for as foundational for later patches to address IA32_TSC_ADJUST. Please write a changelog to reflect the motivation. All those bool parameters scattered all over the place aren't very pretty. Usually we solve this with helpers that embed the parameter name (kvm_set_msr() vs. kvm_set_msr_host()) but there are too many functions for this to work here. Marcelo, any ideas? Its easier to read kvm_x86_ops-kvm_set_msr() kvm_x86_ops-kvm_set_msr_host() then kvm_x86_ops-kvm_set_msr(,false) kvm_x86_ops-kvm_set_msr(,true) So you're right. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Added call parameter to track whether invocation originated with guest or elsewhere
On 10/17/2012 04:09 PM, Marcelo Tosatti wrote:
On Wed, Oct 17, 2012 at 12:35:33PM +0200, Avi Kivity wrote:
On 10/17/2012 04:10 AM, Will Auld wrote:
Signed-off-by: Will Auld will.a...@intel.com
---
Resending to full list
Marcelo,
This patch is what I believe you ask for as foundational for later
patches to address IA32_TSC_ADJUST.
Please write a changelog to reflect the motivation.
All those bool parameters scattered all over the place aren't very
pretty. Usually we solve this with helpers that embed the parameter
name (kvm_set_msr() vs. kvm_set_msr_host()) but there are too many
functions for this to work here.
Marcelo, any ideas?
Its easier to read
kvm_x86_ops-kvm_set_msr()
kvm_x86_ops-kvm_set_msr_host()
then
kvm_x86_ops-kvm_set_msr(,false)
kvm_x86_ops-kvm_set_msr(,true)
So you're right.
Yes, but we have a million functions for setting MSRs.
Maybe
struct msr {
bool host_requested;
u32 index;
u64 data;
};
and change all the APIs to use that.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Added call parameter to track whether invocation originated with guest or elsewhere
OK, agreed it is not pretty. Thanks, Will -Original Message- From: Marcelo Tosatti [mailto:mtosa...@redhat.com] Sent: Wednesday, October 17, 2012 7:09 AM To: Avi Kivity Cc: Auld, Will; Will Auld; kvm@vger.kernel.org; Zhang, Xiantao; Liu, Jinsong Subject: Re: [PATCH] Added call parameter to track whether invocation originated with guest or elsewhere On Wed, Oct 17, 2012 at 12:35:33PM +0200, Avi Kivity wrote: On 10/17/2012 04:10 AM, Will Auld wrote: Signed-off-by: Will Auld will.a...@intel.com --- Resending to full list Marcelo, This patch is what I believe you ask for as foundational for later patches to address IA32_TSC_ADJUST. Please write a changelog to reflect the motivation. All those bool parameters scattered all over the place aren't very pretty. Usually we solve this with helpers that embed the parameter name (kvm_set_msr() vs. kvm_set_msr_host()) but there are too many functions for this to work here. Marcelo, any ideas? Its easier to read kvm_x86_ops-kvm_set_msr() kvm_x86_ops-kvm_set_msr_host() then kvm_x86_ops-kvm_set_msr(,false) kvm_x86_ops-kvm_set_msr(,true) So you're right. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
