| Message ID | 1459371583-4824-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; >
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; >> >
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; > > >
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;
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(-)