On 11/22/2017 03:43 PM, Cornelia Huck wrote:
> On Wed, 22 Nov 2017 15:26:26 +0100
> Christian Borntraeger <borntrae...@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.
>> The same is true for SET_IRQ_STATE. While QEMU is now fixed in
>> that regard, we should make sure to not use the flag and pad
>> fields in the kernel. A corresponding kernel documentation patch
>> will be submitted later.
>
> I'd reword this a bit; it is confusing to read changelogs describing a
> then-future change which already happened. (I assume that we will do
> the kernel sanitizing for 4.15?)
>
> "valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an
> undefined value for flags. Kernels prior to 4.15 did not use that
> field, and later kernels ignore it for compatibility reasons, but we
> better play safe.
>
> The same is true for SET_IRQ_STATE. We should make sure to not use the
> flag field, either."
>
> [I do not see a 'pad' field in the structure; that is a bug in the
> kernel documentation.]
Would be ok for me. Can you take the patch and change the wording yourself?
>
>>
>> Signed-off-by: Christian Borntraeger <borntrae...@de.ibm.com>
>> ---
>> target/s390x/kvm.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 343fcec..76065ec 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;
>> @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu)
>> int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>> {
>> CPUState *cs = CPU(cpu);
>> - struct kvm_s390_irq_state irq_state;
>> + struct kvm_s390_irq_state irq_state = {
>> + .buf = (uint64_t) cpu->irqstate,
>> + .len = cpu->irqstate_saved_size,
>> + };
>> int r;
>>
>> if (cpu->irqstate_saved_size == 0) {
>> @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu)
>> return -ENOSYS;
>> }
>>
>> - irq_state.buf = (uint64_t) cpu->irqstate;
>> - irq_state.len = cpu->irqstate_saved_size;
>> -
>> r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state);
>> if (r) {
>> error_report("Setting interrupt state failed %d", r);
>
> Patch looks good.
>
