Re: [Qemu-devel] [PATCH qemu v4 1/3] spapr_iommu: Do not replay mappings from just created DMA window

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

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

On Thu, Mar 07, 2019 at 04:05:16PM +1100, Alexey Kardashevskiy wrote:
> On sPAPR vfio_listener_region_add() is called in 2 situations:
> 1. a new listener is registered from vfio_connect_container();
> 2. a new IOMMU Memory Region is added from rtas_ibm_create_pe_dma_window().
> 
> In both cases vfio_listener_region_add() calls
> memory_region_iommu_replay() to notify newly registered IOMMU notifiers
> about existing mappings which is totally desirable for case 1.
> 
> However for case 2 it is nothing but noop as the window has just been
> created and has no valid mappings so replaying those does not do anything.
> It is barely noticeable with usual guests but if the window happens to be
> really big, such no-op replay might take minutes and trigger RCU stall
> warnings in the guest.
> 
> For example, a upcoming GPU RAM memory region mapped at 64TiB (right
> after SPAPR_PCI_LIMIT) causes a 64bit DMA window to be at least 128TiB
> which is (128<<40)/0x10000=2.147.483.648 TCEs to replay.
> 
> This mitigates the problem by adding an "skipping_replay" flag to
> sPAPRTCETable and defining sPAPR own IOMMU MR replay() hook which does
> exactly the same thing as the generic one except it returns early if
> @skipping_replay==true.
> 
> Another way of fixing this would be delaying replay till the very first
> H_PUT_TCE but this does not work if in-kernel H_PUT_TCE handler is
> enabled (a likely case).
> 
> When "ibm,create-pe-dma-window" is complete, the guest will map only
> required regions of the huge DMA window.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Applied, thanks.

> ---
> Changes:
> v4:
> * more explaining in the commit log and comments
> ---
>  include/hw/ppc/spapr.h  |  1 +
>  hw/ppc/spapr_iommu.c    | 31 +++++++++++++++++++++++++++++++
>  hw/ppc/spapr_rtas_ddw.c | 10 ++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ff1bd6061540..2b368e6677c5 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -723,6 +723,7 @@ struct sPAPRTCETable {
>      uint64_t *mig_table;
>      bool bypass;
>      bool need_vfio;
> +    bool skipping_replay;
>      int fd;
>      MemoryRegion root;
>      IOMMUMemoryRegion iommu;
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 37e98f93214d..8f231799b29b 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -141,6 +141,36 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(IOMMUMemoryRegion *iommu,
>      return ret;
>  }
>  
> +static void spapr_tce_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(iommu_mr);
> +    IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
> +    hwaddr addr, granularity;
> +    IOMMUTLBEntry iotlb;
> +    sPAPRTCETable *tcet = container_of(iommu_mr, sPAPRTCETable, iommu);
> +
> +    if (tcet->skipping_replay) {
> +        return;
> +    }
> +
> +    granularity = memory_region_iommu_get_min_page_size(iommu_mr);
> +
> +    for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
> +        iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx);
> +        if (iotlb.perm != IOMMU_NONE) {
> +            n->notify(n, &iotlb);
> +        }
> +
> +        /*
> +         * if (2^64 - MR size) < granularity, it's possible to get an
> +         * infinite loop here.  This should catch such a wraparound.
> +         */
> +        if ((addr + granularity) < addr) {
> +            break;
> +        }
> +    }
> +}
> +
>  static int spapr_tce_table_pre_save(void *opaque)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> @@ -659,6 +689,7 @@ static void spapr_iommu_memory_region_class_init(ObjectClass *klass, void *data)
>      IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
>  
>      imrc->translate = spapr_tce_translate_iommu;
> +    imrc->replay = spapr_tce_replay;
>      imrc->get_min_page_size = spapr_tce_get_min_page_size;
>      imrc->notify_flag_changed = spapr_tce_notify_flag_changed;
>      imrc->get_attr = spapr_tce_get_attr;
> diff --git a/hw/ppc/spapr_rtas_ddw.c b/hw/ppc/spapr_rtas_ddw.c
> index cb8a4103592e..cc9d1f5c1cc8 100644
> --- a/hw/ppc/spapr_rtas_ddw.c
> +++ b/hw/ppc/spapr_rtas_ddw.c
> @@ -171,8 +171,18 @@ static void rtas_ibm_create_pe_dma_window(PowerPCCPU *cpu,
>      }
>  
>      win_addr = (windows == 0) ? sphb->dma_win_addr : sphb->dma64_win_addr;
> +    /*
> +     * We have just created a window, we know for the fact that it is empty,
> +     * use a hack to avoid iterating over the table as it is quite possible
> +     * to have billions of TCEs, all empty.
> +     * Note that we cannot delay this to the first H_PUT_TCE as this hcall is
> +     * mostly likely to be handled in KVM so QEMU just does not know if it
> +     * happened.
> +     */
> +    tcet->skipping_replay = true;
>      spapr_tce_table_enable(tcet, page_shift, win_addr,
>                             1ULL << (window_shift - page_shift));
> +    tcet->skipping_replay = false;
>      if (!tcet->nb_table) {
>          goto hw_error_exit;
>      }

-- 
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: 833 bytes --]