| Message ID | 20171120123525.147663-2-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | valgrind fallout for s390x/kvm | expand |
On Mon, 20 Nov 2017 13:35:24 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an > undefined value for flags. Right now this is unused, but we > better play safe. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > target/s390x/kvm.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 343fcec..b0439a1 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) > > void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > { > - struct kvm_s390_irq_state irq_state; > + struct kvm_s390_irq_state irq_state = { > + .buf = (uint64_t) cpu->irqstate, > + .len = VCPU_IRQ_BUF_SIZE, > + }; > CPUState *cs = CPU(cpu); > int32_t bytes; > > @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > return; > } > > - irq_state.buf = (uint64_t) cpu->irqstate; > - irq_state.len = VCPU_IRQ_BUF_SIZE; > - > bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); > if (bytes < 0) { > cpu->irqstate_saved_size = 0; I'm wondering why it does not also complain for KVM_S390_SET_IRQ_STATE? It would make sense to use a struct initializer there as well.
On 20.11.2017 13:35, Christian Borntraeger wrote: > valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an > undefined value for flags. Right now this is unused, but we > better play safe. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > target/s390x/kvm.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 343fcec..b0439a1 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) > > void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > { > - struct kvm_s390_irq_state irq_state; > + struct kvm_s390_irq_state irq_state = { > + .buf = (uint64_t) cpu->irqstate, > + .len = VCPU_IRQ_BUF_SIZE, > + }; > CPUState *cs = CPU(cpu); > int32_t bytes; > > @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > return; > } > > - irq_state.buf = (uint64_t) cpu->irqstate; > - irq_state.len = VCPU_IRQ_BUF_SIZE; > - > bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); > if (bytes < 0) { > cpu->irqstate_saved_size = 0; > Reviewed-by: Thomas Huth <thuth@redhat.com> BTW, should we have a check in the kernel that flags is properly set to zero? Thomas
On 11/20/2017 02:01 PM, Thomas Huth wrote: > On 20.11.2017 13:35, Christian Borntraeger wrote: >> valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an >> undefined value for flags. Right now this is unused, but we >> better play safe. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> target/s390x/kvm.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 343fcec..b0439a1 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) >> >> void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) >> { >> - struct kvm_s390_irq_state irq_state; >> + struct kvm_s390_irq_state irq_state = { >> + .buf = (uint64_t) cpu->irqstate, >> + .len = VCPU_IRQ_BUF_SIZE, >> + }; >> CPUState *cs = CPU(cpu); >> int32_t bytes; >> >> @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) >> return; >> } >> >> - irq_state.buf = (uint64_t) cpu->irqstate; >> - irq_state.len = VCPU_IRQ_BUF_SIZE; >> - >> bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); >> if (bytes < 0) { >> cpu->irqstate_saved_size = 0; >> > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > BTW, should we have a check in the kernel that flags is properly set to > zero? We should have one. Doing that now will break old QEMUs :-/
On 11/20/2017 01:57 PM, Cornelia Huck wrote: > On Mon, 20 Nov 2017 13:35:24 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an >> undefined value for flags. Right now this is unused, but we >> better play safe. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> target/s390x/kvm.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 343fcec..b0439a1 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) >> >> void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) >> { >> - struct kvm_s390_irq_state irq_state; >> + struct kvm_s390_irq_state irq_state = { >> + .buf = (uint64_t) cpu->irqstate, >> + .len = VCPU_IRQ_BUF_SIZE, >> + }; >> CPUState *cs = CPU(cpu); >> int32_t bytes; >> >> @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) >> return; >> } >> >> - irq_state.buf = (uint64_t) cpu->irqstate; >> - irq_state.len = VCPU_IRQ_BUF_SIZE; >> - >> bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); >> if (bytes < 0) { >> cpu->irqstate_saved_size = 0; > > I'm wondering why it does not also complain for KVM_S390_SET_IRQ_STATE? I guess that my tests do not having peding interrupts during migration and we exit early in int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) { CPUState *cs = CPU(cpu); struct kvm_s390_irq_state irq_state; int r; --> if (cpu->irqstate_saved_size == 0) { --> return 0; } > It would make sense to use a struct initializer there as well. Yes, I think you are right. >
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 343fcec..b0439a1 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) { - struct kvm_s390_irq_state irq_state; + struct kvm_s390_irq_state irq_state = { + .buf = (uint64_t) cpu->irqstate, + .len = VCPU_IRQ_BUF_SIZE, + }; CPUState *cs = CPU(cpu); int32_t bytes; @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) return; } - irq_state.buf = (uint64_t) cpu->irqstate; - irq_state.len = VCPU_IRQ_BUF_SIZE; - bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); if (bytes < 0) { cpu->irqstate_saved_size = 0;
valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an undefined value for flags. Right now this is unused, but we better play safe. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- target/s390x/kvm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)