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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 <[email protected]> > > > > 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
