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

[Peter Xu <peterx@redhat.com>]

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