Re: [PATCH] iommu: silence iommu group prints

[Laurentiu Tudor <laurentiu.tudor@nxp.com>]

On 04.03.2020 12:07, Russell King - ARM Linux admin wrote:
> On Wed, Mar 04, 2020 at 11:56:53AM +0200, Laurentiu Tudor wrote:
>>  From 44ae46501b5379bd0890df87fd3827248626ed03 Mon Sep 17 00:00:00 2001
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> Date: Tue, 1 Oct 2019 16:22:48 +0300
>> Subject: [PATCH 1/6] bus: fsl-mc: make mc work with smmu disable bypass on
>> Content-Type: text/plain; charset="us-ascii"
>>
>> Since this commit [1] booting kernel on MC based chips started to
>> fail because this firmware starts before the kernel and as soon as
>> SMMU is probed it starts to trigger contiguous faults.
> 
> I think you mean "continuous" here.

Yes, thanks.

>> This is a workaround that allows MC firmware to run with SMMU
>> in disable bypass mode. It consists of the following steps:
>>   1. pause the firmware at early boot to get a chance to setup SMMU
>>   2. request direct mapping for MC device
>>   3. resume the firmware
>> The workaround relies on the fact that no state is lost when
>> pausing / resuming firmware, as per the docs.
>> With this patch, platforms with MC firmware can now boot without
>> having to change the default config to set:
>>    CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n
> 
> This alone is a definite improvement, and has been needed for a while.
> Please submit this patch with an appropriate Fixes: tag so stable trees
> can pick it up.

The thing is that probably this workaround will never make it in the 
kernel because it questionable to say the least, e.g. see [1]. My plan 
is to give this approach [2] a try sometime soon.

[1] https://patchwork.kernel.org/comment/23149049/
[2] https://patchwork.kernel.org/cover/11279577/

---
Best Regards, Laurentiu

>> [1] 954a03be033 ("iommu/arm-smmu: Break insecure users by disabling bypass by default")
> 
> Please put this where you're referencing it above - it's fine to wrap
> the description of the commit when using it in the body of the commit
> message.  However, that should _never_ when providing a Fixes: tag
> (linux-next has a script which will detect and complain about broken
> Fixes: tags.)
> 
> Thanks.
> 
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> ---
>>   drivers/bus/fsl-mc/fsl-mc-bus.c | 53 +++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
>> index a0f8854acb3a..683a6401ffe8 100644
>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
>> @@ -18,6 +18,8 @@
>>   #include <linux/bitops.h>
>>   #include <linux/msi.h>
>>   #include <linux/dma-mapping.h>
>> +#include <linux/iommu.h>
>> +#include <linux/io.h>
>>   
>>   #include "fsl-mc-private.h"
>>   
>> @@ -889,6 +891,12 @@ static int get_mc_addr_translation_ranges(struct device *dev,
>>   	return 0;
>>   }
>>   
>> +#define FSL_MC_GCR1	0x0
>> +#define GCR1_P1_STOP	BIT(31)
>> +
>> +static u32 boot_gcr1;
>> +static void __iomem *fsl_mc_regs;
>> +
>>   /**
>>    * fsl_mc_bus_probe - callback invoked when the root MC bus is being
>>    * added
>> @@ -906,6 +914,21 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
>>   	struct mc_version mc_version;
>>   	struct resource res;
>>   
>> +	/*
>> +	 * The MC firmware requires full access to the whole address space plus
>> +	 * it has no way of dealing with any kind of address translation, so
>> +	 * request direct mapping for it.
>> +	 */
>> +	error = iommu_request_dm_for_dev(&pdev->dev);
>> +	if (error)
>> +		dev_warn(&pdev->dev, "iommu_request_dm_for_dev() failed: %d\n",
>> +			 error);
>> +
>> +	if (fsl_mc_regs) {
>> +		/* Resume the firmware */
>> +		writel(boot_gcr1 & ~GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
>> +	}
>> +
>>   	mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL);
>>   	if (!mc)
>>   		return -ENOMEM;
>> @@ -990,6 +1013,13 @@ static int fsl_mc_bus_remove(struct platform_device *pdev)
>>   	if (!fsl_mc_is_root_dprc(&mc->root_mc_bus_dev->dev))
>>   		return -EINVAL;
>>   
>> +	/*
>> +	 * Pause back the firmware so that it doesn't trigger faults by the
>> +	 * time SMMU gets brought down.
>> +	 */
>> +	writel(boot_gcr1 | GCR1_P1_STOP, fsl_mc_regs + FSL_MC_GCR1);
>> +	iounmap(fsl_mc_regs);
>> +
>>   	fsl_mc_device_remove(mc->root_mc_bus_dev);
>>   
>>   	fsl_destroy_mc_io(mc->root_mc_bus_dev->mc_io);
>> @@ -1018,6 +1048,8 @@ static struct platform_driver fsl_mc_bus_driver = {
>>   static int __init fsl_mc_bus_driver_init(void)
>>   {
>>   	int error;
>> +	struct device_node *np;
>> +	struct resource res;
>>   
>>   	error = bus_register(&fsl_mc_bus_type);
>>   	if (error < 0) {
>> @@ -1039,9 +1071,30 @@ static int __init fsl_mc_bus_driver_init(void)
>>   	if (error < 0)
>>   		goto error_cleanup_dprc_driver;
>>   
>> +	np = of_find_matching_node(NULL, fsl_mc_bus_match_table);
>> +	if (np && of_device_is_available(np)) {
>> +		error = of_address_to_resource(np, 1, &res);
>> +		if (error)
>> +			goto error_cleanup_dprc_driver;
>> +		fsl_mc_regs = ioremap(res.start, resource_size(&res));
>> +		if (!fsl_mc_regs) {
>> +			error = -ENXIO;
>> +			goto error_cleanup_dprc_driver;
>> +		}
>> +
>> +		boot_gcr1 = readl(fsl_mc_regs + FSL_MC_GCR1);
>> +		/*
>> +		 * If found running, pause MC firmware in order to get a chance
>> +		 * to setup SMMU for it.
>> +		 */
>> +		if (!(boot_gcr1 & GCR1_P1_STOP))
>> +			writel(boot_gcr1 | GCR1_P1_STOP,  fsl_mc_regs + FSL_MC_GCR1);
>> +	}
>> +
>>   	return 0;
>>   
>>   error_cleanup_dprc_driver:
>> +	iounmap(fsl_mc_regs);
>>   	dprc_driver_exit();
>>   
>>   error_cleanup_driver:
>> -- 
>> 2.17.1
>>
> 
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu