Re: [PATCH V3 1/2] Resend - Add code to track call origin for msr assignment.
On Mon, Nov 26, 2012 at 07:42:28PM +, Auld, Will wrote:
> Gleb, Marcelo,
>
> Sorry Gleb, I did not see comments from you but I have now found them. In
> doing so I also found one form Marcelo that I missed.
>
> What I believe is now outstanding to be addressed are:
> >From Gleb:
> - You've changed function pointer signature here, but emulator_set_msr()
> remained the same
> - Also I would prefer adding host_initiated parameter to kvm_set_msr()
> instead of introducing msr_data structure.
>
> >From Marcelo:
> - false, this is guest instruction emulation
>
> I will address these points. However Gleb, your second item above,
> host_initiated parameter was implemented but then rejected agreeing that the
> msr_data structure would be a better solution. This was base on discussion
> with both Avi and Marcelo. I will leave this as is.
>
OK. Thanks.
> Thanks,
>
> Will
>
> > -Original Message-
> > From: Gleb Natapov [mailto:g...@redhat.com]
> > Sent: Monday, November 26, 2012 10:47 AM
> > To: Auld, Will
> > Cc: qemu-devel; mtosa...@redhat.com; kvm@vger.kernel.org; Dugger,
> > Donald D; Liu, Jinsong; Zhang, Xiantao; a...@redhat.com
> > Subject: Re: [PATCH V3 1/2] Resend - Add code to track call origin for
> > msr assignment.
> >
> > Comments are still not addressed.
> >
> > On Mon, Nov 26, 2012 at 10:40:51AM -0800, Will Auld wrote:
> > > In order to track who initiated the call (host or guest) to modify an
> > > msr value I have changed function call parameters along the call
> > path.
> > > The specific change is to add a struct pointer parameter that points
> > > to (index, data, caller) information rather than having this
> > > information passed as individual parameters.
> > >
> > > The initial use for this capability is for updating the
> > > IA32_TSC_ADJUST msr while setting the tsc value. It is anticipated
> > > that this capability is useful other tasks.
> > >
> > > Signed-off-by: Will Auld
> > > ---
> > > arch/x86/include/asm/kvm_host.h | 12 +---
> > > arch/x86/kvm/svm.c | 21 +++--
> > > arch/x86/kvm/vmx.c | 24 +---
> > > arch/x86/kvm/x86.c | 23 +--
> > > arch/x86/kvm/x86.h | 2 +-
> > > 5 files changed, 59 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > b/arch/x86/include/asm/kvm_host.h index 09155d6..da34027 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -598,6 +598,12 @@ struct kvm_vcpu_stat {
> > >
> > > struct x86_instruction_info;
> > >
> > > +struct msr_data {
> > > +bool host_initiated;
> > > +u32 index;
> > > +u64 data;
> > > +};
> > > +
> > > struct kvm_x86_ops {
> > > int (*cpu_has_kvm_support)(void); /* __init */
> > > int (*disabled_by_bios)(void); /* __init */
> > > @@ -621,7 +627,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, struct msr_data *msr);
> > > u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);
> > > void (*get_segment)(struct kvm_vcpu *vcpu,
> > > struct kvm_segment *var, int seg); @@ -772,7
> > +778,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, struct msr_data *msr);
> > >
> > > struct x86_emulate_ctxt;
> > >
> > > @@ -799,7 +805,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, struct msr_data *msr);
> > >
&
RE: [PATCH V3 1/2] Resend - Add code to track call origin for msr assignment.
Gleb, Marcelo,
Sorry Gleb, I did not see comments from you but I have now found them. In doing
so I also found one form Marcelo that I missed.
What I believe is now outstanding to be addressed are:
>From Gleb:
- You've changed function pointer signature here, but emulator_set_msr()
remained the same
- Also I would prefer adding host_initiated parameter to kvm_set_msr() instead
of introducing msr_data structure.
>From Marcelo:
- false, this is guest instruction emulation
I will address these points. However Gleb, your second item above,
host_initiated parameter was implemented but then rejected agreeing that the
msr_data structure would be a better solution. This was base on discussion with
both Avi and Marcelo. I will leave this as is.
Thanks,
Will
> -Original Message-
> From: Gleb Natapov [mailto:g...@redhat.com]
> Sent: Monday, November 26, 2012 10:47 AM
> To: Auld, Will
> Cc: qemu-devel; mtosa...@redhat.com; kvm@vger.kernel.org; Dugger,
> Donald D; Liu, Jinsong; Zhang, Xiantao; a...@redhat.com
> Subject: Re: [PATCH V3 1/2] Resend - Add code to track call origin for
> msr assignment.
>
> Comments are still not addressed.
>
> On Mon, Nov 26, 2012 at 10:40:51AM -0800, Will Auld wrote:
> > In order to track who initiated the call (host or guest) to modify an
> > msr value I have changed function call parameters along the call
> path.
> > The specific change is to add a struct pointer parameter that points
> > to (index, data, caller) information rather than having this
> > information passed as individual parameters.
> >
> > The initial use for this capability is for updating the
> > IA32_TSC_ADJUST msr while setting the tsc value. It is anticipated
> > that this capability is useful other tasks.
> >
> > Signed-off-by: Will Auld
> > ---
> > arch/x86/include/asm/kvm_host.h | 12 +---
> > arch/x86/kvm/svm.c | 21 +++--
> > arch/x86/kvm/vmx.c | 24 +---
> > arch/x86/kvm/x86.c | 23 +--
> > arch/x86/kvm/x86.h | 2 +-
> > 5 files changed, 59 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h
> > b/arch/x86/include/asm/kvm_host.h index 09155d6..da34027 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -598,6 +598,12 @@ struct kvm_vcpu_stat {
> >
> > struct x86_instruction_info;
> >
> > +struct msr_data {
> > +bool host_initiated;
> > +u32 index;
> > +u64 data;
> > +};
> > +
> > struct kvm_x86_ops {
> > int (*cpu_has_kvm_support)(void); /* __init */
> > int (*disabled_by_bios)(void); /* __init */
> > @@ -621,7 +627,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, struct msr_data *msr);
> > u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);
> > void (*get_segment)(struct kvm_vcpu *vcpu,
> > struct kvm_segment *var, int seg); @@ -772,7
> +778,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, struct msr_data *msr);
> >
> > struct x86_emulate_ctxt;
> >
> > @@ -799,7 +805,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, struct msr_data *msr);
> >
> > 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..5ac11f0
> > 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -1211,6 +1211,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct
> kvm *kvm, unsigned int id)
> > struct page *msrpm_pages;
> > struct page *hsave_page;
> > struct page *nested_msrpm_pages;
Re: [PATCH V3 1/2] Resend - Add code to track call origin for msr assignment.
Comments are still not addressed.
On Mon, Nov 26, 2012 at 10:40:51AM -0800, Will Auld wrote:
> In order to track who initiated the call (host or guest) to modify an msr
> value I have changed function call parameters along the call path. The
> specific change is to add a struct pointer parameter that points to (index,
> data, caller) information rather than having this information passed as
> individual parameters.
>
> The initial use for this capability is for updating the IA32_TSC_ADJUST
> msr while setting the tsc value. It is anticipated that this capability
> is useful other tasks.
>
> Signed-off-by: Will Auld
> ---
> arch/x86/include/asm/kvm_host.h | 12 +---
> arch/x86/kvm/svm.c | 21 +++--
> arch/x86/kvm/vmx.c | 24 +---
> arch/x86/kvm/x86.c | 23 +--
> arch/x86/kvm/x86.h | 2 +-
> 5 files changed, 59 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 09155d6..da34027 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -598,6 +598,12 @@ struct kvm_vcpu_stat {
>
> struct x86_instruction_info;
>
> +struct msr_data {
> +bool host_initiated;
> +u32 index;
> +u64 data;
> +};
> +
> struct kvm_x86_ops {
> int (*cpu_has_kvm_support)(void); /* __init */
> int (*disabled_by_bios)(void); /* __init */
> @@ -621,7 +627,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, struct msr_data *msr);
> u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);
> void (*get_segment)(struct kvm_vcpu *vcpu,
> struct kvm_segment *var, int seg);
> @@ -772,7 +778,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, struct msr_data *msr);
>
> struct x86_emulate_ctxt;
>
> @@ -799,7 +805,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, struct msr_data *msr);
>
> 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..5ac11f0 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1211,6 +1211,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm
> *kvm, unsigned int id)
> struct page *msrpm_pages;
> struct page *hsave_page;
> struct page *nested_msrpm_pages;
> + struct msr_data msr;
> int err;
>
> svm = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> @@ -1255,7 +1256,10 @@ 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);
> + msr.data = 0x0;
> + msr.index = MSR_IA32_TSC;
> + msr.host_initiated = true;
> + kvm_write_tsc(&svm->vcpu, &msr);
>
> err = fx_init(&svm->vcpu);
> if (err)
> @@ -3147,13 +3151,15 @@ 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, struct msr_data *msr)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
>
> + u32 ecx = msr->index;
> + u64 data = msr->data;
> switch (ecx) {
> case MSR_IA32_TSC:
> - kvm_write_tsc(vcpu, data);
> + kvm_write_tsc(vcpu, msr);
> break;
> case MSR_STAR:
> svm->vmcb->save.star = data;
> @@ -3208,20 +3214,23 @@ 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, msr);
> }
> return 0;
> }
>
> static int wrmsr_interception(struct vcpu_svm *svm)
> {
> + struct msr_data msr;
> u32 ecx = svm->vcpu.arch.regs[VCPU_REGS_RCX];
> u64 data = (svm->vcpu.arch.regs[VCPU_REGS_RA
[PATCH V3 1/2] Resend - Add code to track call origin for msr assignment.
In order to track who initiated the call (host or guest) to modify an msr
value I have changed function call parameters along the call path. The
specific change is to add a struct pointer parameter that points to (index,
data, caller) information rather than having this information passed as
individual parameters.
The initial use for this capability is for updating the IA32_TSC_ADJUST
msr while setting the tsc value. It is anticipated that this capability
is useful other tasks.
Signed-off-by: Will Auld
---
arch/x86/include/asm/kvm_host.h | 12 +---
arch/x86/kvm/svm.c | 21 +++--
arch/x86/kvm/vmx.c | 24 +---
arch/x86/kvm/x86.c | 23 +--
arch/x86/kvm/x86.h | 2 +-
5 files changed, 59 insertions(+), 23 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 09155d6..da34027 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -598,6 +598,12 @@ struct kvm_vcpu_stat {
struct x86_instruction_info;
+struct msr_data {
+bool host_initiated;
+u32 index;
+u64 data;
+};
+
struct kvm_x86_ops {
int (*cpu_has_kvm_support)(void); /* __init */
int (*disabled_by_bios)(void); /* __init */
@@ -621,7 +627,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, struct msr_data *msr);
u64 (*get_segment_base)(struct kvm_vcpu *vcpu, int seg);
void (*get_segment)(struct kvm_vcpu *vcpu,
struct kvm_segment *var, int seg);
@@ -772,7 +778,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, struct msr_data *msr);
struct x86_emulate_ctxt;
@@ -799,7 +805,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, struct msr_data *msr);
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..5ac11f0 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1211,6 +1211,7 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm,
unsigned int id)
struct page *msrpm_pages;
struct page *hsave_page;
struct page *nested_msrpm_pages;
+ struct msr_data msr;
int err;
svm = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
@@ -1255,7 +1256,10 @@ 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);
+ msr.data = 0x0;
+ msr.index = MSR_IA32_TSC;
+ msr.host_initiated = true;
+ kvm_write_tsc(&svm->vcpu, &msr);
err = fx_init(&svm->vcpu);
if (err)
@@ -3147,13 +3151,15 @@ 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, struct msr_data *msr)
{
struct vcpu_svm *svm = to_svm(vcpu);
+ u32 ecx = msr->index;
+ u64 data = msr->data;
switch (ecx) {
case MSR_IA32_TSC:
- kvm_write_tsc(vcpu, data);
+ kvm_write_tsc(vcpu, msr);
break;
case MSR_STAR:
svm->vmcb->save.star = data;
@@ -3208,20 +3214,23 @@ 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, msr);
}
return 0;
}
static int wrmsr_interception(struct vcpu_svm *svm)
{
+ struct msr_data msr;
u32 ecx = svm->vcpu.arch.regs[VCPU_REGS_RCX];
u64 data = (svm->vcpu.arch.regs[VCPU_REGS_RAX] & -1u)
| ((u64)(svm->vcpu.arch.regs[VCPU_REGS_RDX] & -1u) << 32);
-
+ msr.data = data;
+ msr.index = ecx;
+ msr.host_initiated = false;
svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
- if (svm_set_msr(&s
