[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v4 02/20] vfio: introduce vfio_get_vaddr()
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH RFC v4 02/20] vfio: introduce vfio_get_vaddr() |
Date: |
Mon, 23 Jan 2017 11:49:05 -0700 |
On Fri, 20 Jan 2017 21:08:38 +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);
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,
Alex
> 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();
> }
>
> static void vfio_listener_region_add(MemoryListener *listener,
- [Qemu-devel] [PATCH RFC v4 00/20] VT-d: vfio enablement and misc enhances, Peter Xu, 2017/01/20
- [Qemu-devel] [PATCH RFC v4 03/20] vfio: allow to notify unmap for very large region, Peter Xu, 2017/01/20
- [Qemu-devel] [PATCH RFC v4 04/20] IOMMU: add option to enable VTD_CAP_CM to vIOMMU capility exposoed to guest, Peter Xu, 2017/01/20
- [Qemu-devel] [PATCH RFC v4 05/20] intel_iommu: simplify irq region translation, Peter Xu, 2017/01/20
- [Qemu-devel] [PATCH RFC v4 06/20] intel_iommu: renaming gpa to iova where proper, Peter Xu, 2017/01/20
- [Qemu-devel] [PATCH RFC v4 07/20] intel_iommu: fix trace for inv desc handling, Peter Xu, 2017/01/20
- [Qemu-devel] [PATCH RFC v4 08/20] intel_iommu: fix trace for addr translation, Peter Xu, 2017/01/20
- [Qemu-devel] [PATCH RFC v4 09/20] intel_iommu: vtd_slpt_level_shift check level, Peter Xu, 2017/01/20