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

[Lorenzo Pieralisi]

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);
	case ACPI_IORT_SMMU:
		return IS_ENABLED(CONFIG_ARM_SMMU);
	default:
		pr_warn("Unknown IORT SMMU type\n");
		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.

Thanks,
Lorenzo