* [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs
@ 2016-03-30 20:59 Paolo Bonzini
2016-03-31 13:01 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2016-03-30 20:59 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost
This would have caught the bug in the previous patch.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target-i386/kvm.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 19e2d94..799fdfa 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -141,6 +141,7 @@ static int kvm_get_tsc(CPUState *cs)
return ret;
}
+ assert(ret == 1);
env->tsc = msr_data.entries[0].data;
return 0;
}
@@ -1446,6 +1447,7 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
struct kvm_msr_entry entries[1];
} msr_data;
struct kvm_msr_entry *msrs = msr_data.entries;
+ int ret;
if (!has_msr_tsc_deadline) {
return 0;
@@ -1457,7 +1459,13 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
.nmsrs = 1,
};
- return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
+ ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
+ if (ret < 0) {
+ return ret;
+ }
+
+ assert(ret == 1);
+ return 0;
}
/*
@@ -1472,6 +1480,11 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
struct kvm_msrs info;
struct kvm_msr_entry entry;
} msr_data;
+ int ret;
+
+ if (!has_msr_feature_control) {
+ return 0;
+ }
kvm_msr_entry_set(&msr_data.entry, MSR_IA32_FEATURE_CONTROL,
cpu->env.msr_ia32_feature_control);
@@ -1480,7 +1493,13 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
.nmsrs = 1,
};
- return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
+ ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
+ if (ret < 0) {
+ return ret;
+ }
+
+ assert(ret == 1);
+ return 0;
}
static int kvm_put_msrs(X86CPU *cpu, int level)
@@ -1492,6 +1511,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
} msr_data;
struct kvm_msr_entry *msrs = msr_data.entries;
int n = 0, i;
+ int ret;
kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS, env->sysenter_cs);
kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
@@ -1685,8 +1705,13 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
.nmsrs = n,
};
- return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
+ ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
+ if (ret < 0) {
+ return ret;
+ }
+ assert(ret == n);
+ return 0;
}
@@ -2055,6 +2080,7 @@ static int kvm_get_msrs(X86CPU *cpu)
return ret;
}
+ assert(ret == n);
for (i = 0; i < ret; i++) {
uint32_t index = msrs[i].index;
switch (index) {
@@ -2511,7 +2537,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
- if (level >= KVM_PUT_RESET_STATE && has_msr_feature_control) {
+ if (level >= KVM_PUT_RESET_STATE) {
ret = kvm_put_msr_feature_control(x86_cpu);
if (ret < 0) {
return ret;
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs
2016-03-30 20:59 [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs Paolo Bonzini
@ 2016-03-31 13:01 ` Laszlo Ersek
2016-03-31 13:03 ` Paolo Bonzini
2016-03-31 16:51 ` Eduardo Habkost
0 siblings, 2 replies; 4+ messages in thread
From: Laszlo Ersek @ 2016-03-31 13:01 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: ehabkost
On 03/30/16 22:59, Paolo Bonzini wrote:
> This would have caught the bug in the previous patch.
Should this patch share a series with
<http://thread.gmane.org/gmane.comp.emulators.qemu/404245>? Otherwise
they could be separated by other patches in the commit history, and then
"previous patch" would be misleading.
(Alternatively, the reference to "previous patch" could be made by subject.)
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target-i386/kvm.c | 34 ++++++++++++++++++++++++++++++----
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 19e2d94..799fdfa 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -141,6 +141,7 @@ static int kvm_get_tsc(CPUState *cs)
> return ret;
> }
>
> + assert(ret == 1);
> env->tsc = msr_data.entries[0].data;
> return 0;
> }
> @@ -1446,6 +1447,7 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
> struct kvm_msr_entry entries[1];
> } msr_data;
> struct kvm_msr_entry *msrs = msr_data.entries;
> + int ret;
>
> if (!has_msr_tsc_deadline) {
> return 0;
> @@ -1457,7 +1459,13 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
> .nmsrs = 1,
> };
>
> - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + assert(ret == 1);
> + return 0;
> }
This changes the return value of kvm_put_tscdeadline_msr() -- and
friends below -- for successful invocations. I guess that's fine, but a
note about it in the commit message would be nice.
Anyway, I'm not an "expert" in this area, so the best I can offer for
this two-part (almost-) series, with the commit message nits fixed, is
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
>
> /*
> @@ -1472,6 +1480,11 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
> struct kvm_msrs info;
> struct kvm_msr_entry entry;
> } msr_data;
> + int ret;
> +
> + if (!has_msr_feature_control) {
> + return 0;
> + }
>
> kvm_msr_entry_set(&msr_data.entry, MSR_IA32_FEATURE_CONTROL,
> cpu->env.msr_ia32_feature_control);
> @@ -1480,7 +1493,13 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
> .nmsrs = 1,
> };
>
> - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + assert(ret == 1);
> + return 0;
> }
>
> static int kvm_put_msrs(X86CPU *cpu, int level)
> @@ -1492,6 +1511,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> } msr_data;
> struct kvm_msr_entry *msrs = msr_data.entries;
> int n = 0, i;
> + int ret;
>
> kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS, env->sysenter_cs);
> kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
> @@ -1685,8 +1705,13 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> .nmsrs = n,
> };
>
> - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> + if (ret < 0) {
> + return ret;
> + }
>
> + assert(ret == n);
> + return 0;
> }
>
>
> @@ -2055,6 +2080,7 @@ static int kvm_get_msrs(X86CPU *cpu)
> return ret;
> }
>
> + assert(ret == n);
> for (i = 0; i < ret; i++) {
> uint32_t index = msrs[i].index;
> switch (index) {
> @@ -2511,7 +2537,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>
> assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
>
> - if (level >= KVM_PUT_RESET_STATE && has_msr_feature_control) {
> + if (level >= KVM_PUT_RESET_STATE) {
> ret = kvm_put_msr_feature_control(x86_cpu);
> if (ret < 0) {
> return ret;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs
2016-03-31 13:01 ` Laszlo Ersek
@ 2016-03-31 13:03 ` Paolo Bonzini
2016-03-31 16:51 ` Eduardo Habkost
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2016-03-31 13:03 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: ehabkost
On 31/03/2016 15:01, Laszlo Ersek wrote:
> On 03/30/16 22:59, Paolo Bonzini wrote:
>> This would have caught the bug in the previous patch.
>
> Should this patch share a series with
> <http://thread.gmane.org/gmane.comp.emulators.qemu/404245>?
They need not, but indeed the commit message has to be adjusted (unless
I send both of them in the same pull request, and then they effectively
become 1/2 and 2/2).
Paolo
> Otherwise
> they could be separated by other patches in the commit history, and then
> "previous patch" would be misleading.
>
> (Alternatively, the reference to "previous patch" could be made by subject.)
>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> target-i386/kvm.c | 34 ++++++++++++++++++++++++++++++----
>> 1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 19e2d94..799fdfa 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -141,6 +141,7 @@ static int kvm_get_tsc(CPUState *cs)
>> return ret;
>> }
>>
>> + assert(ret == 1);
>> env->tsc = msr_data.entries[0].data;
>> return 0;
>> }
>> @@ -1446,6 +1447,7 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
>> struct kvm_msr_entry entries[1];
>> } msr_data;
>> struct kvm_msr_entry *msrs = msr_data.entries;
>> + int ret;
>>
>> if (!has_msr_tsc_deadline) {
>> return 0;
>> @@ -1457,7 +1459,13 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
>> .nmsrs = 1,
>> };
>>
>> - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
>> + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + assert(ret == 1);
>> + return 0;
>> }
>
> This changes the return value of kvm_put_tscdeadline_msr() -- and
> friends below -- for successful invocations. I guess that's fine, but a
> note about it in the commit message would be nice.
>
> Anyway, I'm not an "expert" in this area, so the best I can offer for
> this two-part (almost-) series, with the commit message nits fixed, is
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
>
>>
>> /*
>> @@ -1472,6 +1480,11 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
>> struct kvm_msrs info;
>> struct kvm_msr_entry entry;
>> } msr_data;
>> + int ret;
>> +
>> + if (!has_msr_feature_control) {
>> + return 0;
>> + }
>>
>> kvm_msr_entry_set(&msr_data.entry, MSR_IA32_FEATURE_CONTROL,
>> cpu->env.msr_ia32_feature_control);
>> @@ -1480,7 +1493,13 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
>> .nmsrs = 1,
>> };
>>
>> - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
>> + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + assert(ret == 1);
>> + return 0;
>> }
>>
>> static int kvm_put_msrs(X86CPU *cpu, int level)
>> @@ -1492,6 +1511,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>> } msr_data;
>> struct kvm_msr_entry *msrs = msr_data.entries;
>> int n = 0, i;
>> + int ret;
>>
>> kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS, env->sysenter_cs);
>> kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
>> @@ -1685,8 +1705,13 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>> .nmsrs = n,
>> };
>>
>> - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
>> + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
>> + if (ret < 0) {
>> + return ret;
>> + }
>>
>> + assert(ret == n);
>> + return 0;
>> }
>>
>>
>> @@ -2055,6 +2080,7 @@ static int kvm_get_msrs(X86CPU *cpu)
>> return ret;
>> }
>>
>> + assert(ret == n);
>> for (i = 0; i < ret; i++) {
>> uint32_t index = msrs[i].index;
>> switch (index) {
>> @@ -2511,7 +2537,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>>
>> assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
>>
>> - if (level >= KVM_PUT_RESET_STATE && has_msr_feature_control) {
>> + if (level >= KVM_PUT_RESET_STATE) {
>> ret = kvm_put_msr_feature_control(x86_cpu);
>> if (ret < 0) {
>> return ret;
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs
2016-03-31 13:01 ` Laszlo Ersek
2016-03-31 13:03 ` Paolo Bonzini
@ 2016-03-31 16:51 ` Eduardo Habkost
1 sibling, 0 replies; 4+ messages in thread
From: Eduardo Habkost @ 2016-03-31 16:51 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Paolo Bonzini, qemu-devel
On Thu, Mar 31, 2016 at 03:01:29PM +0200, Laszlo Ersek wrote:
> On 03/30/16 22:59, Paolo Bonzini wrote:
> > This would have caught the bug in the previous patch.
>
> Should this patch share a series with
> <http://thread.gmane.org/gmane.comp.emulators.qemu/404245>? Otherwise
> they could be separated by other patches in the commit history, and then
> "previous patch" would be misleading.
>
> (Alternatively, the reference to "previous patch" could be made by subject.)
>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > target-i386/kvm.c | 34 ++++++++++++++++++++++++++++++----
> > 1 file changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 19e2d94..799fdfa 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -141,6 +141,7 @@ static int kvm_get_tsc(CPUState *cs)
> > return ret;
> > }
> >
> > + assert(ret == 1);
> > env->tsc = msr_data.entries[0].data;
> > return 0;
> > }
> > @@ -1446,6 +1447,7 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
> > struct kvm_msr_entry entries[1];
> > } msr_data;
> > struct kvm_msr_entry *msrs = msr_data.entries;
> > + int ret;
> >
> > if (!has_msr_tsc_deadline) {
> > return 0;
> > @@ -1457,7 +1459,13 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
> > .nmsrs = 1,
> > };
> >
> > - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > +
> > + assert(ret == 1);
> > + return 0;
> > }
>
> This changes the return value of kvm_put_tscdeadline_msr() -- and
> friends below -- for successful invocations. I guess that's fine, but a
> note about it in the commit message would be nice.
All these functions have only one caller each, that only checks
if ret < 0.
(As they are all static functions with a single caller in
target-i386/kvm.c, I don't mind if this is not mentioned in the
commit message.)
>
> Anyway, I'm not an "expert" in this area, so the best I can offer for
> this two-part (almost-) series, with the commit message nits fixed, is
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
>
> >
> > /*
> > @@ -1472,6 +1480,11 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
> > struct kvm_msrs info;
> > struct kvm_msr_entry entry;
> > } msr_data;
> > + int ret;
> > +
> > + if (!has_msr_feature_control) {
> > + return 0;
> > + }
This is not strictly needed to implement what's described in the
commit message, but it makes kvm_put_msr_feature_control() safer
and harder to break.
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> > kvm_msr_entry_set(&msr_data.entry, MSR_IA32_FEATURE_CONTROL,
> > cpu->env.msr_ia32_feature_control);
> > @@ -1480,7 +1493,13 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
> > .nmsrs = 1,
> > };
> >
> > - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > +
> > + assert(ret == 1);
> > + return 0;
> > }
> >
> > static int kvm_put_msrs(X86CPU *cpu, int level)
> > @@ -1492,6 +1511,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > } msr_data;
> > struct kvm_msr_entry *msrs = msr_data.entries;
> > int n = 0, i;
> > + int ret;
> >
> > kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS, env->sysenter_cs);
> > kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
> > @@ -1685,8 +1705,13 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > .nmsrs = n,
> > };
> >
> > - return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > + ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > + if (ret < 0) {
> > + return ret;
> > + }
> >
> > + assert(ret == n);
> > + return 0;
> > }
> >
> >
> > @@ -2055,6 +2080,7 @@ static int kvm_get_msrs(X86CPU *cpu)
> > return ret;
> > }
> >
> > + assert(ret == n);
> > for (i = 0; i < ret; i++) {
> > uint32_t index = msrs[i].index;
> > switch (index) {
> > @@ -2511,7 +2537,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >
> > assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
> >
> > - if (level >= KVM_PUT_RESET_STATE && has_msr_feature_control) {
> > + if (level >= KVM_PUT_RESET_STATE) {
> > ret = kvm_put_msr_feature_control(x86_cpu);
> > if (ret < 0) {
> > return ret;
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-03-31 16:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-30 20:59 [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs Paolo Bonzini
2016-03-31 13:01 ` Laszlo Ersek
2016-03-31 13:03 ` Paolo Bonzini
2016-03-31 16:51 ` Eduardo Habkost
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).