Re: [Qemu-devel] [PATCH 2/3] memory: add iommu_notify_flag

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4650 bytes --]

On Mon, Sep 05, 2016 at 03:21:20PM +0800, Peter Xu wrote:
> When there are active IOMMU notify registers, the iommu_notify_flag will
> cache existing notify flag. When notifiers are triggered, flag will be
> checked against the cached result.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/ppc/spapr_iommu.c     | 2 +-
>  hw/s390x/s390-pci-inst.c | 2 +-
>  include/exec/memory.h    | 6 +++++-
>  memory.c                 | 6 +++++-
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 09660fc..14b1f00 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -411,7 +411,7 @@ static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba,
>      entry.translated_addr = tce & page_mask;
>      entry.addr_mask = ~page_mask;
>      entry.perm = spapr_tce_iommu_access_flags(tce);
> -    memory_region_notify_iommu(&tcet->iommu, entry);
> +    memory_region_notify_iommu(&tcet->iommu, entry, IOMMU_RW);
>  
>      return H_SUCCESS;
>  }
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index f069b11..29ef345 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -616,7 +616,7 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>              goto out;
>          }
>  
> -        memory_region_notify_iommu(mr, entry);
> +        memory_region_notify_iommu(mr, entry, IOMMU_RW);
>          start += entry.addr_mask + 1;
>      }
>  
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 6c16a86..c67b3da 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -57,6 +57,7 @@ typedef enum {
>      IOMMU_RO   = 1,
>      IOMMU_WO   = 2,
>      IOMMU_RW   = 3,
> +    IOMMU_ACCESS_INVALID = 4,
>  } IOMMUAccessFlags;
>  
>  struct IOMMUTLBEntry {
> @@ -202,6 +203,7 @@ struct MemoryRegion {
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
>      NotifierList iommu_notify;
> +    IOMMUAccessFlags iommu_notify_flag;
>  };
>  
>  /**
> @@ -611,9 +613,11 @@ uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr);
>   * @entry: the new entry in the IOMMU translation table.  The entry
>   *         replaces all old entries for the same virtual I/O address range.
>   *         Deleted entries have .@perm == 0.
> + * @flag: type of IOMMU notification (IOMMU_RW, IOMMU_NONE)

This makes no sense.  The overall type of the notifier is already
noted in register / notify_start.  The permission on this specific
mapping is already included in the IOMMUTLBEntry.

>   */
>  void memory_region_notify_iommu(MemoryRegion *mr,
> -                                IOMMUTLBEntry entry);
> +                                IOMMUTLBEntry entry,
> +                                IOMMUAccessFlags flag);
>  
>  /**
>   * memory_region_register_iommu_notifier: register a notifier for changes to
> diff --git a/memory.c b/memory.c
> index 747a9ec..5b50d15 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1418,6 +1418,7 @@ void memory_region_init_iommu(MemoryRegion *mr,
>      memory_region_init(mr, owner, name, size);
>      mr->iommu_ops = ops,
>      mr->terminates = true;  /* then re-forwards */
> +    mr->iommu_notify_flag = IOMMU_ACCESS_INVALID;
>      notifier_list_init(&mr->iommu_notify);
>  }
>  
> @@ -1520,6 +1521,7 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n,
>      if (mr->iommu_ops->notify_started &&
>          QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
>          mr->iommu_ops->notify_started(mr, flag);
> +        mr->iommu_notify_flag = flag;
>      }
>      notifier_list_add(&mr->iommu_notify, n);
>  }
> @@ -1560,13 +1562,15 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n)
>      if (mr->iommu_ops->notify_stopped &&
>          QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
>          mr->iommu_ops->notify_stopped(mr);
> +        mr->iommu_notify_flag = IOMMU_ACCESS_INVALID;
>      }
>  }
>  
>  void memory_region_notify_iommu(MemoryRegion *mr,
> -                                IOMMUTLBEntry entry)
> +                                IOMMUTLBEntry entry, IOMMUAccessFlags flag)
>  {
>      assert(memory_region_is_iommu(mr));
> +    assert(flag == mr->iommu_notify_flag);
>      notifier_list_notify(&mr->iommu_notify, &entry);
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]