[PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Marek Szyprowski]

Add set_platform_dma_ops() required for proper driver operation on ARM
32bit arch after recent changes in the IOMMU framework (detach ops
removal).

Fixes: c1fe9119ee70 ("iommu: Add set_platform_dma_ops callbacks")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 6fc58e89712f..c4955d045855 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1346,8 +1346,10 @@ static void exynos_iommu_release_device(struct device *dev)
 		struct iommu_group *group = iommu_group_get(dev);
 
 		if (group) {
+#ifndef CONFIG_ARM
 			WARN_ON(owner->domain !=
 				iommu_group_default_domain(group));
+#endif
 			exynos_iommu_detach_device(owner->domain, dev);
 			iommu_group_put(group);
 		}
@@ -1398,6 +1400,9 @@ static int exynos_iommu_of_xlate(struct device *dev,
 static const struct iommu_ops exynos_iommu_ops = {
 	.domain_alloc = exynos_iommu_domain_alloc,
 	.device_group = generic_device_group,
+#ifdef CONFIG_ARM
+	.set_platform_dma_ops = exynos_iommu_release_device,
+#endif
 	.probe_device = exynos_iommu_probe_device,
 	.release_device = exynos_iommu_release_device,
 	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
-- 
2.39.0

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Jason Gunthorpe]

On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
> Add set_platform_dma_ops() required for proper driver operation on ARM
> 32bit arch after recent changes in the IOMMU framework (detach ops
> removal).

Thanks for looking into this!

Can you explain more about how this actually solves the problem in the
commit message? I don't get it.

> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 6fc58e89712f..c4955d045855 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -1346,8 +1346,10 @@ static void exynos_iommu_release_device(struct device *dev)
>  		struct iommu_group *group = iommu_group_get(dev);
>  
>  		if (group) {
> +#ifndef CONFIG_ARM
>  			WARN_ON(owner->domain !=
>  				iommu_group_default_domain(group));
> +#endif
>  			exynos_iommu_detach_device(owner->domain, dev);
>  			iommu_group_put(group);
>  		}
> @@ -1398,6 +1400,9 @@ static int exynos_iommu_of_xlate(struct device *dev,
>  static const struct iommu_ops exynos_iommu_ops = {
>  	.domain_alloc = exynos_iommu_domain_alloc,
>  	.device_group = generic_device_group,
> +#ifdef CONFIG_ARM
> +	.set_platform_dma_ops = exynos_iommu_release_device,
> +#endif

This is ugly, if you need a set_platform_dma_ops it should not be
called release... Release is supposed to be about putting the HW back
to some idle state because we are unplugging the struct device.

Jason

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Joerg Roedel]

On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
> Add set_platform_dma_ops() required for proper driver operation on ARM
> 32bit arch after recent changes in the IOMMU framework (detach ops
> removal).
> 
> Fixes: c1fe9119ee70 ("iommu: Add set_platform_dma_ops callbacks")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Applied, thanks.

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Marek Szyprowski]

Hi,

I'm sorry for a delay in replying, but I was busy with other stuff.

On 23.01.2023 22:00, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
>> Add set_platform_dma_ops() required for proper driver operation on ARM
>> 32bit arch after recent changes in the IOMMU framework (detach ops
>> removal).
> Thanks for looking into this!
>
> Can you explain more about how this actually solves the problem in the
> commit message? I don't get it.

Exynos DRM driver calls arm_iommu_detach_device(), then 
arm_iommu_attach_device() with a difrent 'mapping', see 
drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops 
leads to a warning in iommu_group_do_set_platform_dma(). The other case 
of calling arm_iommu_detach_device() is after unsuccessful probe of the 
platform device.


>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index 6fc58e89712f..c4955d045855 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -1346,8 +1346,10 @@ static void exynos_iommu_release_device(struct device *dev)
>>   		struct iommu_group *group = iommu_group_get(dev);
>>   
>>   		if (group) {
>> +#ifndef CONFIG_ARM
>>   			WARN_ON(owner->domain !=
>>   				iommu_group_default_domain(group));
>> +#endif
>>   			exynos_iommu_detach_device(owner->domain, dev);
>>   			iommu_group_put(group);
>>   		}
>> @@ -1398,6 +1400,9 @@ static int exynos_iommu_of_xlate(struct device *dev,
>>   static const struct iommu_ops exynos_iommu_ops = {
>>   	.domain_alloc = exynos_iommu_domain_alloc,
>>   	.device_group = generic_device_group,
>> +#ifdef CONFIG_ARM
>> +	.set_platform_dma_ops = exynos_iommu_release_device,
>> +#endif
> This is ugly, if you need a set_platform_dma_ops it should not be
> called release... Release is supposed to be about putting the HW back
> to some idle state because we are unplugging the struct device.

Indeed this patch was a bit ugly, I've did it in a bit of hurry. That 
time I've simply checked how it is implemented in other drivers and 
found that it very similar to the release operation, so I did it in 
exynos-iommu that way. I've checked again and indeed there are some 
differences, so I will send a fix in a few minutes.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Baolu Lu]

On 2023/2/17 19:08, Marek Szyprowski wrote:
> Hi,
> 
> I'm sorry for a delay in replying, but I was busy with other stuff.
> 
> On 23.01.2023 22:00, Jason Gunthorpe wrote:
>> On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
>>> Add set_platform_dma_ops() required for proper driver operation on ARM
>>> 32bit arch after recent changes in the IOMMU framework (detach ops
>>> removal).
>> Thanks for looking into this!
>>
>> Can you explain more about how this actually solves the problem in the
>> commit message? I don't get it.
> Exynos DRM driver calls arm_iommu_detach_device(), then
> arm_iommu_attach_device() with a difrent 'mapping', see
> drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops
> leads to a warning in iommu_group_do_set_platform_dma(). The other case
> of calling arm_iommu_detach_device() is after unsuccessful probe of the
> platform device.

Do we really need to call iommu_detach_device() in
arm_iommu_detach_device() for 32-bit ARM? The dma-iommu is disabled for
32-bit ARM, hence calling .set_platform_dma_ops makes no sense.

In below patch,

https://lore.kernel.org/linux-iommu/20230217094736.159005-2-baolu.lu@linux.intel.com/

I removed iommu_detach_device() from arm_iommu_detach_device().

Best regards,
baolu

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Jason Gunthorpe]

On Fri, Feb 17, 2023 at 12:08:42PM +0100, Marek Szyprowski wrote:
> Hi,
> 
> I'm sorry for a delay in replying, but I was busy with other stuff.
> 
> On 23.01.2023 22:00, Jason Gunthorpe wrote:
> > On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
> >> Add set_platform_dma_ops() required for proper driver operation on ARM
> >> 32bit arch after recent changes in the IOMMU framework (detach ops
> >> removal).
> > Thanks for looking into this!
> >
> > Can you explain more about how this actually solves the problem in the
> > commit message? I don't get it.
> 
> Exynos DRM driver calls arm_iommu_detach_device(), then 
> arm_iommu_attach_device() with a difrent 'mapping', see 
> drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops 
> leads to a warning in iommu_group_do_set_platform_dma(). The other case 
> of calling arm_iommu_detach_device() is after unsuccessful probe of the 
> platform device.

Why can't this just use the normal iommu path in all cases?

It looks like it is trying to copy the DMA API domain from a parent
device to a sub device.

Even when using arm_iommu an iommu_domain is still present, so the
copy code should work?

Though I'm still not really sure how this arm_iommu stuff works..

eg if a driver does iommu_device_claim_dma_owner() how does the
iommu_domain get set back to the arm_iommu's mapping's iommu_domain?

According to the API that is what set_platform is supposed to do (eg
it is what s390 does), but I don't see any code like that in any of
the ARM32 drivers..

Jason

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Robin Murphy]

On 2023-02-17 12:35, Jason Gunthorpe wrote:
> On Fri, Feb 17, 2023 at 12:08:42PM +0100, Marek Szyprowski wrote:
>> Hi,
>>
>> I'm sorry for a delay in replying, but I was busy with other stuff.
>>
>> On 23.01.2023 22:00, Jason Gunthorpe wrote:
>>> On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
>>>> Add set_platform_dma_ops() required for proper driver operation on ARM
>>>> 32bit arch after recent changes in the IOMMU framework (detach ops
>>>> removal).
>>> Thanks for looking into this!
>>>
>>> Can you explain more about how this actually solves the problem in the
>>> commit message? I don't get it.
>>
>> Exynos DRM driver calls arm_iommu_detach_device(), then
>> arm_iommu_attach_device() with a difrent 'mapping', see
>> drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops
>> leads to a warning in iommu_group_do_set_platform_dma(). The other case
>> of calling arm_iommu_detach_device() is after unsuccessful probe of the
>> platform device.
> 
> Why can't this just use the normal iommu path in all cases?
> 
> It looks like it is trying to copy the DMA API domain from a parent
> device to a sub device.
> 
> Even when using arm_iommu an iommu_domain is still present, so the
> copy code should work?

The ARM DMA domain is a regular unmanaged domain owned by the ARM DMA 
code - in order to use any *other* domain, a user has to detach from 
that first (wrapped up in arm_iommu_detach_device() which also swizzles 
the DMA ops at the same time). Without set_platform_dma, that detach is 
now impossible (because no IOMMU API default domain exists either).

> Though I'm still not really sure how this arm_iommu stuff works..
> 
> eg if a driver does iommu_device_claim_dma_owner() how does the
> iommu_domain get set back to the arm_iommu's mapping's iommu_domain?

It doesn't. Fact is that VFIO has only ever worked on Arm with arm-smmu 
and its old deprecated DT bindings which don't interact with any of the 
DMA ops stuff. And ownership has always been inherently enforced here 
since the DMA ops are just another external user from the IOMMU API's PoV.

> According to the API that is what set_platform is supposed to do (eg
> it is what s390 does), but I don't see any code like that in any of
> the ARM32 drivers..

As above, it's in the relevant users, not the drivers. For what you're 
thinking of, the "platform" state is only ever transient.

(for the purposes of this discussion, ignore the MTK and Renesas drivers 
bodging around to implement pseudo-default-domain support on *top* of 
the user-centric arm_iommu_* APIs; that's a whole other mess to unravel)

Thanks,
Robin.

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Jason Gunthorpe]

On Mon, Feb 20, 2023 at 01:58:40PM +0000, Robin Murphy wrote:
> On 2023-02-17 12:35, Jason Gunthorpe wrote:
> > On Fri, Feb 17, 2023 at 12:08:42PM +0100, Marek Szyprowski wrote:
> > > Hi,
> > > 
> > > I'm sorry for a delay in replying, but I was busy with other stuff.
> > > 
> > > On 23.01.2023 22:00, Jason Gunthorpe wrote:
> > > > On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
> > > > > Add set_platform_dma_ops() required for proper driver operation on ARM
> > > > > 32bit arch after recent changes in the IOMMU framework (detach ops
> > > > > removal).
> > > > Thanks for looking into this!
> > > > 
> > > > Can you explain more about how this actually solves the problem in the
> > > > commit message? I don't get it.
> > > 
> > > Exynos DRM driver calls arm_iommu_detach_device(), then
> > > arm_iommu_attach_device() with a difrent 'mapping', see
> > > drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops
> > > leads to a warning in iommu_group_do_set_platform_dma(). The other case
> > > of calling arm_iommu_detach_device() is after unsuccessful probe of the
> > > platform device.
> > 
> > Why can't this just use the normal iommu path in all cases?
> > 
> > It looks like it is trying to copy the DMA API domain from a parent
> > device to a sub device.
> > 
> > Even when using arm_iommu an iommu_domain is still present, so the
> > copy code should work?
> 
> The ARM DMA domain is a regular unmanaged domain owned by the ARM DMA code -
> in order to use any *other* domain, a user has to detach from that first
> (wrapped up in arm_iommu_detach_device() which also swizzles the DMA ops at
> the same time). Without set_platform_dma, that detach is now impossible
> (because no IOMMU API default domain exists either).

I was looking at this again, and for completeness, the reason it
doesn't have a default_domain is a bit subtle.

The driver does support IOMMU_DOMAIN_DMA, and it will go through all
the iommu_group_alloc_default_domain() stuff with that..

But... __iommu_domain_alloc() calls iommu_get_dma_cookie() which will
be wired to fail on ARM32, so the core code nixes the IOMMU_DOMAIN_DMA
after the driver successfully created it.

I wonder if that is actually what we want? As it seems like it would
be OK for ARM32 to have, effectively, a permanently empty unmanaged
domain as the default_domain?

If instead of failing due to iommu_get_dma_cookie() we set the type to
UNMANAGED and make that the default domain it could fix all of this,
even for all the other ARM32 drivers that haven't reported this yet?

Alternatively since we taught these drivers to support IDENTITY, we
should be able to remove the set_platform_dma_ops and instead
implement for ARM32 def_domain_type fixed to return IDENTITY?

Jason

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Marek Szyprowski]

On 07.04.2023 01:37, Jason Gunthorpe wrote:
> On Mon, Feb 20, 2023 at 01:58:40PM +0000, Robin Murphy wrote:
>> On 2023-02-17 12:35, Jason Gunthorpe wrote:
>>> On Fri, Feb 17, 2023 at 12:08:42PM +0100, Marek Szyprowski wrote:
>>>> I'm sorry for a delay in replying, but I was busy with other stuff.
>>>>
>>>> On 23.01.2023 22:00, Jason Gunthorpe wrote:
>>>>> On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
>>>>>> Add set_platform_dma_ops() required for proper driver operation on ARM
>>>>>> 32bit arch after recent changes in the IOMMU framework (detach ops
>>>>>> removal).
>>>>> Thanks for looking into this!
>>>>>
>>>>> Can you explain more about how this actually solves the problem in the
>>>>> commit message? I don't get it.
>>>> Exynos DRM driver calls arm_iommu_detach_device(), then
>>>> arm_iommu_attach_device() with a difrent 'mapping', see
>>>> drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops
>>>> leads to a warning in iommu_group_do_set_platform_dma(). The other case
>>>> of calling arm_iommu_detach_device() is after unsuccessful probe of the
>>>> platform device.
>>> Why can't this just use the normal iommu path in all cases?
>>>
>>> It looks like it is trying to copy the DMA API domain from a parent
>>> device to a sub device.
>>>
>>> Even when using arm_iommu an iommu_domain is still present, so the
>>> copy code should work?
>> The ARM DMA domain is a regular unmanaged domain owned by the ARM DMA code -
>> in order to use any *other* domain, a user has to detach from that first
>> (wrapped up in arm_iommu_detach_device() which also swizzles the DMA ops at
>> the same time). Without set_platform_dma, that detach is now impossible
>> (because no IOMMU API default domain exists either).
> I was looking at this again, and for completeness, the reason it
> doesn't have a default_domain is a bit subtle.
>
> The driver does support IOMMU_DOMAIN_DMA, and it will go through all
> the iommu_group_alloc_default_domain() stuff with that..
>
> But... __iommu_domain_alloc() calls iommu_get_dma_cookie() which will
> be wired to fail on ARM32, so the core code nixes the IOMMU_DOMAIN_DMA
> after the driver successfully created it.
>
> I wonder if that is actually what we want? As it seems like it would
> be OK for ARM32 to have, effectively, a permanently empty unmanaged
> domain as the default_domain?
>
> If instead of failing due to iommu_get_dma_cookie() we set the type to
> UNMANAGED and make that the default domain it could fix all of this,
> even for all the other ARM32 drivers that haven't reported this yet?
>
> Alternatively since we taught these drivers to support IDENTITY, we
> should be able to remove the set_platform_dma_ops and instead
> implement for ARM32 def_domain_type fixed to return IDENTITY?

Maybe it would be easier to simply switch ARM32 to generic IOMMU-DMA layer?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Jason Gunthorpe]

On Thu, Apr 13, 2023 at 12:03:55AM +0200, Marek Szyprowski wrote:
> On 07.04.2023 01:37, Jason Gunthorpe wrote:
> > On Mon, Feb 20, 2023 at 01:58:40PM +0000, Robin Murphy wrote:
> >> On 2023-02-17 12:35, Jason Gunthorpe wrote:
> >>> On Fri, Feb 17, 2023 at 12:08:42PM +0100, Marek Szyprowski wrote:
> >>>> I'm sorry for a delay in replying, but I was busy with other stuff.
> >>>>
> >>>> On 23.01.2023 22:00, Jason Gunthorpe wrote:
> >>>>> On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote:
> >>>>>> Add set_platform_dma_ops() required for proper driver operation on ARM
> >>>>>> 32bit arch after recent changes in the IOMMU framework (detach ops
> >>>>>> removal).
> >>>>> Thanks for looking into this!
> >>>>>
> >>>>> Can you explain more about how this actually solves the problem in the
> >>>>> commit message? I don't get it.
> >>>> Exynos DRM driver calls arm_iommu_detach_device(), then
> >>>> arm_iommu_attach_device() with a difrent 'mapping', see
> >>>> drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops
> >>>> leads to a warning in iommu_group_do_set_platform_dma(). The other case
> >>>> of calling arm_iommu_detach_device() is after unsuccessful probe of the
> >>>> platform device.
> >>> Why can't this just use the normal iommu path in all cases?
> >>>
> >>> It looks like it is trying to copy the DMA API domain from a parent
> >>> device to a sub device.
> >>>
> >>> Even when using arm_iommu an iommu_domain is still present, so the
> >>> copy code should work?
> >> The ARM DMA domain is a regular unmanaged domain owned by the ARM DMA code -
> >> in order to use any *other* domain, a user has to detach from that first
> >> (wrapped up in arm_iommu_detach_device() which also swizzles the DMA ops at
> >> the same time). Without set_platform_dma, that detach is now impossible
> >> (because no IOMMU API default domain exists either).
> > I was looking at this again, and for completeness, the reason it
> > doesn't have a default_domain is a bit subtle.
> >
> > The driver does support IOMMU_DOMAIN_DMA, and it will go through all
> > the iommu_group_alloc_default_domain() stuff with that..
> >
> > But... __iommu_domain_alloc() calls iommu_get_dma_cookie() which will
> > be wired to fail on ARM32, so the core code nixes the IOMMU_DOMAIN_DMA
> > after the driver successfully created it.
> >
> > I wonder if that is actually what we want? As it seems like it would
> > be OK for ARM32 to have, effectively, a permanently empty unmanaged
> > domain as the default_domain?
> >
> > If instead of failing due to iommu_get_dma_cookie() we set the type to
> > UNMANAGED and make that the default domain it could fix all of this,
> > even for all the other ARM32 drivers that haven't reported this yet?
> >
> > Alternatively since we taught these drivers to support IDENTITY, we
> > should be able to remove the set_platform_dma_ops and instead
> > implement for ARM32 def_domain_type fixed to return IDENTITY?
> 
> Maybe it would be easier to simply switch ARM32 to generic IOMMU-DMA layer?

It is the long term goal, yes

In the short term it would be nice if we could at least keep the hacks
out of the drivers

Jason