Hi, On 11/05/18 17:44, Paolo Bonzini wrote: > On 11/05/2018 15:25, Andre Przywara wrote: >> - I couldn't find any protection for the usage in >> arch/powerpc/kvm/powerpc.c, but the call chain is quite convoluted >> there, so I might have missed something. It would be good if someone >> more familiar with this code would take a look. > > I also didn't find anything, I got up to kvmppc_handle_exit_pr in > book3s_pr.c and kvmppc_handle_exit in booke.c, then the callers are > assembly and I decided it's buggy. :) > >>> Adding the srcu_read_lock/unlock directly in kvm_arch_vcpu_ioctl_run and >>> any other ioctls that need it is best, but in any case adding more pairs >>> is safe because they can be nested. >> >> So I added a small wrapper around kvm_read_guest(), which takes and >> drops the lock. Will send out the patch shortly. If powerpc needs it, I >> am happy to provide this wrapper in kvm_main.c instead of some arm >> header file instead. > > I think that risks having some performance impact, though perhaps > mitigated by ARM having many virtual devices in the core. Moving it > above would be better, to the equivalent of POWER's kvmppc_handle_exit* > functions. Well, I am not sure the performance is super important here, since doing kvm_read_guest() in the first place is probably not very cheap anyway. Also some instances of kvm_read_guest() are called from the save/restore code, which originates from userland, so we wouldn't be covered by doing in handle_exit. But we can somewhat optimize by taking some locks outside of loops, for instance. Not sure that is premature optimization, though. I would prefer doing that later. I think in the past I had some idea how to avoid accessing guest memory in some cases, not sure those still apply. Cheers, Andre.
