Re: [Qemu-devel] [PATCH v5 02/18] vfio: introduce vfio_get_vaddr()

[Peter Xu <peterx@redhat.com>]

On Wed, Jan 25, 2017 at 06:11:37PM +0100, Paolo Bonzini wrote:

[...]

> > The comment from v4 still needs input from Paolo, is it valid to make
> > use of vaddr (based on address_space_translate ->
> > memory_region_get_ram_ptr) outside of the rcu read lock or could future
> > BQL reduction efforts allow this to race?
> 
> You need to keep a reference to the MemoryRegion if you do
> rcu_read_unlock.  But it's simpler to call vfio_get_vaddr within
> rcu_read_lock, and keep the lock/unlock in vfio_iommu_map_notify.
> 
> You probably should also put a comment about why VFIO does *not* need to
> keep a reference between vfio_dma_map and vfio_dma_unmap (which doesn't
> sound easy to do either).

How about this one?

    /*
     * 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.
     */

Thanks,

-- peterx