[PATCH 02/10] iommu/of: Prepare for deferred IOMMU configuration

[Robin Murphy]

On 05/01/17 12:27, Lorenzo Pieralisi wrote:
> On Thu, Jan 05, 2017 at 02:04:37PM +0530, Sricharan wrote:
>> Hi Robin/Lorenzo,
>>
>>> Hi Robin,Lorenzo,
>>>
>>>> On Wed, Nov 30, 2016 at 04:42:27PM +0000, Robin Murphy wrote:
>>>>> On 30/11/16 16:17, Lorenzo Pieralisi wrote:
>>>>>> Sricharan, Robin,
>>>>>>
>>>>>> I gave this series a go on ACPI and apart from an SMMU v3 fix-up
>>>>>> it seems to work, more thorough testing required though.
>>>>>>
>>>>>> A key question below.
>>>>>>
>>>>>> On Wed, Nov 30, 2016 at 05:52:16AM +0530, Sricharan R wrote:
>>>>>>> From: Robin Murphy <robin.murphy at arm.com>
>>>>>>>
>>>>>>> IOMMU configuration represents unchanging properties of the hardware,
>>>>>>> and as such should only need happen once in a device's lifetime, but
>>>>>>> the necessary interaction with the IOMMU device and driver complicates
>>>>>>> exactly when that point should be.
>>>>>>>
>>>>>>> Since the only reasonable tool available for handling the inter-device
>>>>>>> dependency is probe deferral, we need to prepare of_iommu_configure()
>>>>>>> to run later than it is currently called (i.e. at driver probe rather
>>>>>>> than device creation), to handle being retried, and to tell whether a
>>>>>>> not-yet present IOMMU should be waited for or skipped (by virtue of
>>>>>>> having declared a built-in driver or not).
>>>>>>>
>>>>>>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>>>>>>> ---
>>>>>>>  drivers/iommu/of_iommu.c | 30 +++++++++++++++++++++++++++++-
>>>>>>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>>>>>>> index ee49081..349bd1d 100644
>>>>>>> --- a/drivers/iommu/of_iommu.c
>>>>>>> +++ b/drivers/iommu/of_iommu.c
>>>>>>> @@ -104,12 +104,20 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
>>>>>>>  	int err;
>>>>>>>
>>>>>>>  	ops = iommu_get_instance(fwnode);
>>>>>>> -	if (!ops || !ops->of_xlate)
>>>>>>> +	if ((ops && !ops->of_xlate) ||
>>>>>>> +	    (!ops && !of_match_node(&__iommu_of_table, iommu_spec->np)))
>>>>>>
>>>>>> IIUC of_match_node() here is there to check there is a driver compiled
>>>>>> in for this device_node (aka compatible string in OF world), correct ?
>>>>>
>>>>> Yes - specifically, it's checking the magic table for a matching
>>>>> IOMMU_OF_DECLARE entry.
>>>>>
>>>>>> If that's the case (and I think that's what Sricharan was referring to
>>>>>> in his ACPI query) I need to cook-up something on the ACPI side to
>>>>>> emulate the OF linker table behaviour (or anyway to detect a driver is
>>>>>> actually in the kernel), it is not that difficult but it is key to know,
>>>>>> I will give it some thought to make it as clean as possible.
>>>>>
>>>>> I didn't think this would be a concern for ACPI, since IORT works much
>>>>> the same way the current of_iommu_init_fn/of_platform_device_create()
>>>>> bodges in drivers so for DT. If you can only discover SMMUs from IORT,
>>>>> then iort_init_platform_devices() will have already created every SMMU
>>>>> that's going to exist before discovering other devices from wherever
>>>>> they come from, thus you could never get into the situation of probing a
>>>>> device without its SMMU being ready (if it's ever going to be). Is that
>>>>> not right?
>>>>
>>>> It is right, my point and question is: we are probing a device and we
>>>> have to know whether it is worth deferring its IOMMU DMA setup. On DT,
>>>> through of_match_node(&__iommu_of_table, iommu_device_node) we check at
>>>> once that:
>>>>
>>>> 1 - A device for the IOMMU exists
>>>>
>>>> AND
>>>>
>>>> 2 - A driver for the IOMMU is compiled in the kernel
>>>>
>>>> Is this correct ? As you said (1) is not a concern on ACPI IORT (because
>>>> we create the IOMMU device before _any_ other device so either the IOMMU
>>>> device is there or it will never be by the time master devices are
>>>> probed), but for (2) I need to slightly change how the IORT linker entry
>>>> work to make sure we can detect a driver is actually compiled in the
>>>> kernel, it is easy, I was just asking if my understanding was correct
>>>> and I think that was what Sricharan was referring to in his query.
>>>>
>>>
>>> Yes right, this was what i was looking for in the ACPI case and putting this
>>> in the iort_iommu_xlate was needed to return EPROBE_DEFER when the
>>> driver is not yet been probed.
>>
>> With the thinking of taking this series through, would it be fine if i
>> cleanup the pci configure hanging outside and push it in to
>> of/acpi_iommu configure respectively ? This time with all neeeded for
>> ACPI added as well.  Also on the last post of V4, Lorenzo commented
>> that it worked for him, although still the of_match_node equivalent in
>> ACPI has to be added. If i can get that, then i will add that as well
>> to make this complete.
> 
> Question: I had a look into this and instead of fiddling about with the
> linker script entries in ACPI (ie IORT_ACPI_DECLARE - which I hope this
> patchset would help remove entirely), I think that the only check we
> need in IORT is, depending on what type of SMMU a given device is
> connected to, to check if the respective SMMU driver is compiled in the
> kernel and it will be probed, _eventually_.
> 
> As Robin said, by the time a device is probed the respective SMMU
> devices are already created and registered with IORT kernel code or
> they will never be, so to understand if we should defer probing
> SMMU device creation is _not_ really a problem in ACPI.
> 
> To check if a SMMU driver is enabled, do we really need a linker
> table ?
> 
> Would not a check based on eg:
> 
> /**
>  * @type: IOMMU IORT node type of the IOMMU a device is connected to
>  */
> static bool iort_iommu_driver_enabled(u8 type)
> {
> 	switch (type) {
> 	case ACPI_IORT_SMMU_V3:
> 		return IS_ENABLED(CONFIG_ARM_SMMU_V3);

IS_BUILTIN(...)

> 	case ACPI_IORT_SMMU:
> 		return IS_ENABLED(CONFIG_ARM_SMMU);
> 	default:
> 		pr_warn("Unknown IORT SMMU type\n");

Might displaying the actual value be helfpul for debugging a broken IORT
table?

> 		return false;
> 	}
> }
> 
> be sufficient (it is a bit gross, agreed, but it is to understand if
> that's all we need) ? Is there anything I am missing ?
> 
> Let me know, I will put together a patch for you I really do not
> want to block your series for this trivial niggle.

Other than that, though, I like it :) IORT has a small, strictly
bounded, set of supported devices, so I really don't see the need to go
overboard putting it on parity with DT when something this neat and
simple will suffice.

Robin.

> 
> Thanks,
> Lorenzo
>