* 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