qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Xu
Subject: Re: [Qemu-devel] [PATCH v5 02/18] vfio: introduce vfio_get_vaddr()
Date: Wed, 25 Jan 2017 12:32:21 +0800
User-agent: Mutt/1.5.24 (2015-08-30)

On Tue, Jan 24, 2017 at 09:29:05AM -0700, Alex Williamson wrote:
> On Tue, 24 Jan 2017 18:25:55 +0800
> Peter Xu <address@hidden> 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 <address@hidden>
> > ---
> >  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);
> >          if (ret) {
> >              error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> >                           "0x%"HWADDR_PRIx", %p) = %d (%m)",
> > @@ -357,8 +377,6 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, 
> > IOMMUTLBEntry *iotlb)
> >                           iotlb->addr_mask + 1, ret);
> >          }
> >      }
> > -out:
> > -    rcu_read_unlock();
> 
> 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?

Yes we should continue the discussion. Sorry for the inconvenient and
misunderstanding (I was only trying to provide a non-rfc version of
the series, in case Michael would like to pick up some cleanup any of
the patches).

I have similar question as well above - IIUC the RCU read lock
protects us from not losing the references of memory objects, however
in our case even after we release the lock, we are still using the
backend ram (vaddr) since we have set it up inside kernel to build up
the IO page table. After that, the kernel/device should be able to
write to addresses of that backend ram any time. Is there possibly a
window that kernel/device may write to that backend ram even after
guest destroyed the memory objects (e.g., by memory unplug?) and its
backend ram?

Thanks,

-- peterx



reply via email to

[Prev in Thread] Current Thread [Next in Thread]