On Thu, Jan 26, 2017 at 11:55:22AM +0100, Paolo Bonzini wrote: > > > On 26/01/2017 08:12, Peter Xu wrote: > > > > /* > > * Here, we need to have the lock not only for vfio_get_vaddr(), > > * but also needs to make sure that the vaddr will be valid for > > * further operations. > > * > > * When we map new pages, we need the lock to make sure that vaddr > > * is valid along the way we build up the IO page table (via > > * vfio_dma_map()). Then, as long as the mapping is set up, we can > > * unlock since those pages will be pinned in kernel (which makes > > * sure that the RAM backend of vaddr will always be there, even > > * if the memory object is destroyed and RAM released). > > * > > * For unmapping case, we don't really need the protection since > > * the pages are in all cases locked in kernel, so we'll probably > > * be safe even without the lock. However, it won't hurt we have > > * the lock as well here. > > */ > > Even simpler, just before the definition of vfio_get_vaddr: > > /* Called with rcu_read_lock held. */ > > and just before the vfio_dma_map call: > > /* vaddr is only valid until rcu_read_unlock(). But after > * vfio_dma_map has set up the mapping the pages will be pinned > * by the kernel. This makes sure that the RAM backend of vaddr > * will always be there, even if the memory object is destroyed > * and its backing memory munmap-ed. > */ > > I'm not sure that you can get rid of the lock for the unmapping case. > Better remove that part of the comment.
Sure. Let me take yours. Thanks! -- peterx