Re: [Qemu-devel] [PATCH RFC v4 02/20] vfio: introduce vfio_get_vaddr()

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 24 Jan 2017 11:28:18 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Mon, Jan 23, 2017 at 11:49:05AM -0700, Alex Williamson wrote:
> > On Fri, 20 Jan 2017 21:08:38 +0800
> > Peter Xu <peterx@redhat.com> wrote:
> >   
> > > A cleanup for vfio_iommu_map_notify(). Should have no functional change,
> > > just to make the function shorter and easier to understand.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  hw/vfio/common.c | 58 +++++++++++++++++++++++++++++++++++++-------------------
> > >  1 file changed, 38 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > index 174f351..ce55dff 100644
> > > --- a/hw/vfio/common.c
> > > +++ b/hw/vfio/common.c
> > > @@ -294,25 +294,14 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> > >             section->offset_within_address_space & (1ULL << 63);
> > >  }
> > >  
> > > -static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > +static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > +                           bool *read_only)
> > >  {
> > > -    VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > > -    VFIOContainer *container = giommu->container;
> > > -    hwaddr iova = iotlb->iova + giommu->iommu_offset;
> > >      MemoryRegion *mr;
> > >      hwaddr xlat;
> > >      hwaddr len = iotlb->addr_mask + 1;
> > > -    void *vaddr;
> > > -    int ret;
> > > -
> > > -    trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
> > > -                                iova, iova + iotlb->addr_mask);
> > > -
> > > -    if (iotlb->target_as != &address_space_memory) {
> > > -        error_report("Wrong target AS \"%s\", only system memory is allowed",
> > > -                     iotlb->target_as->name ? iotlb->target_as->name : "none");
> > > -        return;
> > > -    }
> > > +    bool ret = false;
> > > +    bool writable = iotlb->perm & IOMMU_WO;
> > >  
> > >      /*
> > >       * The IOMMU TLB entry we have just covers translation through
> > > @@ -322,12 +311,13 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > >      rcu_read_lock();
> > >      mr = address_space_translate(&address_space_memory,
> > >                                   iotlb->translated_addr,
> > > -                                 &xlat, &len, iotlb->perm & IOMMU_WO);
> > > +                                 &xlat, &len, writable);
> > >      if (!memory_region_is_ram(mr)) {
> > >          error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > >                       xlat);
> > >          goto out;
> > >      }
> > > +
> > >      /*
> > >       * Translation truncates length to the IOMMU page size,
> > >       * check that it did not truncate too much.
> > > @@ -337,11 +327,41 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > >          goto out;
> > >      }
> > >  
> > > +    *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > +    *read_only = !writable || mr->readonly;
> > > +    ret = true;
> > > +
> > > +out:
> > > +    rcu_read_unlock();
> > > +    return ret;
> > > +}
> > > +
> > > +static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > +{
> > > +    VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > > +    VFIOContainer *container = giommu->container;
> > > +    hwaddr iova = iotlb->iova + giommu->iommu_offset;
> > > +    bool read_only;
> > > +    void *vaddr;
> > > +    int ret;
> > > +
> > > +    trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
> > > +                                iova, iova + iotlb->addr_mask);
> > > +
> > > +    if (iotlb->target_as != &address_space_memory) {
> > > +        error_report("Wrong target AS \"%s\", only system memory is allowed",
> > > +                     iotlb->target_as->name ? iotlb->target_as->name : "none");
> > > +        return;
> > > +    }
> > > +
> > > +    if (!vfio_get_vaddr(iotlb, &vaddr, &read_only)) {
> > > +        return;
> > > +    }
> > > +
> > >      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > > -        vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > >          ret = vfio_dma_map(container, iova,
> > >                             iotlb->addr_mask + 1, vaddr,
> > > -                           !(iotlb->perm & IOMMU_WO) || mr->readonly);
> > > +                           read_only);  
> > 
> > Is it really valid to move the map ioctl out of the rcu read lock?
> > We're making use of vaddr, which is directly a property of a
> > MemoryRegion which may have now disappeared.  With the lock released,
> > could an unmap race the map resulting in the wrong ordering?  As noted
> > previously, there are some subtle changes here, we do the
> > memory_region_get_ram_ptr() translation on both map and unmap (fixed in
> > next patch) and then pull map out of the rcu lock.  I'm not sure the
> > extra function is worthwhile or really has no functional change.
> > Thanks,  
> 
> Thanks for raising this question up.
> 
> IIUC this function can be triggered by three cases (this is for x86, I
> suppose the rule should be same for all platforms):
> 
> - memory hot add/remove
> - a PSI (page selective invalidation) for a newly mapped io page
> - a domain switch (needs a iommu replay)
> 
> IMHO all these places are protected by the QBL (both 2nd/3rd cases
> should be invoked in VT-d IOMMU MMIO write to queue invalidation
> registers)? And I thought BQL should be regarded as a write lock even
> stronger than RCU read lock?
> 
> If I understand it correctly above, looks like we should be safe here
> as long as we are always with BQL? And, if so, do we really need RCU
> read protection here?
> 
> Please kindly correct if I missed anything.

Note that this code is originally used on power systems, David Gibson
will also need to review whether the SPAPR IOMMU can handle the
relaxed unmap in patch 03/20.  I suspect you're right about current
usage vs BQL, but I wonder how that plays into the long term plans for
the BQL and whether the intention of 41063e1 was to standardize all
address_space_translate() callers regardless of any one caller's BQL
usage.

Paolo, what do you think?  Thanks,

Alex