The first issue that I encountered after working around the pSeries
machine type crashes on my PPC 970 that I mentioned in my previous
email is one that may affect many more users of KVM-PR, or at least
those using VGA output.
The bug manifests as the VGA screen not being refreshed properly, with
horizontal strips that aren't redrawn even though updated. Switching
GTK to a different pane and then back to VGA forces a redraw and I can
see the video memory was updated, so why wasn't it being redrawn?
I traced the issue to a race condition in the KVM-PR module: dirty
pages of video memory were setting the dirty bitmap bits to 1, but the
caller wasn't seeing those set bits. I'm not sure what the proper
etiquette is for contributing patches to KVM. I'm not quite ready to
send an email to LKML but here's a URL for my diffs in progress.
https://github.com/torvalds/linux/compare/master...jhamby:master
The bug in the current version of kvm_vm_ioctl_get_dirty_log_pr() is a
race condition between memcpy() of the dirty bits to user space, and
clearing the bits to 0. I think the ideal fix would be for all ppc
variants of KVM to use the generic dirty bitmap code like x86 and ARM
do, because then you'll get the new dirty ring buffer API support for
free, but that would require touching more files than I wanted to, or
could test on my limited ppc64 hardware.
The patch I came up with (in two commits) to
arch/powerpc/kvm/book3s_pr.c is to atomically exchange 0's into the
dirty bitmap while copying the old values into the second half of the
dirty bitmap buffer (twice the bitmap size is allocated to use for
this purpose on other CPUs). Then I copy the bitmap from the second
half of the buffer to user space and if any bits were one, I flush the
PTEs so any future writes after the call returns will set the dirty
bitmap (same logic as before).
I tried various other approaches to fix the race condition, like
locking mutexes and spinlocks, but only the atomic exchange approach
worked reliably. My first working attempt used cmpxchg64() but then I
realized that wouldn't work for 32-bit, and that I could use
xchg_relaxed() instead, which also worked.