| Message ID | 1414661809-21383-1-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 30 October 2014 09:36, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > This series avoids most memcheck false positives in KVM ioctls on s390x > and x86_64. > > Please review and consider for 2.2 or later. Some of these things could > also be fixed in valgrind, but it will take a while until these changes > hit a release or distros. Are you planning to submit the valgrind fixes as well? These definitely seem like valgrind bugs that we're having to work around here (though the workarounds are pretty simple so they're not a huge deal). thanks -- PMM
Am 30.10.2014 10:46, schrieb Peter Maydell: > On 30 October 2014 09:36, Christian Borntraeger <borntraeger@de.ibm.com> wrote: >> This series avoids most memcheck false positives in KVM ioctls on s390x >> and x86_64. >> >> Please review and consider for 2.2 or later. Some of these things could >> also be fixed in valgrind, but it will take a while until these changes >> hit a release or distros. > > Are you planning to submit the valgrind fixes as well? These definitely > seem like valgrind bugs that we're having to work around here (though > the workarounds are pretty simple so they're not a huge deal). Yes, I will try to get some of this fixed in valgrind as well. This will take a little longer though because the code changes are bigger than just 1 line of code. Given that valgrind has around 1 release/year, this patch set is certainly a nice band-aid that is useful for todays development. Christian
On 30 October 2014 09:50, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > Yes, I will try to get some of this fixed in valgrind as well. This will > take a little longer though because the code changes are bigger than just > 1 line of code. Given that valgrind has around 1 release/year, this patch > set is certainly a nice band-aid that is useful for todays development. I guess these patches also mean we're not going to get valgrind warnings if we forget to initialize a necessary field in the struct, but I suppose we can live with that. -- PMM
Am 30.10.2014 10:58, schrieb Peter Maydell: > On 30 October 2014 09:50, Christian Borntraeger <borntraeger@de.ibm.com> wrote: >> Yes, I will try to get some of this fixed in valgrind as well. This will >> take a little longer though because the code changes are bigger than just >> 1 line of code. Given that valgrind has around 1 release/year, this patch >> set is certainly a nice band-aid that is useful for todays development. > > I guess these patches also mean we're not going to get valgrind warnings > if we forget to initialize a necessary field in the struct. Yes, thats correct. The alternative would be to not use an designated initializer and instead memset reserved and pad field. > but I suppose we can live with that. > > -- PMM >
On 10/30/2014 10:36 AM, Christian Borntraeger wrote: > Some of these things could > also be fixed in valgrind, but it will take a while until these changes > hit a release or distros. Ok, it's sensible to have it fixed in QEMU if it's temporary. Which could not be fixed in valgrind? Paolo
Am 30.10.2014 14:03, schrieb Paolo Bonzini: > On 10/30/2014 10:36 AM, Christian Borntraeger wrote: >> Some of these things could >> also be fixed in valgrind, but it will take a while until these changes >> hit a release or distros. > > Ok, it's sensible to have it fixed in QEMU if it's temporary. Which > could not be fixed in valgrind? This is a tricky question. A typical annotation in valgrind for an more complex ioctl looks like case VKI_SIOCGMIIREG: /* get hardware entry registers */ PRE_MEM_RASCIIZ( "ioctl(SIOCGIFMIIREG)", (Addr)((struct vki_ifreq *)ARG3)->vki_ifr_name ); PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)", (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id, sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id) ); PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)", (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num, sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num) ); PRE_MEM_WRITE( "ioctl(SIOCGIFMIIREG)", ARG3, sizeof(struct vki_ifreq)); break; This scheme works fine as long as the ioctl is unchanged. So any ioctl that has padding and no flags this should be doable. For all KVM ioctls with reserved fields that might become used on certain flags, we have two options: a: we would instruct valgrind to not check the reserved fields Whenever we start using them, we would still not check those field b: we would instruct valgrind to not check the reserved fields if flags has a certain value (e.g. 0), otherwise all reserved fields would be checked. Whenever we start using the reserved fields, valgrind would complain unless we write all. So in that case we have to modify valgrind again In essence a will cause false negatives, b will cause false positives I think b is preferred Christian
On 30/10/2014 14:20, Christian Borntraeger wrote: > Am 30.10.2014 14:03, schrieb Paolo Bonzini: >> On 10/30/2014 10:36 AM, Christian Borntraeger wrote: >>> Some of these things could >>> also be fixed in valgrind, but it will take a while until these changes >>> hit a release or distros. >> >> Ok, it's sensible to have it fixed in QEMU if it's temporary. Which >> could not be fixed in valgrind? > > This is a tricky question. A typical annotation in valgrind for an more complex ioctl looks like > > case VKI_SIOCGMIIREG: /* get hardware entry registers */ > PRE_MEM_RASCIIZ( "ioctl(SIOCGIFMIIREG)", > (Addr)((struct vki_ifreq *)ARG3)->vki_ifr_name ); > PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)", > (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id, > sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->phy_id) ); > PRE_MEM_READ( "ioctl(SIOCGIFMIIREG)", > (Addr)&((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num, > sizeof(((struct vki_mii_ioctl_data *)&((struct vki_ifreq *)ARG3)->vki_ifr_data)->reg_num) ); > PRE_MEM_WRITE( "ioctl(SIOCGIFMIIREG)", ARG3, > sizeof(struct vki_ifreq)); > break; > > This scheme works fine as long as the ioctl is unchanged. > So any ioctl that has padding and no flags this should be doable. > > For all KVM ioctls with reserved fields that might become used on certain flags, we have two options: > > a: we would instruct valgrind to not check the reserved fields > Whenever we start using them, we would still not check those field > > b: we would instruct valgrind to not check the reserved fields if flags has a certain value (e.g. 0), otherwise all reserved fields would be checked. > Whenever we start using the reserved fields, valgrind would complain unless we write all. So in that case we have to modify valgrind again > > In essence a will cause false negatives, b will cause false positives > > I think b is preferred I agree. Paolo
On 30/10/2014 10:36, Christian Borntraeger wrote: > This series avoids most memcheck false positives in KVM ioctls on s390x > and x86_64. > > Please review and consider for 2.2 or later. Some of these things could > also be fixed in valgrind, but it will take a while until these changes > hit a release or distros. > > The series is also available via signed tag: > > The following changes since commit 3e9418e160cd8901c83a3c88967158084f5b5c03: > > Revert "main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously" (2014-10-27 15:05:09 +0000) > > are available in the git repository at: > > git://github.com/borntraeger/qemu.git tags/memcheck > > for you to fetch changes up to e5bcf96f709e57a54188ffa6a988b6acb603df7a: > > valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl (2014-10-30 10:08:49 +0100) > > ---------------------------------------------------------------- > valgrind/i386/s390x: memcheck false positives > > Let's avoid most memcheck false positives in KVM ioctls. > > ---------------------------------------------------------------- > Christian Borntraeger (9): > valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl > valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl > valgrind/i386: avoid false positives on KVM_SET_PIT ioctl > valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl > valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl > valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl > valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl > valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl > valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl > > hw/i386/kvm/clock.c | 3 +-- > hw/i386/kvm/i8254.c | 2 +- > kvm-all.c | 2 +- > target-i386/kvm.c | 9 +++++---- > target-s390x/kvm.c | 2 +- > 5 files changed, 9 insertions(+), 9 deletions(-) > > > Christian, when you respin can you add another one in kvm_irqchip_add_adapter_route? Coverity reports kroute.pad as uninitialized. Paolo
Am 13.11.2014 um 15:34 schrieb Paolo Bonzini: > On 30/10/2014 10:36, Christian Borntraeger wrote: >> This series avoids most memcheck false positives in KVM ioctls on s390x >> and x86_64. >> >> Please review and consider for 2.2 or later. Some of these things could >> also be fixed in valgrind, but it will take a while until these changes >> hit a release or distros. >> >> The series is also available via signed tag: >> >> The following changes since commit 3e9418e160cd8901c83a3c88967158084f5b5c03: >> >> Revert "main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously" (2014-10-27 15:05:09 +0000) >> >> are available in the git repository at: >> >> git://github.com/borntraeger/qemu.git tags/memcheck >> >> for you to fetch changes up to e5bcf96f709e57a54188ffa6a988b6acb603df7a: >> >> valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl (2014-10-30 10:08:49 +0100) >> >> ---------------------------------------------------------------- >> valgrind/i386/s390x: memcheck false positives >> >> Let's avoid most memcheck false positives in KVM ioctls. >> >> ---------------------------------------------------------------- >> Christian Borntraeger (9): >> valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl >> valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl >> valgrind/i386: avoid false positives on KVM_SET_PIT ioctl >> valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl >> valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl >> valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl >> valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl >> valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl >> valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl >> >> hw/i386/kvm/clock.c | 3 +-- >> hw/i386/kvm/i8254.c | 2 +- >> kvm-all.c | 2 +- >> target-i386/kvm.c | 9 +++++---- >> target-s390x/kvm.c | 2 +- >> 5 files changed, 9 insertions(+), 9 deletions(-) >> >> >> > > Christian, when you respin can you add another one in > kvm_irqchip_add_adapter_route? Coverity reports kroute.pad as > uninitialized. OK, will do. It will take probably some more days, but it will happen :-)
This series avoids most memcheck false positives in KVM ioctls on s390x and x86_64. Please review and consider for 2.2 or later. Some of these things could also be fixed in valgrind, but it will take a while until these changes hit a release or distros. The series is also available via signed tag: The following changes since commit 3e9418e160cd8901c83a3c88967158084f5b5c03: Revert "main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously" (2014-10-27 15:05:09 +0000) are available in the git repository at: git://github.com/borntraeger/qemu.git tags/memcheck for you to fetch changes up to e5bcf96f709e57a54188ffa6a988b6acb603df7a: valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl (2014-10-30 10:08:49 +0100) ---------------------------------------------------------------- valgrind/i386/s390x: memcheck false positives Let's avoid most memcheck false positives in KVM ioctls. ---------------------------------------------------------------- Christian Borntraeger (9): valgrind: avoid false positives in KVM_GET_DIRTY_LOG ioctl valgrind/i386: avoid false positives on KVM_SET_CLOCK ioctl valgrind/i386: avoid false positives on KVM_SET_PIT ioctl valgrind/i386: avoid false positives on KVM_SET_XCRS ioctl valgrind/i386: avoid false positives on KVM_SET_MSRS ioctl valgrind/i386: avoid false positives on KVM_SET_MSRS(TSC) ioctl valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl valgrind/i386: avoid false positives on KVM_SET_VCPU_EVENTS ioctl valgrind/s390x: avoid false positives on KVM_SET_FPU ioctl hw/i386/kvm/clock.c | 3 +-- hw/i386/kvm/i8254.c | 2 +- kvm-all.c | 2 +- target-i386/kvm.c | 9 +++++---- target-s390x/kvm.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-)