Re: [PATCH] iommu/vt-d: Remove comment reference to iommu_dev_has_feature
On 2/2/22 10:37 AM, Akeem G Abodunrin wrote:
iommu_dev_has_feature() api has been removed by the commit 262948f8ba573
("iommu: Delete iommu_dev_has_feature()") - So this patch removes comment
about the api to avoid any confusion.
Signed-off-by: Akeem G Abodunrin
Cc: Lu Baolu
It's not a change for iommu/vt-d, but for iommu core.
Please add Joerg in the to list.
Best regards,
baolu
---
include/linux/iommu.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index de0c57a567c8..bea054f2bd4d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -153,8 +153,7 @@ struct iommu_resv_region {
* supported, this feature must be enabled before and
* disabled after %IOMMU_DEV_FEAT_SVA.
*
- * Device drivers query whether a feature is supported using
- * iommu_dev_has_feature(), and enable it using iommu_dev_enable_feature().
+ * Device drivers enable the feature via iommu_dev_enable_feature().
*/
enum iommu_dev_features {
IOMMU_DEV_FEAT_AUX,
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On 2/5/22 1:10 PM, Fenghua Yu wrote: Hi, Baolu, On Sat, Feb 05, 2022 at 11:50:59AM +0800, Lu Baolu wrote: Hi Fenghua, On 2022/1/29 4:28, Fenghua Yu wrote: diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 92fea3fbbb11..ef03b2176bbd 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4781,7 +4781,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain, link_failed: spin_unlock_irqrestore(_domain_lock, flags); if (list_empty(>subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); return ret; } @@ -4811,7 +4811,7 @@ static void aux_domain_remove_dev(struct dmar_domain *domain, spin_unlock_irqrestore(_domain_lock, flags); if (list_empty(>subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); } static int prepare_domain_attach_device(struct iommu_domain *domain, The domain->default_pasid is not relevant to SVA and it's being cleaned up by another series. No need to take care of it in this series. Because ioasid_put() is renamed to ioasid_free() in this patch, without above changes, this series cannot be compiled. Thomas and I discussed how to handle aux_domain while you will remove the entire aux_domain code (https://lore.kernel.org/lkml/87zgnf29op.ffs@tglx/). The above changes are minimal and temporary changes to compile this series. The changes will be removed along with the entire aux_domain by your removing aux_domain series later in 5.18. Okay. Make sense to me. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi, Baolu, On Sat, Feb 05, 2022 at 11:50:59AM +0800, Lu Baolu wrote: > Hi Fenghua, > > On 2022/1/29 4:28, Fenghua Yu wrote: > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > index 92fea3fbbb11..ef03b2176bbd 100644 > > --- a/drivers/iommu/intel/iommu.c > > +++ b/drivers/iommu/intel/iommu.c > > @@ -4781,7 +4781,7 @@ static int aux_domain_add_dev(struct dmar_domain > > *domain, > > link_failed: > > spin_unlock_irqrestore(_domain_lock, flags); > > if (list_empty(>subdevices) && domain->default_pasid > 0) > > - ioasid_put(domain->default_pasid); > > + ioasid_free(domain->default_pasid); > > return ret; > > } > > @@ -4811,7 +4811,7 @@ static void aux_domain_remove_dev(struct dmar_domain > > *domain, > > spin_unlock_irqrestore(_domain_lock, flags); > > if (list_empty(>subdevices) && domain->default_pasid > 0) > > - ioasid_put(domain->default_pasid); > > + ioasid_free(domain->default_pasid); > > } > > static int prepare_domain_attach_device(struct iommu_domain *domain, > > The domain->default_pasid is not relevant to SVA and it's being cleaned > up by another series. No need to take care of it in this series. Because ioasid_put() is renamed to ioasid_free() in this patch, without above changes, this series cannot be compiled. Thomas and I discussed how to handle aux_domain while you will remove the entire aux_domain code (https://lore.kernel.org/lkml/87zgnf29op.ffs@tglx/). The above changes are minimal and temporary changes to compile this series. The changes will be removed along with the entire aux_domain by your removing aux_domain series later in 5.18. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi Fenghua, On 2022/1/29 4:28, Fenghua Yu wrote: diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 92fea3fbbb11..ef03b2176bbd 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4781,7 +4781,7 @@ static int aux_domain_add_dev(struct dmar_domain *domain, link_failed: spin_unlock_irqrestore(_domain_lock, flags); if (list_empty(>subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); return ret; } @@ -4811,7 +4811,7 @@ static void aux_domain_remove_dev(struct dmar_domain *domain, spin_unlock_irqrestore(_domain_lock, flags); if (list_empty(>subdevices) && domain->default_pasid > 0) - ioasid_put(domain->default_pasid); + ioasid_free(domain->default_pasid); } static int prepare_domain_attach_device(struct iommu_domain *domain, The domain->default_pasid is not relevant to SVA and it's being cleaned up by another series. No need to take care of it in this series. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] IOMMU: Intel: DMAR: Replace acpi_bus_get_device()
On 2022/2/2 2:11, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki
Replace acpi_bus_get_device() that is going to be dropped with
acpi_fetch_acpi_dev().
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki
---
drivers/iommu/intel/dmar.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/iommu/intel/dmar.c
===
--- linux-pm.orig/drivers/iommu/intel/dmar.c
+++ linux-pm/drivers/iommu/intel/dmar.c
@@ -789,7 +789,8 @@ static int __init dmar_acpi_dev_scope_in
andd->device_name);
continue;
}
- if (acpi_bus_get_device(h, )) {
+ adev = acpi_fetch_acpi_dev(h);
+ if (!adev) {
pr_err("Failed to get device for ACPI object
%s\n",
andd->device_name);
continue;
Please adjust the patch title to "iommu/vtd: Replace acpi_bus_get_device()"
Reviewed-by: Lu Baolu
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On Sat, Feb 05, 2022 at 12:56:00AM +0100, Thomas Gleixner wrote: > On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > > To avoid complexity of updating each thread's PASID status (e.g. sending > > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > > allocated and assigned to an mm, the PASID stays with the mm for the > > rest of the mm's lifetime. A reference to the PASID is taken on > > allocating the PASID. Binding/unbinding the PASID won't change refcount. > > The reference is dropped on mm exit and thus the PASID is freed. > > > > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > > the PASID operations handle the pasid member in mm_struct and should be > > part of mm operations. Because IOASID's reference count is not used any > > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > > are deleted and ioasid_put() is renamed to ioasid_free(). > > > > 20-bit PASID allows up to 1M processes bound to PASIDs at the same time. > > With cgroups and other controls that might limit the number of process > > creation, the limited number of PASIDs is not a realistic issue for > > lazy PASID free. > > Please take a step back and think hard about it whether that changelog > makes sense to you a year from now. > > Let me walk you through: > > > To avoid complexity of updating each thread's PASID status (e.g. sending > > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > > allocated and assigned to an mm, the PASID stays with the mm for the > > rest of the mm's lifetime. > > You are missing the oportunity to tell a story about the history of this > decision here: > > PASIDs are process wide. It was attempted to use refcounted PASIDs to > free them when the last thread drops the refcount. This turned out to > be complex and error prone. Given the fact that the PASID space is 20 > bits, which allows up to 1M processes to have a PASID associated > concurrently, PASID resource exhaustion is not a realistic concern. > > Therefore it was decided to simplify the approach and stick with lazy > on demand PASID allocation, but drop the eager free approach and make > a allocated PASID lifetime bound to the life time of the process. > > > A reference to the PASID is taken on allocating the > > PASID. Binding/unbinding the PASID won't change refcount. The > > reference is dropped on mm exit and thus the PASID is freed. > > There is no refcount in play anymore, right? So how does this part of > the changelog make any sense? > > This is followed by: > > > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > > the PASID operations handle the pasid member in mm_struct and should be > > part of mm operations. Because IOASID's reference count is not used any > > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > > are deleted and ioasid_put() is renamed to ioasid_free(). > > which does not provide much rationale and just blurbs about _what_ the > patch is doing and not about the why and the connection to the > above. And the refcount removal section contradicts the stale text > above. > > So this paragraph should be something like this: > > Get rid of the refcounting mechanisms and replace/rename the > interfaces to reflect this new approach. > > Hmm? Sure. I will update the commit message with your comments. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 04/11] kernel/fork: Initialize mm's PASID
Hi, Thomas,
On Sat, Feb 05, 2022 at 12:22:12AM +0100, Thomas Gleixner wrote:
> On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote:
> > A new mm doesn't have a PASID yet when it's created. Initialize
> > the mm's PASID on fork() or for init_mm to INVALID_IOASID (-1).
>
> I must be missing something here.
>
> > diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> > index aa5f09ca5bcf..c74d1edbac2f 100644
> > --- a/include/linux/sched/mm.h
> > +++ b/include/linux/sched/mm.h
> > @@ -8,6 +8,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Routines for handling mm_structs
> > @@ -433,4 +434,13 @@ static inline void membarrier_update_current_mm(struct
> > mm_struct *next_mm)
> > }
> > #endif
> >
> > +#ifdef CONFIG_IOMMU_SVA
> > +static inline void mm_pasid_init(struct mm_struct *mm)
> > +{
> > + mm->pasid = INVALID_IOASID;
> > +}
> > +#else
> > +static inline void mm_pasid_init(struct mm_struct *mm) {}
> > +#endif
> > +
> > #endif /* _LINUX_SCHED_MM_H */
>
> So this adds mm_pasid_init() to linux/sched/mm.h which replaces:
>
> > -static void mm_init_pasid(struct mm_struct *mm)
> > -{
> > -#ifdef CONFIG_IOMMU_SVA
> > - mm->pasid = INIT_PASID;
> > -#endif
> > -}
> > -
>
> I.e. already existing code which is initializing mm->pasid with
> INIT_PASID (0) while the replacement initializes it with INVALID_IOASID
> (-1).
>
> The change log does not have any information about why INIT_PASID is the
> wrong initialization value and why this change is not having any side
> effects.
I should add the following info in the commit message to explain why
change INIT_PASID (0) to INVALID_IOASID (-1):
INIT_PASID (0) is reserved for kernel legacy DMA PASID. It cannot be
allocated to a user process. Initialize the process's PASID to 0 may
cause confusion that why the process uses reserved kernel legacy DMA
PASID. Initializing the PASID to INVALID_IOASID (-1) explicitly
tells the process doesn't have a valid PASID yet initially.
Is it OK for you?
>
> It neither mentions why having this in a global available header makes
> sense when the only call site is in the C file from which the already
> existing function is removed.
This series defines three helpers mm_pasid_init(), mm_pasid_set(), and
mm_pasid_drop() in mm because they handle the pasid member in mm_struct
and should be part of mm operations. I explained why mm_pasid_set() and
mm_pasid_drop() are defined in mm, but I didn't explain why mm_pasid_init()
is define in mm.
Is it OK to add the following explanation on why mm_pasid_init() is defined?
mm_pasid_init() is defined in mm and replaces mm_init_pasid() because
the PASID init operation initializes the pasid member in mm_struct and
should be part of mm operations.
Thanks,
-Fenghua
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 09/11] x86/cpufeatures: Re-enable ENQCMD
On Fri, Jan 28 2022 at 12:29, Fenghua Yu wrote: > Since ENQCMD is handled by #GP fix up, it can be re-enabled. > > The ENQCMD feature can only be used if CONFIG_INTEL_IOMMU_SVM is set. Add > X86_FEATURE_ENQCMD to the disabled features mask as appropriate so that > cpu_feature_enabled() can be used to check the feature. > > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 08/11] x86/traps: Demand-populate PASID MSR via #GP
On Fri, Jan 28 2022 at 12:29, Fenghua Yu wrote: > All tasks start with PASID state disabled. This means that the first > time they execute an ENQCMD instruction they will take a #GP fault. > > Modify the #GP fault handler to check if the "mm" for the task has > already been allocated a PASID. If so, try to fix the #GP fault by > loading the IA32_PASID MSR. > > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 07/11] sched: Define and initialize a flag to identify valid PASID in the task
On Fri, Jan 28 2022 at 12:29, Fenghua Yu wrote: > From: Peter Zijlstra > > Add a new single bit field to the task structure to track whether this task > has initialized the IA32_PASID MSR to the mm's PASID. > > Initialize the field to zero when creating a new task with fork/clone. > > Signed-off-by: Peter Zijlstra > Co-developed-by: Fenghua Yu > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 06/11] x86/fpu: Clear PASID when copying fpstate
On Fri, Jan 28 2022 at 12:29, Fenghua Yu wrote: > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > To avoid complexity of updating each thread's PASID status (e.g. sending > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > allocated and assigned to an mm, the PASID stays with the mm for the > rest of the mm's lifetime. A reference to the PASID is taken on > allocating the PASID. Binding/unbinding the PASID won't change refcount. > The reference is dropped on mm exit and thus the PASID is freed. > > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > the PASID operations handle the pasid member in mm_struct and should be > part of mm operations. Because IOASID's reference count is not used any > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > are deleted and ioasid_put() is renamed to ioasid_free(). > > 20-bit PASID allows up to 1M processes bound to PASIDs at the same time. > With cgroups and other controls that might limit the number of process > creation, the limited number of PASIDs is not a realistic issue for > lazy PASID free. Please take a step back and think hard about it whether that changelog makes sense to you a year from now. Let me walk you through: > To avoid complexity of updating each thread's PASID status (e.g. sending > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once > allocated and assigned to an mm, the PASID stays with the mm for the > rest of the mm's lifetime. You are missing the oportunity to tell a story about the history of this decision here: PASIDs are process wide. It was attempted to use refcounted PASIDs to free them when the last thread drops the refcount. This turned out to be complex and error prone. Given the fact that the PASID space is 20 bits, which allows up to 1M processes to have a PASID associated concurrently, PASID resource exhaustion is not a realistic concern. Therefore it was decided to simplify the approach and stick with lazy on demand PASID allocation, but drop the eager free approach and make a allocated PASID lifetime bound to the life time of the process. > A reference to the PASID is taken on allocating the > PASID. Binding/unbinding the PASID won't change refcount. The > reference is dropped on mm exit and thus the PASID is freed. There is no refcount in play anymore, right? So how does this part of the changelog make any sense? This is followed by: > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because > the PASID operations handle the pasid member in mm_struct and should be > part of mm operations. Because IOASID's reference count is not used any > more and removed, unused ioasid_get() and iommu_sva_free_pasid() > are deleted and ioasid_put() is renamed to ioasid_free(). which does not provide much rationale and just blurbs about _what_ the patch is doing and not about the why and the connection to the above. And the refcount removal section contradicts the stale text above. So this paragraph should be something like this: Get rid of the refcounting mechanisms and replace/rename the interfaces to reflect this new approach. Hmm? Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 03/11] iommu/ioasid: Introduce a helper to check for valid PASIDs
On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > pasid_valid() is defined to check if a given PASID is valid. > > Suggested-by: Ashok Raj > Suggested-by: Jacob Pan > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 04/11] kernel/fork: Initialize mm's PASID
On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote:
> A new mm doesn't have a PASID yet when it's created. Initialize
> the mm's PASID on fork() or for init_mm to INVALID_IOASID (-1).
I must be missing something here.
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index aa5f09ca5bcf..c74d1edbac2f 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -8,6 +8,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * Routines for handling mm_structs
> @@ -433,4 +434,13 @@ static inline void membarrier_update_current_mm(struct
> mm_struct *next_mm)
> }
> #endif
>
> +#ifdef CONFIG_IOMMU_SVA
> +static inline void mm_pasid_init(struct mm_struct *mm)
> +{
> + mm->pasid = INVALID_IOASID;
> +}
> +#else
> +static inline void mm_pasid_init(struct mm_struct *mm) {}
> +#endif
> +
> #endif /* _LINUX_SCHED_MM_H */
So this adds mm_pasid_init() to linux/sched/mm.h which replaces:
> -static void mm_init_pasid(struct mm_struct *mm)
> -{
> -#ifdef CONFIG_IOMMU_SVA
> - mm->pasid = INIT_PASID;
> -#endif
> -}
> -
I.e. already existing code which is initializing mm->pasid with
INIT_PASID (0) while the replacement initializes it with INVALID_IOASID
(-1).
The change log does not have any information about why INIT_PASID is the
wrong initialization value and why this change is not having any side
effects.
It neither mentions why having this in a global available header makes
sense when the only call site is in the C file from which the already
existing function is removed.
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 03/11] iommu/ioasid: Introduce a helper to check for valid PASIDs
On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > pasid_valid() is defined to check if a given PASID is valid. > > Suggested-by: Ashok Raj > Suggested-by: Jacob Pan > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/11] mm: Change CONFIG option for mm->pasid field
On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > This currently depends on CONFIG_IOMMU_SUPPORT. But it is only > needed when CONFIG_IOMMU_SVA option is enabled. > > Change the CONFIG guards around definition and initialization > of mm->pasid field. > > Suggested-by: Jacob Pan > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 01/11] iommu/sva: Rename CONFIG_IOMMU_SVA_LIB to CONFIG_IOMMU_SVA
On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote: > This CONFIG option originally only referred to the Shared > Virtual Address (SVA) library. But it is now also used for > non-library portions of code. > > Drop the "_LIB" suffix so that there is just one configuration > options for all code relating to SVA. > > Signed-off-by: Fenghua Yu > Reviewed-by: Tony Luck Reviewed-by: Thomas Gleixner ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/rockchip: : Use standard driver registration
Am Freitag, 4. Februar 2022, 21:16:41 CET schrieb Robin Murphy:
> It's been a long time since there was any reason to register IOMMU
> drivers early. Convert to the standard platform driver helper.
>
> CC: Heiko Stuebner
> Signed-off-by: Robin Murphy
Reviewed-by: Heiko Stuebner
> ---
> drivers/iommu/rockchip-iommu.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 7f23ad61c094..204a93a72572 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1407,9 +1407,4 @@ static struct platform_driver rk_iommu_driver = {
> .suppress_bind_attrs = true,
> },
> };
> -
> -static int __init rk_iommu_init(void)
> -{
> - return platform_driver_register(_iommu_driver);
> -}
> -subsys_initcall(rk_iommu_init);
> +builtin_platform_driver(rk_iommu_driver);
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [git pull] IOMMU Fixes for Linux v5.17-rc2
The pull request you sent on Fri, 4 Feb 2022 16:49:53 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git > tags/iommu-fixes-v5.17-rc2 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/07cd9ac4c54039c99f98d30e83e23040e330fad5 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/rockchip: : Use standard driver registration
It's been a long time since there was any reason to register IOMMU
drivers early. Convert to the standard platform driver helper.
CC: Heiko Stuebner
Signed-off-by: Robin Murphy
---
drivers/iommu/rockchip-iommu.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 7f23ad61c094..204a93a72572 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1407,9 +1407,4 @@ static struct platform_driver rk_iommu_driver = {
.suppress_bind_attrs = true,
},
};
-
-static int __init rk_iommu_init(void)
-{
- return platform_driver_register(_iommu_driver);
-}
-subsys_initcall(rk_iommu_init);
+builtin_platform_driver(rk_iommu_driver);
--
2.28.0.dirty
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/msm: Use standard driver registration
It's been a long time since there was any reason to register IOMMU
drivers early. Convert to the standard platform driver helper.
CC: Andy Gross
CC: Bjorn Andersson
Signed-off-by: Robin Murphy
---
drivers/iommu/msm_iommu.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 3a38352b603f..06bde6b66732 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -831,16 +831,4 @@ static struct platform_driver msm_iommu_driver = {
.probe = msm_iommu_probe,
.remove = msm_iommu_remove,
};
-
-static int __init msm_iommu_driver_init(void)
-{
- int ret;
-
- ret = platform_driver_register(_iommu_driver);
- if (ret != 0)
- pr_err("Failed to register IOMMU driver\n");
-
- return ret;
-}
-subsys_initcall(msm_iommu_driver_init);
-
+builtin_platform_driver(msm_iommu_driver);
--
2.28.0.dirty
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[git pull] IOMMU Fixes for Linux v5.17-rc2
Hi Linus, The following changes since commit 26291c54e111ff6ba87a164d85d4a4e134b7315c: Linux 5.17-rc2 (2022-01-30 15:37:07 +0200) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git tags/iommu-fixes-v5.17-rc2 for you to fetch changes up to 9b45a7738eec52bf0f5d8d3d54e822962781c5f2: iommu/amd: Fix loop timeout issue in iommu_ga_log_enable() (2022-02-04 12:57:26 +0100) IOMMU Fixes for Linux v5.17-rc2 Including: - Warning fixes and a fix for a potential use-after-free in IOMMU core code - Another potential memory leak fix for the Intel VT-d driver - Fix for an IO polling loop timeout issue in the AMD IOMMU driver Guoqing Jiang (1): iommu/vt-d: Fix potential memory leak in intel_setup_irq_remapping() Joerg Roedel (1): iommu/amd: Fix loop timeout issue in iommu_ga_log_enable() John Garry (1): iommu: Fix some W=1 warnings Vijayanand Jitta (1): iommu: Fix potential use-after-free during probe drivers/iommu/amd/init.c| 2 ++ drivers/iommu/intel/irq_remapping.c | 13 ++--- drivers/iommu/ioasid.c | 1 + drivers/iommu/iommu.c | 33 +++-- drivers/iommu/omap-iommu.c | 2 +- 5 files changed, 33 insertions(+), 18 deletions(-) Please pull. Thanks, Joerg signature.asc Description: Digital signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 4/7] mm: make alloc_contig_range work at pageblock granularity
On 4 Feb 2022, at 8:56, Oscar Salvador wrote:
> On Wed, Jan 19, 2022 at 02:06:20PM -0500, Zi Yan wrote:
>> From: Zi Yan
>>
>> alloc_contig_range() worked at MAX_ORDER-1 granularity to avoid merging
>> pageblocks with different migratetypes. It might unnecessarily convert
>> extra pageblocks at the beginning and at the end of the range. Change
>> alloc_contig_range() to work at pageblock granularity.
>>
>> It is done by restoring pageblock types and split >pageblock_order free
>> pages after isolating at MAX_ORDER-1 granularity and migrating pages
>> away at pageblock granularity. The reason for this process is that
>> during isolation, some pages, either free or in-use, might have >pageblock
>> sizes and isolating part of them can cause free accounting issues.
>> Restoring the migratetypes of the pageblocks not in the interesting
>> range later is much easier.
>
> Hi Zi Yan,
>
> Due to time constraints I only glanced over, so some comments below
> about stuff that caught my eye:
Thanks for looking at the patches!
>
>> +static inline void split_free_page_into_pageblocks(struct page *free_page,
>> +int order, struct zone *zone)
>> +{
>> +unsigned long pfn;
>> +
>> +spin_lock(>lock);
>> +del_page_from_free_list(free_page, zone, order);
>> +for (pfn = page_to_pfn(free_page);
>> + pfn < page_to_pfn(free_page) + (1UL << order);
>
> It migt make sense to have a end_pfn variable so that does not have to
> be constantly evaluated. Or maybe the compiler is clever enough to only
> evualuate it once.
Sure. Will add end_pfn.
>
>> + pfn += pageblock_nr_pages) {
>> +int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);
>> +
>> +__free_one_page(pfn_to_page(pfn), pfn, zone, pageblock_order,
>> +mt, FPI_NONE);
>> +}
>> +spin_unlock(>lock);
>
> It is possible that free_page's order is already pageblock_order, so I
> would add a one-liner upfront to catch that case and return, otherwise
> we do the delete_from_freelist-and-free_it_again dance.
Make sense. Will do.
>
>> +/* Save the migratepages of the pageblocks before start and after end */
>> +num_pageblock_to_save = (alloc_start - isolate_start) /
>> pageblock_nr_pages
>> ++ (isolate_end - alloc_end) /
>> pageblock_nr_pages;
>> +saved_mt =
>> +kmalloc_array(num_pageblock_to_save,
>> + sizeof(unsigned char), GFP_KERNEL);
>> +if (!saved_mt)
>> +return -ENOMEM;
>> +
>> +num = save_migratetypes(saved_mt, isolate_start, alloc_start);
>> +
>> +num = save_migratetypes(_mt[num], alloc_end, isolate_end);
>
> I really hope we can put all this magic within start_isolate_page_range,
> and the counterparts in undo_isolate_page_range.
>
That is my plan too.
> Also, I kinda dislike the _mt thing. I thought about some other
> approaches but nothing that wasn't too specific for this case, and I
> guess we want that function to be as generic as possible.
>
I do not like it either. This whole save and restore thing should go away
once I make MIGRATE_ISOLATE a standalone bit, so that the original
migrateypes will not be overwritten during isolation. Hopefully, I can
work on it soon to get rid of this hunk completely.
>> +/*
>> + * Split free page spanning [alloc_end, isolate_end) and put the
>> + * pageblocks in the right migratetype list
>> + */
>> +for (outer_end = alloc_end; outer_end < isolate_end;) {
>> +unsigned long begin_pfn = outer_end;
>> +
>> +order = 0;
>> +while (!PageBuddy(pfn_to_page(outer_end))) {
>> +if (++order >= MAX_ORDER) {
>> +outer_end = begin_pfn;
>> +break;
>> +}
>> +outer_end &= ~0UL << order;
>> +}
>> +
>> +if (outer_end != begin_pfn) {
>> +order = buddy_order(pfn_to_page(outer_end));
>> +
>> +/*
>> + * split the free page has start page and put the
>> pageblocks
>> + * in the right migratetype list
>> + */
>> +VM_BUG_ON(outer_end + (1UL << order) <= begin_pfn);
>
> How could this possibily happen?
Right. It is not possible. Will remove it.
>
>> +{
>> +struct page *free_page = pfn_to_page(outer_end);
>> +
>> +split_free_page_into_pageblocks(free_page,
>> order, cc.zone);
>> +}
>> +outer_end += 1UL << order;
>> +} else
>> +outer_end = begin_pfn + 1;
>> }
>
> I think there are cases could optimize for. If the page has already been
> split in pageblock by the outer_start loop, we could skip this outer_end
> logic altogether.
>
> E.g: An order-10 page is split in two
Re: [PATCH v4 4/7] mm: make alloc_contig_range work at pageblock granularity
On Wed, Jan 19, 2022 at 02:06:20PM -0500, Zi Yan wrote:
> From: Zi Yan
>
> alloc_contig_range() worked at MAX_ORDER-1 granularity to avoid merging
> pageblocks with different migratetypes. It might unnecessarily convert
> extra pageblocks at the beginning and at the end of the range. Change
> alloc_contig_range() to work at pageblock granularity.
>
> It is done by restoring pageblock types and split >pageblock_order free
> pages after isolating at MAX_ORDER-1 granularity and migrating pages
> away at pageblock granularity. The reason for this process is that
> during isolation, some pages, either free or in-use, might have >pageblock
> sizes and isolating part of them can cause free accounting issues.
> Restoring the migratetypes of the pageblocks not in the interesting
> range later is much easier.
Hi Zi Yan,
Due to time constraints I only glanced over, so some comments below
about stuff that caught my eye:
> +static inline void split_free_page_into_pageblocks(struct page *free_page,
> + int order, struct zone *zone)
> +{
> + unsigned long pfn;
> +
> + spin_lock(>lock);
> + del_page_from_free_list(free_page, zone, order);
> + for (pfn = page_to_pfn(free_page);
> + pfn < page_to_pfn(free_page) + (1UL << order);
It migt make sense to have a end_pfn variable so that does not have to
be constantly evaluated. Or maybe the compiler is clever enough to only
evualuate it once.
> + pfn += pageblock_nr_pages) {
> + int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);
> +
> + __free_one_page(pfn_to_page(pfn), pfn, zone, pageblock_order,
> + mt, FPI_NONE);
> + }
> + spin_unlock(>lock);
It is possible that free_page's order is already pageblock_order, so I
would add a one-liner upfront to catch that case and return, otherwise
we do the delete_from_freelist-and-free_it_again dance.
> + /* Save the migratepages of the pageblocks before start and after end */
> + num_pageblock_to_save = (alloc_start - isolate_start) /
> pageblock_nr_pages
> + + (isolate_end - alloc_end) /
> pageblock_nr_pages;
> + saved_mt =
> + kmalloc_array(num_pageblock_to_save,
> + sizeof(unsigned char), GFP_KERNEL);
> + if (!saved_mt)
> + return -ENOMEM;
> +
> + num = save_migratetypes(saved_mt, isolate_start, alloc_start);
> +
> + num = save_migratetypes(_mt[num], alloc_end, isolate_end);
I really hope we can put all this magic within start_isolate_page_range,
and the counterparts in undo_isolate_page_range.
Also, I kinda dislike the _mt thing. I thought about some other
approaches but nothing that wasn't too specific for this case, and I
guess we want that function to be as generic as possible.
> + /*
> + * Split free page spanning [alloc_end, isolate_end) and put the
> + * pageblocks in the right migratetype list
> + */
> + for (outer_end = alloc_end; outer_end < isolate_end;) {
> + unsigned long begin_pfn = outer_end;
> +
> + order = 0;
> + while (!PageBuddy(pfn_to_page(outer_end))) {
> + if (++order >= MAX_ORDER) {
> + outer_end = begin_pfn;
> + break;
> + }
> + outer_end &= ~0UL << order;
> + }
> +
> + if (outer_end != begin_pfn) {
> + order = buddy_order(pfn_to_page(outer_end));
> +
> + /*
> + * split the free page has start page and put the
> pageblocks
> + * in the right migratetype list
> + */
> + VM_BUG_ON(outer_end + (1UL << order) <= begin_pfn);
How could this possibily happen?
> + {
> + struct page *free_page = pfn_to_page(outer_end);
> +
> + split_free_page_into_pageblocks(free_page,
> order, cc.zone);
> + }
> + outer_end += 1UL << order;
> + } else
> + outer_end = begin_pfn + 1;
> }
I think there are cases could optimize for. If the page has already been
split in pageblock by the outer_start loop, we could skip this outer_end
logic altogether.
E.g: An order-10 page is split in two pageblocks. There's nothing else
to be done, right? We could skip this.
--
Oscar Salvador
SUSE Labs
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/2] dt-bindings: iommu: renesas, ipmmu-vmsa: add r8a779f0 support
Document the compatible values for the IPMMU-VMSA blocks in the Renesas R-Car S4-8 (R8A779F0) SoC and R-Car Gen4. Signed-off-by: Yoshihiro Shimoda 3fbefb9570325500dbf3faff80ded6d0d46f48b2 --- .../devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml index ce0c715205c6..5159a87f3fa7 100644 --- a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml +++ b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml @@ -44,6 +44,10 @@ properties: - renesas,ipmmu-r8a77990 # R-Car E3 - renesas,ipmmu-r8a77995 # R-Car D3 - renesas,ipmmu-r8a779a0 # R-Car V3U + - items: + - enum: + - renesas,ipmmu-r8a779f0 # R-Car S4-8 + - const: renesas,rcar-gen4-ipmmu-vmsa # R-Car Gen4 reg: maxItems: 1 -- 2.25.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/2] iommu/ipmmu-vmsa: Add support for R-Car Gen4
Add support for R-Car Gen4 like r8a779f0 (R-Car S4-8). The IPMMU
hardware design of r8a779f0 is the same as r8a779a0. So, rename
"r8a779a0" to "rcar_gen4".
Signed-off-by: Yoshihiro Shimoda
---
drivers/iommu/ipmmu-vmsa.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index ca752bdc710f..0f7e975546e9 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -719,6 +719,7 @@ static int ipmmu_init_platform_device(struct device *dev,
static const struct soc_device_attribute soc_needs_opt_in[] = {
{ .family = "R-Car Gen3", },
+ { .family = "R-Car Gen4", },
{ .family = "RZ/G2", },
{ /* sentinel */ }
};
@@ -743,7 +744,7 @@ static bool ipmmu_device_is_allowed(struct device *dev)
unsigned int i;
/*
-* R-Car Gen3 and RZ/G2 use the allow list to opt-in devices.
+* R-Car Gen3/4 and RZ/G2 use the allow list to opt-in devices.
* For Other SoCs, this returns true anyway.
*/
if (!soc_device_match(soc_needs_opt_in))
@@ -926,7 +927,7 @@ static const struct ipmmu_features ipmmu_features_rcar_gen3
= {
.utlb_offset_base = 0,
};
-static const struct ipmmu_features ipmmu_features_r8a779a0 = {
+static const struct ipmmu_features ipmmu_features_rcar_gen4 = {
.use_ns_alias_offset = false,
.has_cache_leaf_nodes = true,
.number_of_contexts = 16,
@@ -982,7 +983,10 @@ static const struct of_device_id ipmmu_of_ids[] = {
.data = _features_rcar_gen3,
}, {
.compatible = "renesas,ipmmu-r8a779a0",
- .data = _features_r8a779a0,
+ .data = _features_rcar_gen4,
+ }, {
+ .compatible = "renesas,rcar-gen4-ipmmu",
+ .data = _features_rcar_gen4,
}, {
/* Terminator */
},
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 0/2] iommu/ipmmu-vmsa: Add support for R-Car Gen4
This patch series is based on renesas-drivers-2022-01-11-v5.16 [1]. Note that we have to prepare the following registers' setting in a bootloader (U-Boot) because the registers are protected. Otherwise, data mismatch happened if dmatest with the ipmmu is running. => mw eed01500 0xc000; mw eed41500 0xc000 [1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tag/?h=renesas-drivers-2022-01-11-v5.16 Changes from v1: - Add Reviewed-by tag in patch 1. (Geert-san, thanks!) - Revise a comment in patch 2. https://lore.kernel.org/all/20220125125602.4144793-1-yoshihiro.shimoda...@renesas.com/ Yoshihiro Shimoda (2): dt-bindings: iommu: renesas,ipmmu-vmsa: add r8a779f0 support iommu/ipmmu-vmsa: Add support for R-Car Gen4 .../devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml | 4 drivers/iommu/ipmmu-vmsa.c | 10 +++--- 2 files changed, 11 insertions(+), 3 deletions(-) -- 2.25.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: iommu non-strict mode for arm64
On 2022-02-04 05:46, Josh Poimboeuf wrote: Hi all, We've gotten significant slowdowns on arm64 with 4k pages compared to 64k. The slowdowns can be alleviated by setting iommu.strict=0 or iommu.passthrough=1. Is there a reason x86 defaults to lazy iommu, while arm64 does not? Are there security implications which are specific to arm64? The x86 behaviour is basically 2 decades of legacy where nobody now feels brave enough to flip the default. At the time the arm64 IOMMU DMA ops were first added, strict mode was the only thing feasible to implement, but there was also a conscious consideration that having a default assumption of "IOMMU == more protection" wasn't a bad thing anyway. Given what played out a couple of years later, and everyone now being that much more security-aware, I think that decision has only been reinforced. Passthrough and non-strict mode in iommu-dma only came along later, and most IOMMU drivers for arm64 still don't support them, which is another reason I'm still against changing the default today. However, if you're confident that your arm64 users care more about high-bandwidth I/O throughput than memory protection then feel free to set IOMMU_DEFAULT_DMA_LAZY or IOMMU_DEFAULT_PASSTHROUGH in your config. Cheers, Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/amd: Fix loop timeout issue in iommu_ga_log_enable()
From: Joerg Roedel
The polling loop for the register change in iommu_ga_log_enable() needs
to have a udelay() in it. Otherwise the CPU might be faster than the
IOMMU hardware and wrongly trigger the WARN_ON() further down the code
stream. Use a 10us for udelay(), has there is some hardware where
activation of the GA log can take more than a 100ms.
A future optimization should move the activation check of the GA log
to the point where it gets used for the first time. But that is a
bigger change and not suitable for a fix.
Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
Signed-off-by: Joerg Roedel
---
drivers/iommu/amd/init.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index dc338acf3338..b10fb52ea442 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -834,6 +835,7 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
if (status & (MMIO_STATUS_GALOG_RUN_MASK))
break;
+ udelay(10);
}
if (WARN_ON(i >= LOOP_TIMEOUT))
--
2.34.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix loop timeout issue in iommu_ga_log_enable()
On Mon, Jan 31, 2022 at 05:06:03PM +, John Garry wrote: > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > > index dc338acf3338..d2e09d53851f 100644 > > --- a/drivers/iommu/amd/init.c > > +++ b/drivers/iommu/amd/init.c > > @@ -834,6 +834,7 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu) > > status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET); > > if (status & (MMIO_STATUS_GALOG_RUN_MASK)) > > break; > > + udelay(1); > > Maybe readl_relaxed_poll_timeout_atomic() could be used instead I sent another version of this patch which uses readl_poll_timeout_atomic(), but it didn't fix the issue. I take this approach for now and leave using the helper as a future improvement. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
