[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can s
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs |
Date: |
Thu, 31 Mar 2016 13:51:50 -0300 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 <address@hidden>
> > ---
> > 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 <address@hidden>
>
> 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 <address@hidden>
> >
> > 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