diff mbox series

[1/5] iommu: Add mm_get_pasid() helper function

Message ID 20230808074944.7825-2-tina.zhang@intel.com
State New
Headers
Series Share sva domains with all devices bound to a mm |

Commit Message

Zhang, Tina Aug. 8, 2023, 7:49 a.m. UTC 
  mm_get_pasid() is for getting mm pasid value.

The motivation is to replace mm->pasid with an iommu private data
structure that is introduced in a later patch.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 arch/x86/kernel/traps.c | 2 +-
 include/linux/iommu.h   | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe Aug. 8, 2023, 3:02 p.m. UTC | #1 
On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
> mm_get_pasid() is for getting mm pasid value.
> 
> The motivation is to replace mm->pasid with an iommu private data
> structure that is introduced in a later patch.

Maybe we should start out by calling it what it actually is:

'mm_get_enqcmd_pasid()'

We can't actually have multiple SVA domains with different PASIDs
until the places wrongly calling this are removed :\

eg, I would expect this series to also come with removing
'pasid_private' from the Intel driver.

The mmu_notifier should be placed in the singular iommu_domain that is
the SVA domain for the mm. Drivers should not attempt to de-duplicate
this, the core code will do it like you are showing in this series.

Jason
Baolu Lu Aug. 9, 2023, 12:31 a.m. UTC | #2 
On 2023/8/8 23:02, Jason Gunthorpe wrote:
> On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
>> mm_get_pasid() is for getting mm pasid value.
>>
>> The motivation is to replace mm->pasid with an iommu private data
>> structure that is introduced in a later patch.
> Maybe we should start out by calling it what it actually is:
> 
> 'mm_get_enqcmd_pasid()'
> 
> We can't actually have multiple SVA domains with different PASIDs
> until the places wrongly calling this are removed :\
> 
> eg, I would expect this series to also come with removing
> 'pasid_private' from the Intel driver.
> 
> The mmu_notifier should be placed in the singular iommu_domain that is
> the SVA domain for the mm. Drivers should not attempt to de-duplicate
> this, the core code will do it like you are showing in this series.

The two tasks mentioned above are part of our plan. They will be
conducted in stages, which is more conducive to review and testing.
This series is just the beginning.

Best regards,
baolu
Tian, Kevin Aug. 9, 2023, 9:47 a.m. UTC | #3 
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, August 8, 2023 11:02 PM
> 
> On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
> > mm_get_pasid() is for getting mm pasid value.
> >
> > The motivation is to replace mm->pasid with an iommu private data
> > structure that is introduced in a later patch.
> 
> Maybe we should start out by calling it what it actually is:
> 
> 'mm_get_enqcmd_pasid()'
> 
> We can't actually have multiple SVA domains with different PASIDs
> until the places wrongly calling this are removed :\
> 

it's kind of egg-chicken problem. mm_get_pasid() is used by all SVA
scenarios beyond enqcmd then calling it mm_get_enqcmd_pasid()
also sounds weird for non-enqcmd case.

unless you were suggesting to just create a new wrapper for this
specific enqcmd path (try_fixup_enqcmd_gp()) then I'm fine. 😊
Jason Gunthorpe Aug. 9, 2023, 12:31 p.m. UTC | #4 
On Wed, Aug 09, 2023 at 09:47:15AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Tuesday, August 8, 2023 11:02 PM
> > 
> > On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
> > > mm_get_pasid() is for getting mm pasid value.
> > >
> > > The motivation is to replace mm->pasid with an iommu private data
> > > structure that is introduced in a later patch.
> > 
> > Maybe we should start out by calling it what it actually is:
> > 
> > 'mm_get_enqcmd_pasid()'
> > 
> > We can't actually have multiple SVA domains with different PASIDs
> > until the places wrongly calling this are removed :\
> > 
> 
> it's kind of egg-chicken problem. mm_get_pasid() is used by all SVA
> scenarios beyond enqcmd then calling it mm_get_enqcmd_pasid()
> also sounds weird for non-enqcmd case.

Well, those are wrong. We need to be fixing them not hide our eyes to
the wrongness.

Michael is cooking a fix for ARM, Tina should come with a fix for
Intel in this series.

Jason
diff mbox series

Patch

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4a817d20ce3b..6e259b11cd87 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -678,7 +678,7 @@  static bool try_fixup_enqcmd_gp(void)
 	if (!mm_valid_pasid(current->mm))
 		return false;
 
-	pasid = current->mm->pasid;
+	pasid = mm_get_pasid(current->mm);
 
 	/*
 	 * Did this thread already have its PASID activated?
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d31642596675..30e4d1ca39b5 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1180,6 +1180,10 @@  static inline bool mm_valid_pasid(struct mm_struct *mm)
 {
 	return mm->pasid != IOMMU_PASID_INVALID;
 }
+static inline u32 mm_get_pasid(struct mm_struct *mm)
+{
+	return mm->pasid;
+}
 void mm_pasid_drop(struct mm_struct *mm);
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm);
@@ -1202,6 +1206,10 @@  static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
 }
 static inline void mm_pasid_init(struct mm_struct *mm) {}
 static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
+static inline u32 mm_get_pasid(struct mm_struct *mm)
+{
+	return IOMMU_PASID_INVALID;
+}
 static inline void mm_pasid_drop(struct mm_struct *mm) {}
 #endif /* CONFIG_IOMMU_SVA */