Re: [Qemu-devel] [PATCH qemu v17 12/12] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping listening

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

On Wed,  1 Jun 2016 18:57:43 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> The sPAPR TCE tables manage 2 copies when VFIO is using an IOMMU -
> a guest view of the table and a hardware TCE table. If there is no VFIO
> presense in the address space, then just the guest view is used, if
> this is the case, it is allocated in the KVM. However since there is no
> support yet for VFIO in KVM TCE hypercalls, when we start using VFIO,
> we need to move the guest view from KVM to the userspace; and we need
> to do this for every IOMMU on a bus with VFIO devices.
> 
> This adds notify_started/notify_stopped callbacks in MemoryRegionIOMMUOps
> to notify IOMMU that listeners were set/removed. This allows IOMMU to
> take necessary steps before actual notifications happen and do proper
> cleanup when the last notifier is removed.
> 
> This implements the callbacks for the sPAPR IOMMU - notify_started()
> reallocated the guest view to the user space, notify_stopped() does
> the opposite.
> 
> This removes explicit spapr_tce_set_need_vfio() call from PCI hotplug
> path as the new callbacks do this better - they notify IOMMU at
> the exact moment when the configuration is changed, and this also
> includes the case of PCI hot unplug.
> 
> This adds MemoryRegion* to memory_region_unregister_iommu_notifier()
> as we need iommu_ops to call notify_stopped() and Notifier* does not
> store the owner.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> Changes:
> v17:
> * replaced IOMMU users counting with simple QLIST_EMPTY()
> * renamed the callbacks
> * removed requirement for region_del() to be called on memory_listener_unregister()
> 
> v16:
> * added a use counter in VFIOAddressSpace->VFIOIOMMUMR
> 
> v15:
> * s/need_vfio/vfio-Users/g
> ---

This should be two separate patches, patch #2:

>  hw/ppc/spapr_iommu.c  | 12 ++++++++++++
>  hw/ppc/spapr_pci.c    |  6 ------

patch #1:

>  hw/vfio/common.c      |  5 +++--
>  include/exec/memory.h |  8 +++++++-
>  memory.c              | 10 +++++++++-
>  5 files changed, 31 insertions(+), 10 deletions(-)

Otherwise

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 90a45c0..994a8a0 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -156,6 +156,16 @@ static uint64_t spapr_tce_get_page_sizes(MemoryRegion *iommu)
>      return 1ULL << tcet->page_shift;
>  }
>  
> +static void spapr_tce_notify_started(MemoryRegion *iommu)
> +{
> +    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true);
> +}
> +
> +static void spapr_tce_notify_stopped(MemoryRegion *iommu)
> +{
> +    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false);
> +}
> +
>  static int spapr_tce_table_post_load(void *opaque, int version_id)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> @@ -236,6 +246,8 @@ static const VMStateDescription vmstate_spapr_tce_table = {
>  static MemoryRegionIOMMUOps spapr_iommu_ops = {
>      .translate = spapr_tce_translate_iommu,
>      .get_page_sizes = spapr_tce_get_page_sizes,
> +    .notify_started = spapr_tce_notify_started,
> +    .notify_stopped = spapr_tce_notify_stopped,
>  };
>  
>  static int spapr_tce_table_realize(DeviceState *dev)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index bcf0360..06ce902 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1089,12 +1089,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
>      void *fdt = NULL;
>      int fdt_start_offset = 0, fdt_size;
>  
> -    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
> -        sPAPRTCETable *tcet = spapr_tce_find_by_liobn(phb->dma_liobn[0]);
> -
> -        spapr_tce_set_need_vfio(tcet, true);
> -    }
> -
>      fdt = create_device_tree(&fdt_size);
>      fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
>      if (!fdt_start_offset) {
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 7f55c26..356640e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -522,7 +522,8 @@ static void vfio_listener_region_del(MemoryListener *listener,
>  
>          QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
>              if (giommu->iommu == section->mr) {
> -                memory_region_unregister_iommu_notifier(&giommu->n);
> +                memory_region_unregister_iommu_notifier(giommu->iommu,
> +                                                        &giommu->n);
>                  QLIST_REMOVE(giommu, giommu_next);
>                  g_free(giommu);
>                  break;
> @@ -1094,7 +1095,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>          QLIST_REMOVE(container, next);
>  
>          QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, tmp) {
> -            memory_region_unregister_iommu_notifier(&giommu->n);
> +            memory_region_unregister_iommu_notifier(giommu->iommu, &giommu->n);
>              QLIST_REMOVE(giommu, giommu_next);
>              g_free(giommu);
>          }
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index bd9625f..f08439b 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -151,6 +151,10 @@ struct MemoryRegionIOMMUOps {
>      IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write);
>      /* Returns supported page sizes */
>      uint64_t (*get_page_sizes)(MemoryRegion *iommu);
> +    /* Called when the first notifier is set */
> +    void (*notify_started)(MemoryRegion *iommu);
> +    /* Called when the last notifier is removed */
> +    void (*notify_stopped)(MemoryRegion *iommu);
>  };
>  
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> @@ -619,9 +623,11 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write);
>   * memory_region_unregister_iommu_notifier: unregister a notifier for
>   * changes to IOMMU translation entries.
>   *
> + * @mr: the memory region which was observed and for which notity_stopped()
> + *      needs to be called
>   * @n: the notifier to be removed.
>   */
> -void memory_region_unregister_iommu_notifier(Notifier *n);
> +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n);
>  
>  /**
>   * memory_region_name: get a memory region's name
> diff --git a/memory.c b/memory.c
> index 761ae92..ee41649 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1497,6 +1497,10 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
>  
>  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);
> +    }
>      notifier_list_add(&mr->iommu_notify, n);
>  }
>  
> @@ -1530,9 +1534,13 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool is_write)
>      }
>  }
>  
> -void memory_region_unregister_iommu_notifier(Notifier *n)
> +void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n)
>  {
>      notifier_remove(n);
> +    if (mr->iommu_ops->notify_stopped &&
> +        QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
> +        mr->iommu_ops->notify_stopped(mr);
> +    }
>  }
>  
>  void memory_region_notify_iommu(MemoryRegion *mr,