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

RE: [PATCH v3 16/23] IOMMU: fold flush-all hook into "flush one"


  • To: "Beulich, Jan" <JBeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: "Tian, Kevin" <kevin.tian@xxxxxxxxx>
  • Date: Sun, 30 Jan 2022 03:38:35 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=a1a/tJv3DdXh7u5vHvSIt1V00yWp6qTj5dVuUR7VvlY=; b=dqt91hFuSyqx/HJsvWNTff92m5DZWVsTug98hlejmY9URwrbAjvDVKVLnRRcEfS97kf9TVnUEj71Ul8YQAMqpybo8KMsivyHjgOXzGbCvJsQaD+h4VGdrlgEPx/MciHCyio7MkGtETFfPSXQTothB9efaTGdkK3Lk3TgFIT13BztQbbgB/BGN59wWIqfwZ5F2ksbZAL1bcEPRfhcguXL+IfSpnInu5g/Nnjy4BB1Q0mdxyGdMcugyvBZYqPG4uG4FdYiBUqOhDx8lgp0HYP+Nuhw9M+awN4XpEx/ffT74CzPUDJpfOK6wBPEgqokfV6vcg585fcJCzrUMxrJhUN5eQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=UGCHmHteCRIOlWZ8FfbmvbkDULHn8B+JFpHi2LhXqXVio1b2SFxkOn2lD7nLZHBbx3rrHhzWA3evfPSwXs9SlyRA7RivMhqGJhNHuu7YA0TSTofEA+7wEIfK2/S5PiCe7Zk9alVYwcDbMJpvh4dgfJ+TCuUK5A4Wv3/ddVDq6e3Cq+rORw39HKYvzZgRxnhcwuIeGLWD991pcL9w0BfU8zZqBwvML15jNaZs451VUOtzNuaULLR1Wo090UfSKiGR8/X9JFHnKQ6cT4EMLMvD612yTIRRgFsE+ZX3r9yTRKlAQnFBQs+F9tvuMkY05SBgWdKBmPBvv9WWjyaaNn67DA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com;
  • Cc: "Cooper, Andrew" <andrew.cooper3@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>, Pau Monné, Roger <roger.pau@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, "Volodymyr Babchuk" <volodymyr_babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <Rahul.Singh@xxxxxxx>
  • Delivery-date: Sun, 30 Jan 2022 03:38:57 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYBj/qAVjlDex7LEqC55KjKU7bMKx7Beww
  • Thread-topic: [PATCH v3 16/23] IOMMU: fold flush-all hook into "flush one"

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, January 11, 2022 12:34 AM
> 
> Having a separate flush-all hook has always been puzzling me some. We
> will want to be able to force a full flush via accumulated flush flags
> from the map/unmap functions. Introduce a respective new flag and fold
> all flush handling to use the single remaining hook.
> 
> Note that because of the respective comments in SMMU and IPMMU-VMSA
> code, I've folded the two prior hook functions into one. For SMMU-v3,
> which lacks a comment towards incapable hardware, I've left both
> functions in place on the assumption that selective and full flushes
> will eventually want separating.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> [IPMMU-VMSA and SMMU-V2]
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> [SMMUv3]
> Reviewed-by: Rahul Singh <rahul.singh@xxxxxxx>
> [Arm]
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> ---
> TBD: What we really are going to need is for the map/unmap functions to
>      specify that a wider region needs flushing than just the one
>      covered by the present set of (un)maps. This may still be less than
>      a full flush, but at least as a first step it seemed better to me
>      to keep things simple and go the flush-all route.
> ---
> v3: Re-base over changes earlier in the series.
> v2: New.
> 
> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -255,7 +255,6 @@ int amd_iommu_get_reserved_device_memory
>  int __must_check amd_iommu_flush_iotlb_pages(struct domain *d, dfn_t
> dfn,
>                                               unsigned long page_count,
>                                               unsigned int flush_flags);
> -int __must_check amd_iommu_flush_iotlb_all(struct domain *d);
>  void amd_iommu_print_entries(const struct amd_iommu *iommu, unsigned
> int dev_id,
>                               dfn_t dfn);
> 
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -478,15 +478,18 @@ int amd_iommu_flush_iotlb_pages(struct d
>  {
>      unsigned long dfn_l = dfn_x(dfn);
> 
> -    ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
> -    ASSERT(flush_flags);
> +    if ( !(flush_flags & IOMMU_FLUSHF_all) )
> +    {
> +        ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
> +        ASSERT(flush_flags);
> +    }
> 
>      /* Unless a PTE was modified, no flush is required */
>      if ( !(flush_flags & IOMMU_FLUSHF_modified) )
>          return 0;
> 
> -    /* If the range wraps then just flush everything */
> -    if ( dfn_l + page_count < dfn_l )
> +    /* If so requested or if the range wraps then just flush everything. */
> +    if ( (flush_flags & IOMMU_FLUSHF_all) || dfn_l + page_count < dfn_l )
>      {
>          amd_iommu_flush_all_pages(d);
>          return 0;
> @@ -511,13 +514,6 @@ int amd_iommu_flush_iotlb_pages(struct d
> 
>      return 0;
>  }
> -
> -int amd_iommu_flush_iotlb_all(struct domain *d)
> -{
> -    amd_iommu_flush_all_pages(d);
> -
> -    return 0;
> -}
> 
>  int amd_iommu_reserve_domain_unity_map(struct domain *d,
>                                         const struct ivrs_unity_map *map,
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -642,7 +642,6 @@ static const struct iommu_ops __initcons
>      .map_page = amd_iommu_map_page,
>      .unmap_page = amd_iommu_unmap_page,
>      .iotlb_flush = amd_iommu_flush_iotlb_pages,
> -    .iotlb_flush_all = amd_iommu_flush_iotlb_all,
>      .reassign_device = reassign_device,
>      .get_device_group_id = amd_iommu_group_id,
>      .enable_x2apic = iov_enable_xt,
> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> @@ -930,13 +930,19 @@ out:
>  }
> 
>  /* Xen IOMMU ops */
> -static int __must_check ipmmu_iotlb_flush_all(struct domain *d)
> +static int __must_check ipmmu_iotlb_flush(struct domain *d, dfn_t dfn,
> +                                          unsigned long page_count,
> +                                          unsigned int flush_flags)
>  {
>      struct ipmmu_vmsa_xen_domain *xen_domain = dom_iommu(d)-
> >arch.priv;
> 
> +    ASSERT(flush_flags);
> +
>      if ( !xen_domain || !xen_domain->root_domain )
>          return 0;
> 
> +    /* The hardware doesn't support selective TLB flush. */
> +
>      spin_lock(&xen_domain->lock);
>      ipmmu_tlb_invalidate(xen_domain->root_domain);
>      spin_unlock(&xen_domain->lock);
> @@ -944,16 +950,6 @@ static int __must_check ipmmu_iotlb_flus
>      return 0;
>  }
> 
> -static int __must_check ipmmu_iotlb_flush(struct domain *d, dfn_t dfn,
> -                                          unsigned long page_count,
> -                                          unsigned int flush_flags)
> -{
> -    ASSERT(flush_flags);
> -
> -    /* The hardware doesn't support selective TLB flush. */
> -    return ipmmu_iotlb_flush_all(d);
> -}
> -
>  static struct ipmmu_vmsa_domain *ipmmu_get_cache_domain(struct
> domain *d,
>                                                          struct device *dev)
>  {
> @@ -1303,7 +1299,6 @@ static const struct iommu_ops ipmmu_iomm
>      .hwdom_init      = ipmmu_iommu_hwdom_init,
>      .teardown        = ipmmu_iommu_domain_teardown,
>      .iotlb_flush     = ipmmu_iotlb_flush,
> -    .iotlb_flush_all = ipmmu_iotlb_flush_all,
>      .assign_device   = ipmmu_assign_device,
>      .reassign_device = ipmmu_reassign_device,
>      .map_page        = arm_iommu_map_page,
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2649,11 +2649,17 @@ static int force_stage = 2;
>   */
>  static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
> 
> -static int __must_check arm_smmu_iotlb_flush_all(struct domain *d)
> +static int __must_check arm_smmu_iotlb_flush(struct domain *d, dfn_t dfn,
> +                                          unsigned long page_count,
> +                                          unsigned int flush_flags)
>  {
>       struct arm_smmu_xen_domain *smmu_domain = dom_iommu(d)-
> >arch.priv;
>       struct iommu_domain *cfg;
> 
> +     ASSERT(flush_flags);
> +
> +     /* ARM SMMU v1 doesn't have flush by VMA and VMID */
> +
>       spin_lock(&smmu_domain->lock);
>       list_for_each_entry(cfg, &smmu_domain->contexts, list) {
>               /*
> @@ -2670,16 +2676,6 @@ static int __must_check arm_smmu_iotlb_f
>       return 0;
>  }
> 
> -static int __must_check arm_smmu_iotlb_flush(struct domain *d, dfn_t dfn,
> -                                          unsigned long page_count,
> -                                          unsigned int flush_flags)
> -{
> -     ASSERT(flush_flags);
> -
> -     /* ARM SMMU v1 doesn't have flush by VMA and VMID */
> -     return arm_smmu_iotlb_flush_all(d);
> -}
> -
>  static struct iommu_domain *arm_smmu_get_domain(struct domain *d,
>                                               struct device *dev)
>  {
> @@ -2879,7 +2875,6 @@ static const struct iommu_ops arm_smmu_i
>      .add_device = arm_smmu_dt_add_device_generic,
>      .teardown = arm_smmu_iommu_domain_teardown,
>      .iotlb_flush = arm_smmu_iotlb_flush,
> -    .iotlb_flush_all = arm_smmu_iotlb_flush_all,
>      .assign_device = arm_smmu_assign_dev,
>      .reassign_device = arm_smmu_reassign_dev,
>      .map_page = arm_iommu_map_page,
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -3431,7 +3431,6 @@ static const struct iommu_ops arm_smmu_i
>       .hwdom_init             = arm_smmu_iommu_hwdom_init,
>       .teardown               =
> arm_smmu_iommu_xen_domain_teardown,
>       .iotlb_flush            = arm_smmu_iotlb_flush,
> -     .iotlb_flush_all        = arm_smmu_iotlb_flush_all,
>       .assign_device          = arm_smmu_assign_dev,
>       .reassign_device        = arm_smmu_reassign_dev,
>       .map_page               = arm_iommu_map_page,
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -455,15 +455,12 @@ int iommu_iotlb_flush_all(struct domain
>      const struct domain_iommu *hd = dom_iommu(d);
>      int rc;
> 
> -    if ( !is_iommu_enabled(d) || !hd->platform_ops->iotlb_flush_all ||
> +    if ( !is_iommu_enabled(d) || !hd->platform_ops->iotlb_flush ||
>           !flush_flags )
>          return 0;
> 
> -    /*
> -     * The operation does a full flush so we don't need to pass the
> -     * flush_flags in.
> -     */
> -    rc = iommu_call(hd->platform_ops, iotlb_flush_all, d);
> +    rc = iommu_call(hd->platform_ops, iotlb_flush, d, INVALID_DFN, 0,
> +                    flush_flags | IOMMU_FLUSHF_all);
>      if ( unlikely(rc) )
>      {
>          if ( !d->is_shutting_down && printk_ratelimit() )
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -814,18 +814,21 @@ static int __must_check iommu_flush_iotl
>                                                  unsigned long page_count,
>                                                  unsigned int flush_flags)
>  {
> -    ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
> -    ASSERT(flush_flags);
> +    if ( flush_flags & IOMMU_FLUSHF_all )
> +    {
> +        dfn = INVALID_DFN;
> +        page_count = 0;
> +    }
> +    else
> +    {
> +        ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
> +        ASSERT(flush_flags);
> +    }
> 
>      return iommu_flush_iotlb(d, dfn, flush_flags & IOMMU_FLUSHF_modified,
>                               page_count);
>  }
> 
> -static int __must_check iommu_flush_iotlb_all(struct domain *d)
> -{
> -    return iommu_flush_iotlb(d, INVALID_DFN, 0, 0);
> -}
> -
>  static void queue_free_pt(struct domain *d, mfn_t mfn, unsigned int level)
>  {
>      if ( level > 1 )
> @@ -2928,7 +2931,7 @@ static int __init intel_iommu_quarantine
>      spin_unlock(&hd->arch.mapping_lock);
> 
>      if ( !rc )
> -        rc = iommu_flush_iotlb_all(d);
> +        rc = iommu_flush_iotlb(d, INVALID_DFN, 0, 0);
> 
>      /* Pages may be leaked in failure case */
>      return rc;
> @@ -2961,7 +2964,6 @@ static struct iommu_ops __initdata vtd_o
>      .resume = vtd_resume,
>      .crash_shutdown = vtd_crash_shutdown,
>      .iotlb_flush = iommu_flush_iotlb_pages,
> -    .iotlb_flush_all = iommu_flush_iotlb_all,
>      .get_reserved_device_memory =
> intel_iommu_get_reserved_device_memory,
>      .dump_page_tables = vtd_dump_page_tables,
>  };
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -147,9 +147,11 @@ enum
>  {
>      _IOMMU_FLUSHF_added,
>      _IOMMU_FLUSHF_modified,
> +    _IOMMU_FLUSHF_all,
>  };
>  #define IOMMU_FLUSHF_added (1u << _IOMMU_FLUSHF_added)
>  #define IOMMU_FLUSHF_modified (1u << _IOMMU_FLUSHF_modified)
> +#define IOMMU_FLUSHF_all (1u << _IOMMU_FLUSHF_all)
> 
>  int __must_check iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
>                             unsigned long page_count, unsigned int flags,
> @@ -282,7 +284,6 @@ struct iommu_ops {
>      int __must_check (*iotlb_flush)(struct domain *d, dfn_t dfn,
>                                      unsigned long page_count,
>                                      unsigned int flush_flags);
> -    int __must_check (*iotlb_flush_all)(struct domain *d);
>      int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
>      void (*dump_page_tables)(struct domain *d);
> 


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.