linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [git pull] IOMMU Updates for Linux v6.4
@ 2023-04-30 11:13 Joerg Roedel
  2023-04-30 20:07 ` Linus Torvalds
  2023-04-30 20:08 ` pr-tracker-bot
  0 siblings, 2 replies; 10+ messages in thread
From: Joerg Roedel @ 2023-04-30 11:13 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Will Deacon, linux-kernel, iommu

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

Hi Linus,

this pull-request is somewhat messier than usual because it has a lot of
conflicts with your tree. I resolved them in a test-merge and sorted it out
for you to compare your solution to mine (mine is also mostly similar to
the one in linux-next).

My resolution is attached and also available as a branch here:

	git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git pull-test

The resolution branch is compile-tested on x86-64 with defconfig and
allmodconfig, both passed.

With that being said:

The following changes since commit 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d:

  Linux 6.3-rc6 (2023-04-09 11:15:57 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git tags/iommu-updates-v6.4

for you to fetch changes up to e51b4198396cd715b140c0e8b259680429ff0cfb:

  Merge branches 'iommu/fixes', 'arm/allwinner', 'arm/exynos', 'arm/mediatek', 'arm/omap', 'arm/renesas', 'arm/rockchip', 'arm/smmu', 'ppc/pamu', 'unisoc', 'x86/vt-d', 'x86/amd', 'core' and 'platform-remove_new' into next (2023-04-14 13:45:50 +0200)

----------------------------------------------------------------
IOMMU Updates for Linux 6.4

Including:

	- Convert to platform remove callback returning void

	- Extend changing default domain to normal group

	- Intel VT-d updates:
	    - Remove VT-d virtual command interface and IOASID
	    - Allow the VT-d driver to support non-PRI IOPF
	    - Remove PASID supervisor request support
	    - Various small and misc cleanups

	- ARM SMMU updates:
	    - Device-tree binding updates:
	        * Allow Qualcomm GPU SMMUs to accept relevant clock properties
	        * Document Qualcomm 8550 SoC as implementing an MMU-500
	        * Favour new "qcom,smmu-500" binding for Adreno SMMUs

	    - Fix S2CR quirk detection on non-architectural Qualcomm SMMU
	      implementations

	    - Acknowledge SMMUv3 PRI queue overflow when consuming events

	    - Document (in a comment) why ATS is disabled for bypass streams

	- AMD IOMMU updates:
	    - 5-level page-table support
	    - NUMA awareness for memory allocations

	- Unisoc driver: Support for reattaching an existing domain

	- Rockchip driver: Add missing set_platform_dma_ops callback

	- Mediatek driver: Adjust the dma-ranges

	- Various other small fixes and cleanups

----------------------------------------------------------------
Abel Vesa (1):
      dt-bindings: arm-smmu: Add compatible for SM8550 SoC

Christophe JAILLET (2):
      iommu/exynos: Use the devm_clk_get_optional() helper
      iommu/vt-d: Do not use GFP_ATOMIC when not needed

Chunyan Zhang (2):
      iommu/sprd: Release dma buffer to avoid memory leak
      iommu/sprd: Add support for reattaching an existing domain

Geert Uytterhoeven (1):
      iommu: Spelling s/cpmxchg64/cmpxchg64/

Jacob Pan (7):
      iommu/vt-d: Remove virtual command interface
      iommu/sva: Move PASID helpers to sva code
      iommu/sva: Remove PASID to mm lookup function
      iommu/sva: Use GFP_KERNEL for pasid allocation
      iommu/ioasid: Rename INVALID_IOASID
      iommu/vt-d: Use non-privileged mode for all PASIDs
      iommu/vt-d: Remove PASID supervisor request support

Jason Gunthorpe (4):
      iommu/sva: Stop using ioasid_set for SVA
      iommu: Remove ioasid infrastructure
      iommu: Make iommu_release_device() static
      iommu: Remove iommu_group_get_by_id()

Jean-Philippe Brucker (1):
      iommu/arm-smmu-v3: Explain why ATS stays disabled with bypass

Jerry Snitselaar (1):
      iommu/amd: Set page size bitmap during V2 domain allocation

Joerg Roedel (2):
      Merge tag 'arm-smmu-updates' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux into arm/smmu
      Merge branches 'iommu/fixes', 'arm/allwinner', 'arm/exynos', 'arm/mediatek', 'arm/omap', 'arm/renesas', 'arm/rockchip', 'arm/smmu', 'ppc/pamu', 'unisoc', 'x86/vt-d', 'x86/amd', 'core' and 'platform-remove_new' into next

Kishon Vijay Abraham I (1):
      iommu/amd: Fix "Guest Virtual APIC Table Root Pointer" configuration in IRTE

Konrad Dybcio (3):
      dt-bindings: arm-smmu: Use qcom,smmu compatible for MMU500 adreno SMMUs
      dt-bindings: arm-smmu: Add SM8350 Adreno SMMU
      dt-bindings: arm-smmu: Document SM61[12]5 GPU SMMU

Lu Baolu (14):
      iommu/ipmmu-vmsa: Call arm_iommu_release_mapping() in release path
      iommu: Split iommu_group_remove_device() into helpers
      iommu: Same critical region for device release and removal
      iommu: Move lock from iommu_change_dev_def_domain() to its caller
      iommu: Replace device_lock() with group->mutex
      iommu: Cleanup iommu_change_dev_def_domain()
      iommu: Use sysfs_emit() for sysfs show
      dmaengine: idxd: Add enable/disable device IOPF feature
      iommu/vt-d: Allow SVA with device-specific IOPF
      iommu/vt-d: Move iopf code from SVA to IOPF enabling path
      iommu/vt-d: Move pfsid and ats_qdep calculation to device probe path
      iommu/vt-d: Move PRI handling to IOPF feature path
      iommu/vt-d: Remove unnecessary checks in iopf disabling path
      iommu/vt-d: Remove extern from function prototypes

Manivannan Sadhasivam (1):
      iommu/arm-smmu-qcom: Limit the SMR groups to 128

Nick Alcock (1):
      iommu/sun50i: remove MODULE_LICENSE in non-modules

Randy Dunlap (1):
      iommu/fsl: fix all kernel-doc warnings in fsl_pamu.c

Rob Herring (2):
      iommu: Use of_property_present() for testing DT property presence
      iommu/omap: Use of_property_read_bool() for boolean properties

Steven Price (1):
      iommu/rockchip: Add missing set_platform_dma_ops callback

Thomas Weißschuh (1):
      iommu: Make kobj_type structure constant

Tina Zhang (7):
      iommu/vt-d: Make size of operands same in bitwise operations
      iommu/vt-d: Remove BUG_ON on checking valid pfn range
      iommu/vt-d: Remove BUG_ON in handling iotlb cache invalidation
      iommu/vt-d: Remove BUG_ON when domain->pgd is NULL
      iommu/vt-d: Remove BUG_ON in map/unmap()
      iommu/vt-d: Remove a useless BUG_ON(dev->is_virtfn)
      iommu/vt-d: Remove BUG_ON in dmar_insert_dev_scope()

Tomas Krcka (1):
      iommu/arm-smmu-v3: Acknowledge pri/event queue overflow if any

Uwe Kleine-König (10):
      iommu/arm-smmu: Drop if with an always false condition
      iommu/apple-dart: Convert to platform remove callback returning void
      iommu/arm-smmu-v3: Convert to platform remove callback returning void
      iommu/arm-smmu: Convert to platform remove callback returning void
      iommu/ipmmu-vmsa: Convert to platform remove callback returning void
      iommu/msm: Convert to platform remove callback returning void
      iommu/mtk: Convert to platform remove callback returning void
      iommu/mtk_iommu_v1: Convert to platform remove callback returning void
      iommu/omap: Convert to platform remove callback returning void
      iommu/sprd: Convert to platform remove callback returning void

Vasant Hegde (3):
      iommu/amd: Allocate page table using numa locality info
      iommu/amd: Allocate IOMMU irqs using numa locality info
      iommu/amd: Add 5 level guest page table support

Will Deacon (1):
      Merge branch 'for-joerg/arm-smmu/bindings' into for-joerg/arm-smmu/updates

Wolfram Sang (1):
      iommu/ipmmu-vmsa: remove R-Car H3 ES1.* handling

Yong Wu (15):
      iommu/mediatek: Set dma_mask for PGTABLE_PA_35_EN
      dt-bindings: media: mediatek,vcodec: Remove dma-ranges property
      dt-bindings: media: mediatek,jpeg: Remove dma-ranges property
      iommu/mediatek: Improve comment for the current region/bank
      iommu/mediatek: Get regionid from larb/port id
      iommu/mediatek: mt8192: Add iova_region_larb_msk
      iommu/mediatek: mt8195: Add iova_region_larb_msk
      iommu/mediatek: mt8186: Add iova_region_larb_msk
      iommu/mediatek: Add a gap for the iova regions
      iommu/mediatek: Set dma_mask for the master devices
      media: mtk-jpegdec: Remove the setting for dma_mask
      media: mediatek: vcodec: Remove the setting for dma_mask
      arm64: dts: mt8195: Remove the unnecessary dma-ranges
      arm64: dts: mt8195: Add dma-ranges for the parent "soc" node
      arm64: dts: mt8186: Add dma-ranges for the parent "soc" node

Yoshihiro Shimoda (1):
      dt-bindings: iommu: renesas, ipmmu-vmsa: Update for R-Car Gen4

 .../ABI/testing/sysfs-kernel-iommu_groups          |   1 -
 .../devicetree/bindings/iommu/arm,smmu.yaml        |  45 ++-
 .../bindings/iommu/renesas,ipmmu-vmsa.yaml         |  32 +-
 .../bindings/media/mediatek,mt8195-jpegdec.yaml    |   7 -
 .../bindings/media/mediatek,mt8195-jpegenc.yaml    |   7 -
 .../bindings/media/mediatek,vcodec-decoder.yaml    |   5 -
 .../bindings/media/mediatek,vcodec-encoder.yaml    |   5 -
 .../bindings/media/mediatek-jpeg-encoder.yaml      |   5 -
 Documentation/x86/sva.rst                          |   2 +-
 arch/arm64/boot/dts/mediatek/mt8186.dtsi           |   1 +
 arch/arm64/boot/dts/mediatek/mt8195.dtsi           |   4 +-
 arch/x86/kernel/traps.c                            |   2 +-
 drivers/dma/idxd/device.c                          |   8 +-
 drivers/dma/idxd/idxd.h                            |   2 +-
 drivers/dma/idxd/init.c                            |  33 +-
 drivers/dma/idxd/irq.c                             |   2 +-
 drivers/iommu/Kconfig                              |   9 +-
 drivers/iommu/Makefile                             |   1 -
 drivers/iommu/amd/amd_iommu.h                      |   9 +
 drivers/iommu/amd/amd_iommu_types.h                |  12 +-
 drivers/iommu/amd/init.c                           |  30 +-
 drivers/iommu/amd/io_pgtable.c                     |   4 +-
 drivers/iommu/amd/io_pgtable_v2.c                  |  25 +-
 drivers/iommu/amd/iommu.c                          |  17 +-
 drivers/iommu/apple-dart.c                         |   6 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c        |  32 +-
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c         |  16 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c              |  14 +-
 drivers/iommu/arm/arm-smmu/qcom_iommu.c            |  12 +-
 drivers/iommu/exynos-iommu.c                       |  24 +-
 drivers/iommu/fsl_pamu.c                           |   9 +-
 drivers/iommu/intel/cap_audit.c                    |   2 -
 drivers/iommu/intel/dmar.c                         |  13 +-
 drivers/iommu/intel/iommu.c                        | 277 ++++++--------
 drivers/iommu/intel/iommu.h                        |  35 +-
 drivers/iommu/intel/irq_remapping.c                |   2 +-
 drivers/iommu/intel/pasid.c                        |  43 ---
 drivers/iommu/intel/pasid.h                        |   7 -
 drivers/iommu/intel/svm.c                          |   3 +-
 drivers/iommu/ioasid.c                             | 422 ---------------------
 drivers/iommu/iommu-sva.c                          |  61 +--
 drivers/iommu/iommu-sva.h                          |   4 -
 drivers/iommu/iommu.c                              | 347 +++++++----------
 drivers/iommu/ipmmu-vmsa.c                         |  23 +-
 drivers/iommu/msm_iommu.c                          |   5 +-
 drivers/iommu/mtk_iommu.c                          | 158 ++++++--
 drivers/iommu/mtk_iommu_v1.c                       |   5 +-
 drivers/iommu/omap-iommu.c                         |   7 +-
 drivers/iommu/rockchip-iommu.c                     |  61 ++-
 drivers/iommu/sprd-iommu.c                         |  60 ++-
 drivers/iommu/sun50i-iommu.c                       |   1 -
 .../media/platform/mediatek/jpeg/mtk_jpeg_core.c   |   3 -
 .../platform/mediatek/vcodec/mtk_vcodec_dec_drv.c  |   8 -
 .../platform/mediatek/vcodec/mtk_vcodec_enc_drv.c  |   3 -
 include/linux/ioasid.h                             |  83 ----
 include/linux/iommu.h                              |  21 +-
 include/linux/sched/mm.h                           |  26 --
 kernel/fork.c                                      |   1 +
 mm/init-mm.c                                       |   4 +-
 59 files changed, 789 insertions(+), 1277 deletions(-)
 delete mode 100644 drivers/iommu/ioasid.c
 delete mode 100644 include/linux/ioasid.h

Please pull.

Thanks,

	Joerg

diff --cc arch/x86/kernel/process_64.c
index 223b223f713f,bb65a68b4b49..3d181c16a2f6
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@@ -39,6 -39,6 +39,7 @@@
  #include <linux/io.h>
  #include <linux/ftrace.h>
  #include <linux/syscalls.h>
++#include <linux/iommu.h>
  
  #include <asm/processor.h>
  #include <asm/pkru.h>
diff --cc drivers/iommu/iommu-sva.c
index dd76a1a09cf7,c434b95dc8eb..5d4bb11dde01
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@@ -10,26 -9,14 +10,14 @@@
  #include "iommu-sva.h"
  
  static DEFINE_MUTEX(iommu_sva_lock);
- static DECLARE_IOASID_SET(iommu_sva_pasid);
+ static DEFINE_IDA(iommu_global_pasid_ida);
  
- /**
-  * iommu_sva_alloc_pasid - Allocate a PASID for the mm
-  * @mm: the mm
-  * @min: minimum PASID value (inclusive)
-  * @max: maximum PASID value (inclusive)
-  *
-  * Try to allocate a PASID for this mm, or take a reference to the existing one
-  * provided it fits within the [@min, @max] range. On success the PASID is
-  * available in mm->pasid and will be available for the lifetime of the mm.
-  *
-  * Returns 0 on success and < 0 on error.
-  */
- int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
+ /* Allocate a PASID for the mm within range (inclusive) */
+ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
  {
  	int ret = 0;
- 	ioasid_t pasid;
  
- 	if (min == INVALID_IOASID || max == INVALID_IOASID ||
 -	if (!pasid_valid(min) || !pasid_valid(max) ||
++	if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID ||
  	    min == 0 || max < min)
  		return -EINVAL;
  
@@@ -44,11 -28,11 +32,12 @@@
  		goto out;
  	}
  
- 	pasid = ioasid_alloc(&iommu_sva_pasid, min, max, mm);
- 	if (pasid == INVALID_IOASID)
- 		ret = -ENOMEM;
- 	else
- 		mm_pasid_set(mm, pasid);
+ 	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
+ 	if (ret < min)
+ 		goto out;
+ 	mm->pasid = ret;
+ 	ret = 0;
++
  out:
  	mutex_unlock(&iommu_sva_lock);
  	return ret;
@@@ -242,3 -205,11 +210,11 @@@ out_put_mm
  
  	return status;
  }
+ 
+ void mm_pasid_drop(struct mm_struct *mm)
+ {
 -	if (likely(!pasid_valid(mm->pasid)))
++	if (likely(!mm_valid_pasid(mm)))
+ 		return;
+ 
+ 	ida_free(&iommu_global_pasid_ida, mm->pasid);
+ }
diff --cc drivers/iommu/iommu.c
index 807c98de40d4,153a3dab568c..f1dcfa3f1a1b
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@@ -88,9 -87,10 +88,10 @@@ static const char * const iommu_group_r
  
  static int iommu_bus_notifier(struct notifier_block *nb,
  			      unsigned long action, void *data);
+ static void iommu_release_device(struct device *dev);
  static int iommu_alloc_default_domain(struct iommu_group *group,
  				      struct device *dev);
 -static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 +static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
  						 unsigned type);
  static int __iommu_attach_device(struct iommu_domain *domain,
  				 struct device *dev);
diff --cc include/linux/iommu.h
index 0fd4e6734d5b,7dbdd13d7ce0..e8c9a7da1060
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@@ -455,12 -455,11 +455,11 @@@ static inline const struct iommu_ops *d
  	return dev->iommu->iommu_dev->ops;
  }
  
 -extern int bus_iommu_probe(struct bus_type *bus);
 -extern bool iommu_present(struct bus_type *bus);
 +extern int bus_iommu_probe(const struct bus_type *bus);
 +extern bool iommu_present(const struct bus_type *bus);
  extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
  extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
 -extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus);
 +extern struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus);
- extern struct iommu_group *iommu_group_get_by_id(int id);
  extern void iommu_domain_free(struct iommu_domain *domain);
  extern int iommu_attach_device(struct iommu_domain *domain,
  			       struct device *dev);
@@@ -1172,7 -1165,17 +1165,16 @@@ static inline bool tegra_dev_iommu_get_
  	return false;
  }
  
 -static inline bool pasid_valid(ioasid_t ioasid)
 -{
 -	return ioasid != IOMMU_PASID_INVALID;
 -}
 -
  #ifdef CONFIG_IOMMU_SVA
+ static inline void mm_pasid_init(struct mm_struct *mm)
+ {
+ 	mm->pasid = IOMMU_PASID_INVALID;
+ }
++static inline bool mm_valid_pasid(struct mm_struct *mm)
++{
++	return mm->pasid != IOMMU_PASID_INVALID;
++}
+ void mm_pasid_drop(struct mm_struct *mm);
  struct iommu_sva *iommu_sva_bind_device(struct device *dev,
  					struct mm_struct *mm);
  void iommu_sva_unbind_device(struct iommu_sva *handle);
@@@ -1192,6 -1195,8 +1194,9 @@@ static inline u32 iommu_sva_get_pasid(s
  {
  	return IOMMU_PASID_INVALID;
  }
+ static inline void mm_pasid_init(struct mm_struct *mm) {}
++static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
+ static inline void mm_pasid_drop(struct mm_struct *mm) {}
  #endif /* CONFIG_IOMMU_SVA */
  
  #endif /* __LINUX_IOMMU_H */
diff --cc kernel/fork.c
index 735d9f4f5acf,e7d10ad98a69..ed4e01daccaa
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@@ -97,7 -97,7 +97,8 @@@
  #include <linux/io_uring.h>
  #include <linux/bpf.h>
  #include <linux/stackprotector.h>
 +#include <linux/user_events.h>
+ #include <linux/iommu.h>
  
  #include <asm/pgalloc.h>
  #include <linux/uaccess.h>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-04-30 11:13 [git pull] IOMMU Updates for Linux v6.4 Joerg Roedel
@ 2023-04-30 20:07 ` Linus Torvalds
  2023-04-30 23:06   ` Jason Gunthorpe
  2023-05-05 14:02   ` Joerg Roedel
  2023-04-30 20:08 ` pr-tracker-bot
  1 sibling, 2 replies; 10+ messages in thread
From: Linus Torvalds @ 2023-04-30 20:07 UTC (permalink / raw)
  To: Joerg Roedel, Jason Gunthorpe; +Cc: Will Deacon, linux-kernel, iommu

On Sun, Apr 30, 2023 at 4:13 AM Joerg Roedel <joro@8bytes.org> wrote:
>
> this pull-request is somewhat messier than usual because it has a lot of
> conflicts with your tree. I resolved them in a test-merge and sorted it out
> for you to compare your solution to mine (mine is also mostly similar to
> the one in linux-next).

Your resolution is different from mine.

Some of it is just white-space differences etc, but some of it is meaningful.

For example, you have

                if (mm->pasid < min || mm->pasid >= max)

in your iommu_sva_alloc_pasid(), which seems to have undone the change
in commit 4e14176ab13f ("iommu/sva: Stop using ioasid_set for SVA"),
which changed it to check for

           .. mm->pasid > max)

instead (which seems also consistent with what ida_alloc_range() does:
'max' is inclusive).

You also seem to have kept the deleted <linux/ioasid.h> header file.

I'm also a bit unsure about what the intent with mm_valid_pasid() is.
In commit cd3891158a77 ("iommu/sva: Move PASID helpers to sva code")
that helper (under the previous name) got moved to a different header
file, but in the process it also got done unconditionally as

  static inline bool pasid_valid(ioasid_t ioasid)
  {
         return ioasid != INVALID_IOASID;
  }

and didn't have a "ioasid is disabled in the config, so have an
alternate helper that always returns false".

But in your merge, you ended up splitting it into two versions again.

I don't think that's technically the "right" merge (it basically
changes things wrt the two branches), but I do think it's nicer.

So I edited my merge to follow that lead.

Finally, I'm not happy with the Kconfig situation here. Commit
99b5726b4423 ("iommu: Remove ioasid infrastructure") removed
CONFIG_IOASID, but left the

        select IOASID

in the 'config INTEL_IOMMU' Kconfig case. I removed that as dead, but
now we have that

        select IOMMU_SVA

in the 'config INTEL_IOMMU_SVM' case instead. So it's a very different
Kconfig setup.

Anyway, I'm not super-happy with how this all turned out. The example
merge seems to be wrong, and the Kconfig situation is confusing.

Somebody should double-check my result, in other words.

                Linus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-04-30 11:13 [git pull] IOMMU Updates for Linux v6.4 Joerg Roedel
  2023-04-30 20:07 ` Linus Torvalds
@ 2023-04-30 20:08 ` pr-tracker-bot
  1 sibling, 0 replies; 10+ messages in thread
From: pr-tracker-bot @ 2023-04-30 20:08 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: Linus Torvalds, Will Deacon, linux-kernel, iommu

The pull request you sent on Sun, 30 Apr 2023 13:13:11 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git pull-test

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/58390c8ce1bddb6c623f62e7ed36383e7fa5c02f

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-04-30 20:07 ` Linus Torvalds
@ 2023-04-30 23:06   ` Jason Gunthorpe
  2023-05-06  4:09     ` Jacob Pan
  2023-05-05 14:02   ` Joerg Roedel
  1 sibling, 1 reply; 10+ messages in thread
From: Jason Gunthorpe @ 2023-04-30 23:06 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Joerg Roedel, Will Deacon, linux-kernel, iommu

On Sun, Apr 30, 2023 at 01:07:45PM -0700, Linus Torvalds wrote:
> On Sun, Apr 30, 2023 at 4:13 AM Joerg Roedel <joro@8bytes.org> wrote:
> >
> > this pull-request is somewhat messier than usual because it has a lot of
> > conflicts with your tree. I resolved them in a test-merge and sorted it out
> > for you to compare your solution to mine (mine is also mostly similar to
> > the one in linux-next).
> 
> Your resolution is different from mine.
> 
> Some of it is just white-space differences etc, but some of it is meaningful.
> 
> For example, you have
> 
>                 if (mm->pasid < min || mm->pasid >= max)
> 
> in your iommu_sva_alloc_pasid(), which seems to have undone the change
> in commit 4e14176ab13f ("iommu/sva: Stop using ioasid_set for SVA"),
> which changed it to check for
> 
>            .. mm->pasid > max)
> 
> instead (which seems also consistent with what ida_alloc_range() does:
> 'max' is inclusive).

Yes, that is what the new function comment says it should do, and the
only caller is:

drivers/iommu/iommu-sva.c:      ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);

Which looks inclusive also

> You also seem to have kept the deleted <linux/ioasid.h> header file.

Should be deleted
 
> I'm also a bit unsure about what the intent with mm_valid_pasid() is.
> In commit cd3891158a77 ("iommu/sva: Move PASID helpers to sva code")
> that helper (under the previous name) got moved to a different header
> file, but in the process it also got done unconditionally as

I think the whole thing was originally a micro-optimization to remove
this if statement from some mm paths..

> But in your merge, you ended up splitting it into two versions again.
> 
> I don't think that's technically the "right" merge (it basically
> changes things wrt the two branches), but I do think it's nicer.

It is closer to the intent, I think

> Finally, I'm not happy with the Kconfig situation here. Commit
> 99b5726b4423 ("iommu: Remove ioasid infrastructure") removed
> CONFIG_IOASID, but left the
> 
>         select IOASID
 
> in the 'config INTEL_IOMMU' Kconfig case. I removed that as dead, but
> now we have that
> 
>         select IOMMU_SVA

We've had this longstanding confusion in the iommu layer that SVA and
PASID are one and the same thing, we are slowly reorganizing it.. For
now it is fine for IOMMU_SVA to cover the PASID allocator as the only
drivers that support PASID also support SVA.

Arguably the design is backwards and IOMMU_SVA should be user
selectable and it should turn off the SVA code entirely including the
driver code.

> Somebody should double-check my result, in other words.

I didn't notice anything wrong, I'm sure Lu and Yi will test it!

Thanks,
Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-04-30 20:07 ` Linus Torvalds
  2023-04-30 23:06   ` Jason Gunthorpe
@ 2023-05-05 14:02   ` Joerg Roedel
  2023-05-05 18:03     ` Linus Torvalds
  1 sibling, 1 reply; 10+ messages in thread
From: Joerg Roedel @ 2023-05-05 14:02 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Jason Gunthorpe, Will Deacon, linux-kernel, iommu

Hi Linus,

On Sun, Apr 30, 2023 at 01:07:45PM -0700, Linus Torvalds wrote:
> Anyway, I'm not super-happy with how this all turned out. The example
> merge seems to be wrong, and the Kconfig situation is confusing.

You are right, looks like I missed some context when doing my example
merge, sorry for that. I will take more care next time.

Regards,

	Joerg

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-05-05 14:02   ` Joerg Roedel
@ 2023-05-05 18:03     ` Linus Torvalds
  2023-05-05 19:57       ` Jason Gunthorpe
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2023-05-05 18:03 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: Jason Gunthorpe, Will Deacon, linux-kernel, iommu

On Fri, May 5, 2023 at 7:02 AM Joerg Roedel <joro@8bytes.org> wrote:
>
> On Sun, Apr 30, 2023 at 01:07:45PM -0700, Linus Torvalds wrote:
> > Anyway, I'm not super-happy with how this all turned out. The example
> > merge seems to be wrong, and the Kconfig situation is confusing.
>
> You are right, looks like I missed some context when doing my example
> merge, sorry for that. I will take more care next time.

So I find the suggested merges useful even if they are wrong - it just
makes me look at what I did in an area that is clearly non-obvious.

And the part that made me unhappy about the merge was actually how
non-obvious it was, not so much the mis-merge itself.

That config option rename in particular I find to just be bad. We now
have some code that is *very* central, to the point where we have a
field for it in the 'struct mm_struct', and special callback for
fork() and exit(), and then the config option is called something
completely incomprehensible like 'IOMMU_SVA'?

That just smells to high heaven to me.

Now, I'm not saying that 'struct mm_struct' is some holy thing, and it
has a lot of other strange fields that get enabled by strange config
options. But I *am* saying that it's one of our most central data
structures in the kernel (just behind 'struct task_struct'), and when
it has something like this in it:

    #ifdef CONFIG_IOMMU_SVA
                u32 pasid;
    #endif

that just doesn't look right. "IOMMU_SVA" just isn't an obvious or
descriptive enough name to be involved in that data structure.

The old name ("CONFIG_IOMMU_SUPPORT") honestly made a ton more sense
in that context.

So I was a bit frustrated with the merge.  When there are merge
conflicts, the way I resolve code is to make the resolution make
sense. Most of the time that's completely trivial, then occasionally I
have to look at the code history to see what people were doing.

And in this case, I didn't feel like the end result made sense, if you
see what I mean. The resolution is _right_, but I'm not happy about
it.

                 Linus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-05-05 18:03     ` Linus Torvalds
@ 2023-05-05 19:57       ` Jason Gunthorpe
  2023-05-05 20:10         ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Jason Gunthorpe @ 2023-05-05 19:57 UTC (permalink / raw)
  To: Linus Torvalds, Kevin Tian; +Cc: Joerg Roedel, Will Deacon, linux-kernel, iommu

On Fri, May 05, 2023 at 11:03:46AM -0700, Linus Torvalds wrote:

> That config option rename in particular I find to just be bad. We now
> have some code that is *very* central, to the point where we have a
> field for it in the 'struct mm_struct', and special callback for
> fork() and exit(), and then the config option is called something
> completely incomprehensible like 'IOMMU_SVA'?

The purpose of this field is to enable the new Intel ENQCMD
instruction that requries the arch code to put the processes PASID
value into some MSR and keep it there across context switches. See
commit fa6af69f38d3 ("x86/traps: Demand-populate PASID MSR via #GP")

ENQCMD is used when the IOMMU page table points directly at the CPU
page table (Shared Virtual Addressing) and supports some simple
stateless "PCI" devices that Intel has designed.

At least with the current situation CONFIG_INTEL_ENQCMD might be an
appropriate name, split out from the IOMMU kconfig and put in arch
kconfig?

Ideally we wouldn't need this on today's ARM systems, for instance.

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-05-05 19:57       ` Jason Gunthorpe
@ 2023-05-05 20:10         ` Linus Torvalds
  2023-05-06  2:58           ` Tian, Kevin
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2023-05-05 20:10 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Kevin Tian, Joerg Roedel, Will Deacon, linux-kernel, iommu

On Fri, May 5, 2023 at 12:57 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> At least with the current situation CONFIG_INTEL_ENQCMD might be an
> appropriate name, split out from the IOMMU kconfig and put in arch
> kconfig?

That would at least somewhat clarify the use. I find IOMMU_SVA to be a
particularly opaque name.

Admittedly I probably find it opaque because I come at it as somebody
much more familiar with the MM side, not from the IOMMU side (and the
_other_ conditional fields there make sense from that standpoint), but
even so I think it would be good to clarify.

Of course, making clear it's architecture-specific would also be an
argument for having an actual architecture-specific part of 'struct
mm_struct' (the same way we have 'struct thread_info' in task_struct),
but with only one single field I suspect that's just not worth it.

And who knows - maybe other architectures will pick something like this up?

But yes, it might be good to really pin down what the rules are. Right
now IOMMU_SVA has *no* real help-text (there's a comment saying "#
Shared Virtual Addressing"), and is also enabled by ARM_SMMU_V3_SVA,
which apparently doesn't actually want pasid support at all.

I dunno. I don't think this is a huge deal, but it did cause some
confusion during the merge.

                 Linus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [git pull] IOMMU Updates for Linux v6.4
  2023-05-05 20:10         ` Linus Torvalds
@ 2023-05-06  2:58           ` Tian, Kevin
  0 siblings, 0 replies; 10+ messages in thread
From: Tian, Kevin @ 2023-05-06  2:58 UTC (permalink / raw)
  To: Torvalds, Linus, Jason Gunthorpe, jean-philippe
  Cc: Joerg Roedel, Will Deacon, linux-kernel, iommu

> From: Linus Torvalds <torvalds@linux-foundation.org>
> Sent: Saturday, May 6, 2023 4:11 AM
> 
> On Fri, May 5, 2023 at 12:57 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > At least with the current situation CONFIG_INTEL_ENQCMD might be an
> > appropriate name, split out from the IOMMU kconfig and put in arch
> > kconfig?
> 
> That would at least somewhat clarify the use. I find IOMMU_SVA to be a
> particularly opaque name.
> 
> Admittedly I probably find it opaque because I come at it as somebody
> much more familiar with the MM side, not from the IOMMU side (and the
> _other_ conditional fields there make sense from that standpoint), but
> even so I think it would be good to clarify.
> 
> Of course, making clear it's architecture-specific would also be an
> argument for having an actual architecture-specific part of 'struct
> mm_struct' (the same way we have 'struct thread_info' in task_struct),
> but with only one single field I suspect that's just not worth it.
> 
> And who knows - maybe other architectures will pick something like this up?

Looks ARM has ST64BV/ST64BV0 similar to ENQCMD according to [1].

Looking at ARM spec [2]:

  Single-copy Atomic 64-byte EL0 Store with Return stores eight 64-bit
  doublewords from consecutive registers, Xt to X(t+7), to a memory
  location, with the bottom 32 bits taken from ACCDATA_EL1, and writes
  the status result of the store to a register.

ACCDATA_EL1 sounds like the PASID_MSR used in ENQCMD, though it's
not explicitly stated so...

With that CONFIG_CPU_PASID might be more future-proof based on
the semantics of those ENQCMD-like instructions?

But...

> 
> But yes, it might be good to really pin down what the rules are. Right
> now IOMMU_SVA has *no* real help-text (there's a comment saying "#
> Shared Virtual Addressing"), and is also enabled by ARM_SMMU_V3_SVA,
> which apparently doesn't actually want pasid support at all.

... ARM_SMMU_V3_SVA does require pasid support. Just no ENQCMD.

SVA allows the device to access CPU virtual addresses with pasid to
identify a specific CPU page table when the device is shared by many
processes. e.g. each work queue in the device can be associated with
a unique pasid pointing to the CPU page table of the owning process.

w/o ENQCMD mm->pasid is not strictly necessary. IOMMU driver can
store the pasid information in its own structure when a CPU page
table is associated to a device.

ENQCMD further allows a single work queue shared by many processes,
by storing pasid in a MSR and sending it in the wire so the work
queue can be stateless. This needs to store the pasid in mm to handle
#GP, fork(), etc.

But after mm->pasid was introduced in 52ad9bc64c74 ("mm: Add a
pasid member to struct mm_struct "), it then became the common
storage for SVA beyond ENQCMD and common sva helpers are built
around mm->pasid, e.g. iommu_sva_bind_device().

From this angle using IOMMU_SVA to guard mm->pasid still makes
sense to me. But we do need to add the missing help-text for it.

> 
> I dunno. I don't think this is a huge deal, but it did cause some
> confusion during the merge.
> 
>                  Linus

[1] https://stackoverflow.com/questions/70561491/temporality-of-st64b-and-movdir64b
[2] https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/ST64BV0--Single-copy-Atomic-64-byte-EL0-Store-with-Return-

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [git pull] IOMMU Updates for Linux v6.4
  2023-04-30 23:06   ` Jason Gunthorpe
@ 2023-05-06  4:09     ` Jacob Pan
  0 siblings, 0 replies; 10+ messages in thread
From: Jacob Pan @ 2023-05-06  4:09 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Linus Torvalds, Joerg Roedel, Will Deacon, linux-kernel, iommu,
	jacob.jun.pan

Hi Jason,

On Sun, 30 Apr 2023 20:06:30 -0300, Jason Gunthorpe <jgg@ziepe.ca> wrote:

> We've had this longstanding confusion in the iommu layer that SVA and
> PASID are one and the same thing, we are slowly reorganizing it.. For
> now it is fine for IOMMU_SVA to cover the PASID allocator as the only
> drivers that support PASID also support SVA.
> 
> Arguably the design is backwards and IOMMU_SVA should be user
> selectable and it should turn off the SVA code entirely including the
> driver code.
> 
> > Somebody should double-check my result, in other words.  
> 
> I didn't notice anything wrong, I'm sure Lu and Yi will test it!
FWIW, I did a quick test with SVA ENQCMD on an Intel Data Streaming
Accelerator (DSA) shared work queue, seems to work fine. Code looks good to
me too.

Thanks,

Jacob

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-05-06  4:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-30 11:13 [git pull] IOMMU Updates for Linux v6.4 Joerg Roedel
2023-04-30 20:07 ` Linus Torvalds
2023-04-30 23:06   ` Jason Gunthorpe
2023-05-06  4:09     ` Jacob Pan
2023-05-05 14:02   ` Joerg Roedel
2023-05-05 18:03     ` Linus Torvalds
2023-05-05 19:57       ` Jason Gunthorpe
2023-05-05 20:10         ` Linus Torvalds
2023-05-06  2:58           ` Tian, Kevin
2023-04-30 20:08 ` pr-tracker-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).