[RFC PATCH 0/4] SMMUv3 MSI support

[RFC PATCH 0/4] SMMUv3 MSI support

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

IORT revision C introduced SMMUv3 MSI support for control interrupts,
which introduced a device ID mapping index to retrieve the dev ID
and ITS parent.

This patch set reuse the existing APIs to retrieve the data to
configure the MSI, but doing special checks to not introduce regressions
for CPI/NC--->SMMU--->ITS cases, please refer to each patch for detail
commit message.

Hanjun Guo (4):
  ACPICA: Add SMMUv3 device ID mapping index support
  ACPI: IORT: lookup iort node via fwnode
  ACPI: IORT: Skip SMMUv3 device ID map for two steps mappings
  ACPI: IORT: Add SMMUv3 MSI support

 drivers/acpi/arm64/iort.c | 126 +++++++++++++++++++++++++++++++++++++++++-----
 include/acpi/actbl2.h     |   1 +
 2 files changed, 115 insertions(+), 12 deletions(-)

-- 
1.7.12.4

[RFC PATCH 1/4] ACPICA: Add SMMUv3 device ID mapping index support

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

SMMUv3 device ID mapping index is used for SMMUv3
MSIs, update the IORT to support that.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 include/acpi/actbl2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index d568948..e9a9842 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -809,6 +809,7 @@ struct acpi_iort_smmu_v3 {
 	u8 pxm;
 	u8 reserved1;
 	u16 reserved2;
+	u32 id_mapping_index;
 };
 
 /* Values for Model field above */
-- 
1.7.12.4

[RFC PATCH 2/4] ACPI: IORT: lookup iort node via fwnode

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Now we have a helper function iort_get_fwnode() which
lookup fwnode via iort node for SMMU, but sometimes we
just need something exctly the opposite, which means we
need to get the iort node via fwnode.

For example, we need to get SMMU's iort node when adding
support for SMMU MSI, but SMMU is not a named component
which has a associated device node in DSDT, that means
we can't match the ACPI full path name to get the iort
node for SMMU.

But with SMMU or other devices in IORT probed as platform
device, it created a fwnode to associate with the iort
node, so we introduce iort_get_iort_node() to get the
iort node via fwnode.

This can be extended to PMCG node usage in IORT too.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/arm64/iort.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index fa99798..32bd4a4 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -126,6 +126,31 @@ static inline void iort_delete_fwnode(struct acpi_iort_node *node)
 	spin_unlock(&iort_fwnode_lock);
 }
 
+/**
+ * iort_get_iort_node() - Retrieve iort_node associated with an fwnode
+ *
+ * @fwnode: fwnode associated with device to be looked-up
+ *
+ * Returns: iort_node pointer on success, NULL on failure
+ */
+static inline
+struct acpi_iort_node *iort_get_iort_node(struct fwnode_handle *fwnode)
+{
+	struct iort_fwnode *curr;
+	struct acpi_iort_node *iort_node = NULL;
+
+	spin_lock(&iort_fwnode_lock);
+	list_for_each_entry(curr, &iort_fwnode_list, list) {
+		if (curr->fwnode == fwnode) {
+			iort_node = curr->iort_node;
+			break;
+		}
+	}
+	spin_unlock(&iort_fwnode_lock);
+
+	return iort_node;
+}
+
 typedef acpi_status (*iort_find_node_callback)
 	(struct acpi_iort_node *node, void *context);
 
@@ -424,9 +449,25 @@ static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
 {
 	struct pci_bus *pbus;
 
-	if (!dev_is_pci(dev))
+	if (!dev_is_pci(dev)) {
+		struct acpi_iort_node *node;
+		/*
+		 * scan iort_fwnode_list to see if it's an iort platform
+		 * device (such as SMMU, PMCG),its iort node already cached
+		 * and associated with fwnode when iort platform devices
+		 * were initialized.
+		 */
+		node = iort_get_iort_node(dev->fwnode);
+		if (node)
+			return node;
+
+		/*
+		 * if not, then it should be a platform device defined in
+		 * DSDT/SSDT (with Named Component node in IORT)
+		 */
 		return iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
 				      iort_match_node_callback, dev);
+	}
 
 	/* Find a PCI root bus */
 	pbus = to_pci_dev(dev)->bus;
-- 
1.7.12.4

[RFC PATCH 3/4] ACPI: IORT: Skip SMMUv3 device ID map for two steps mappings

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

IORT revision C introduced SMMUv3 MSI support which adding a
device ID mapping index in SMMUv3 sub table, to get the SMMUv3
device ID mapping for the output ID (dev ID for ITS) and the
link to which ITS.

So if a platform supports SMMUv3 MSI for control interrupt,
there will be a additional single map entry under SMMU, this
will not introduce any difference for devices just use one
step map to get its output ID and parent (ITS or SMMU), such
as PCI/NC/PMCG ---> ITS or PCI/NC ---> SMMU, but we need to
do the spcial handling for two steps map case such as
PCI/NC--->SMMUv3--->ITS.

Take a PCI hostbridge for example,

|----------------------|
|  Root Complex Node   |
|----------------------|
|    map entry[x]      |
|----------------------|
|       id value       |
| output_reference     |
|---|------------------|
    |
    |   |----------------------|
    |-->|        SMMUv3        |
        |----------------------|
        |     SMMU dev ID      |
        |     mapping index 0  |
        |----------------------|
        |      map entry[0]    |
        |----------------------|
        |       id value       |
        | output_reference-----------> ITS 1 (SMMU MSI domain)
        |----------------------|
        |      map entry[1]    |
        |----------------------|
        |       id value       |
        | output_reference-----------> ITS 2 (PCI MSI domain)
        |----------------------|

When the SMMU dev ID mapping index is 0, there is entry[0]
to map to a ITS, we need to skip that map entry for PCI
or NC (named component), or we may get the wrong ITS parent.

For now we have two APIs for ID mapping, iort_node_map_id()
and iort_node_map_platform_id(), and iort_node_map_id() is
used for optional two steps mapping, so we just need to
skip the map entry in iort_node_map_id() for non-SMMUv3
devices.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/arm64/iort.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 32bd4a4..9439f02 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -366,6 +366,28 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 	return NULL;
 }
 
+static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
+					     u32 *index)
+{
+	struct acpi_iort_smmu_v3 *smmu;
+
+	/*
+	 * SMMUv3 dev ID mapping index was introdueced in revision 1
+	 * table, not avaible in revision 0
+	 */
+	if (node->revision < 1)
+		return -EINVAL;
+
+	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
+	/* if any of the gsi for control interrupts is not 0, ignore the MSI */
+	if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
+	    || smmu->sync_gsiv)
+		return -EINVAL;
+
+	*index = smmu->id_mapping_index;
+	return 0;
+}
+
 static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 					       u32 id_in, u32 *id_out,
 					       u8 type_mask)
@@ -375,7 +397,9 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 	/* Parse the ID mapping tree to find specified node type */
 	while (node) {
 		struct acpi_iort_id_mapping *map;
-		int i;
+		int i, ret = -EINVAL;
+		/* big enough for an invalid id index in practical */
+		u32 index = U32_MAX;
 
 		if (IORT_TYPE_MASK(node->type) & type_mask) {
 			if (id_out)
@@ -396,8 +420,19 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 			goto fail_map;
 		}
 
+		/*
+		 *  we need to get SMMUv3 dev ID mapping index and skip its
+		 *  associated ID map for single mapping cases.
+		 */
+		if (node->type == ACPI_IORT_NODE_SMMU_V3)
+			ret = iort_get_smmu_v3_id_mapping_index(node, &index);
+
 		/* Do the ID translation */
 		for (i = 0; i < node->mapping_count; i++, map++) {
+			/* if it's a SMMUv3 device id mapping index, skip it */
+			if (!ret && i == index)
+				continue;
+
 			if (!iort_id_map(map, node->type, id, &id))
 				break;
 		}
-- 
1.7.12.4

[RFC PATCH 3/4] ACPI: IORT: Skip SMMUv3 device ID map for two steps mappings

[Robin Murphy]

Hi Hanjun,

It's a nice surprise to see this already; one less thing for us to do :)

On 09/08/17 11:53, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
> 
> IORT revision C introduced SMMUv3 MSI support which adding a
> device ID mapping index in SMMUv3 sub table, to get the SMMUv3
> device ID mapping for the output ID (dev ID for ITS) and the
> link to which ITS.
> 
> So if a platform supports SMMUv3 MSI for control interrupt,
> there will be a additional single map entry under SMMU, this
> will not introduce any difference for devices just use one
> step map to get its output ID and parent (ITS or SMMU), such
> as PCI/NC/PMCG ---> ITS or PCI/NC ---> SMMU, but we need to
> do the spcial handling for two steps map case such as
> PCI/NC--->SMMUv3--->ITS.
> 
> Take a PCI hostbridge for example,
> 
> |----------------------|
> |  Root Complex Node   |
> |----------------------|
> |    map entry[x]      |
> |----------------------|
> |       id value       |
> | output_reference     |
> |---|------------------|
>     |
>     |   |----------------------|
>     |-->|        SMMUv3        |
>         |----------------------|
>         |     SMMU dev ID      |
>         |     mapping index 0  |
>         |----------------------|
>         |      map entry[0]    |
>         |----------------------|
>         |       id value       |
>         | output_reference-----------> ITS 1 (SMMU MSI domain)
>         |----------------------|
>         |      map entry[1]    |
>         |----------------------|
>         |       id value       |
>         | output_reference-----------> ITS 2 (PCI MSI domain)
>         |----------------------|
> 
> When the SMMU dev ID mapping index is 0, there is entry[0]
> to map to a ITS, we need to skip that map entry for PCI
> or NC (named component), or we may get the wrong ITS parent.
> 
> For now we have two APIs for ID mapping, iort_node_map_id()
> and iort_node_map_platform_id(), and iort_node_map_id() is
> used for optional two steps mapping, so we just need to
> skip the map entry in iort_node_map_id() for non-SMMUv3
> devices.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/acpi/arm64/iort.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 32bd4a4..9439f02 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -366,6 +366,28 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>  	return NULL;
>  }
>  
> +static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
> +					     u32 *index)
> +{
> +	struct acpi_iort_smmu_v3 *smmu;
> +
> +	/*
> +	 * SMMUv3 dev ID mapping index was introdueced in revision 1
> +	 * table, not avaible in revision 0
> +	 */
> +	if (node->revision < 1)
> +		return -EINVAL;
> +
> +	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> +	/* if any of the gsi for control interrupts is not 0, ignore the MSI */

IORT says "If all the SMMU control interrupts are GSIV based, this
field is ignored" - not "any". There doesn't seem to be any reason to
disallow a mixture of MSIs and GSIs.

> +	if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
> +	    || smmu->sync_gsiv)
> +		return -EINVAL;
> +
> +	*index = smmu->id_mapping_index;
> +	return 0;
> +}
> +
>  static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>  					       u32 id_in, u32 *id_out,
>  					       u8 type_mask)
> @@ -375,7 +397,9 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>  	/* Parse the ID mapping tree to find specified node type */
>  	while (node) {
>  		struct acpi_iort_id_mapping *map;
> -		int i;
> +		int i, ret = -EINVAL;
> +		/* big enough for an invalid id index in practical */
> +		u32 index = U32_MAX;
>  
>  		if (IORT_TYPE_MASK(node->type) & type_mask) {
>  			if (id_out)
> @@ -396,8 +420,19 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>  			goto fail_map;
>  		}
>  
> +		/*
> +		 *  we need to get SMMUv3 dev ID mapping index and skip its
> +		 *  associated ID map for single mapping cases.
> +		 */
> +		if (node->type == ACPI_IORT_NODE_SMMU_V3)
> +			ret = iort_get_smmu_v3_id_mapping_index(node, &index);
> +
>  		/* Do the ID translation */
>  		for (i = 0; i < node->mapping_count; i++, map++) {
> +			/* if it's a SMMUv3 device id mapping index, skip it */
> +			if (!ret && i == index)

Given that i is an int anyway, there doesn't seem to be much need for
the ret dance - iort_get_smmu_v3_id_mapping_index() could just return
the index/error value as an int directly. You can rely on (i == index)
being false if index is negative, because for node->mapping_count to
overflow INT_MAX the IORT would have to be over 40GB in size, which is
definitely impossible.

Robin.

> +				continue;
> +
>  			if (!iort_id_map(map, node->type, id, &id))
>  				break;
>  		}
> 

[RFC PATCH 3/4] ACPI: IORT: Skip SMMUv3 device ID map for two steps mappings

[Hanjun Guo]

Hi Robin,

On 9 August 2017 at 19:50, Robin Murphy <robin.murphy@arm.com> wrote:
> Hi Hanjun,
>
> It's a nice surprise to see this already; one less thing for us to do :)

Glad to hear this patch set helps :)

>
> On 09/08/17 11:53, Hanjun Guo wrote:
>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> IORT revision C introduced SMMUv3 MSI support which adding a
>> device ID mapping index in SMMUv3 sub table, to get the SMMUv3
>> device ID mapping for the output ID (dev ID for ITS) and the
>> link to which ITS.
>>
>> So if a platform supports SMMUv3 MSI for control interrupt,
>> there will be a additional single map entry under SMMU, this
>> will not introduce any difference for devices just use one
>> step map to get its output ID and parent (ITS or SMMU), such
>> as PCI/NC/PMCG ---> ITS or PCI/NC ---> SMMU, but we need to
>> do the spcial handling for two steps map case such as
>> PCI/NC--->SMMUv3--->ITS.
>>
>> Take a PCI hostbridge for example,
>>
>> |----------------------|
>> |  Root Complex Node   |
>> |----------------------|
>> |    map entry[x]      |
>> |----------------------|
>> |       id value       |
>> | output_reference     |
>> |---|------------------|
>>     |
>>     |   |----------------------|
>>     |-->|        SMMUv3        |
>>         |----------------------|
>>         |     SMMU dev ID      |
>>         |     mapping index 0  |
>>         |----------------------|
>>         |      map entry[0]    |
>>         |----------------------|
>>         |       id value       |
>>         | output_reference-----------> ITS 1 (SMMU MSI domain)
>>         |----------------------|
>>         |      map entry[1]    |
>>         |----------------------|
>>         |       id value       |
>>         | output_reference-----------> ITS 2 (PCI MSI domain)
>>         |----------------------|
>>
>> When the SMMU dev ID mapping index is 0, there is entry[0]
>> to map to a ITS, we need to skip that map entry for PCI
>> or NC (named component), or we may get the wrong ITS parent.
>>
>> For now we have two APIs for ID mapping, iort_node_map_id()
>> and iort_node_map_platform_id(), and iort_node_map_id() is
>> used for optional two steps mapping, so we just need to
>> skip the map entry in iort_node_map_id() for non-SMMUv3
>> devices.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>  drivers/acpi/arm64/iort.c | 37 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 32bd4a4..9439f02 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -366,6 +366,28 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>>       return NULL;
>>  }
>>
>> +static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
>> +                                          u32 *index)
>> +{
>> +     struct acpi_iort_smmu_v3 *smmu;
>> +
>> +     /*
>> +      * SMMUv3 dev ID mapping index was introdueced in revision 1
>> +      * table, not avaible in revision 0
>> +      */
>> +     if (node->revision < 1)
>> +             return -EINVAL;
>> +
>> +     smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>> +     /* if any of the gsi for control interrupts is not 0, ignore the MSI */
>
> IORT says "If all the SMMU control interrupts are GSIV based, this
> field is ignored" - not "any". There doesn't seem to be any reason to
> disallow a mixture of MSIs and GSIs.

Hmm, since GSIV for control interrupts are SPI, those GSIVs should not
be zero, does it mean we need the code below?

if (smmu->event_gsiv && smmu->pri_gsiv && smmu->gerr_gsiv && smmu->sync_gsiv)
        return -EINVAL;

This seems not consistent with the code for now (parsing
the SMMU GSIV for SMMU platform device).

>
>> +     if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
>> +         || smmu->sync_gsiv)
>> +             return -EINVAL;
>> +
>> +     *index = smmu->id_mapping_index;
>> +     return 0;
>> +}
>> +
>>  static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>                                              u32 id_in, u32 *id_out,
>>                                              u8 type_mask)
>> @@ -375,7 +397,9 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>       /* Parse the ID mapping tree to find specified node type */
>>       while (node) {
>>               struct acpi_iort_id_mapping *map;
>> -             int i;
>> +             int i, ret = -EINVAL;
>> +             /* big enough for an invalid id index in practical */
>> +             u32 index = U32_MAX;
>>
>>               if (IORT_TYPE_MASK(node->type) & type_mask) {
>>                       if (id_out)
>> @@ -396,8 +420,19 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>                       goto fail_map;
>>               }
>>
>> +             /*
>> +              *  we need to get SMMUv3 dev ID mapping index and skip its
>> +              *  associated ID map for single mapping cases.
>> +              */
>> +             if (node->type == ACPI_IORT_NODE_SMMU_V3)
>> +                     ret = iort_get_smmu_v3_id_mapping_index(node, &index);
>> +
>>               /* Do the ID translation */
>>               for (i = 0; i < node->mapping_count; i++, map++) {
>> +                     /* if it's a SMMUv3 device id mapping index, skip it */
>> +                     if (!ret && i == index)
>
> Given that i is an int anyway, there doesn't seem to be much need for
> the ret dance - iort_get_smmu_v3_id_mapping_index() could just return
> the index/error value as an int directly. You can rely on (i == index)
> being false if index is negative, because for node->mapping_count to
> overflow INT_MAX the IORT would have to be over 40GB in size, which is
> definitely impossible.

Good point, it will simplify the code, I will update this patch :)

Thanks
Hanjun

[RFC PATCH 3/4] ACPI: IORT: Skip SMMUv3 device ID map for two steps mappings

[Lorenzo Pieralisi]

On Wed, Aug 09, 2017 at 10:48:00PM +0800, Hanjun Guo wrote:
> Hi Robin,
> 
> On 9 August 2017 at 19:50, Robin Murphy <robin.murphy@arm.com> wrote:
> > Hi Hanjun,
> >
> > It's a nice surprise to see this already; one less thing for us to do :)
> 
> Glad to hear this patch set helps :)
> 
> >
> > On 09/08/17 11:53, Hanjun Guo wrote:
> >> From: Hanjun Guo <hanjun.guo@linaro.org>
> >>
> >> IORT revision C introduced SMMUv3 MSI support which adding a
> >> device ID mapping index in SMMUv3 sub table, to get the SMMUv3
> >> device ID mapping for the output ID (dev ID for ITS) and the
> >> link to which ITS.
> >>
> >> So if a platform supports SMMUv3 MSI for control interrupt,
> >> there will be a additional single map entry under SMMU, this
> >> will not introduce any difference for devices just use one
> >> step map to get its output ID and parent (ITS or SMMU), such
> >> as PCI/NC/PMCG ---> ITS or PCI/NC ---> SMMU, but we need to
> >> do the spcial handling for two steps map case such as
> >> PCI/NC--->SMMUv3--->ITS.
> >>
> >> Take a PCI hostbridge for example,
> >>
> >> |----------------------|
> >> |  Root Complex Node   |
> >> |----------------------|
> >> |    map entry[x]      |
> >> |----------------------|
> >> |       id value       |
> >> | output_reference     |
> >> |---|------------------|
> >>     |
> >>     |   |----------------------|
> >>     |-->|        SMMUv3        |
> >>         |----------------------|
> >>         |     SMMU dev ID      |
> >>         |     mapping index 0  |
> >>         |----------------------|
> >>         |      map entry[0]    |
> >>         |----------------------|
> >>         |       id value       |
> >>         | output_reference-----------> ITS 1 (SMMU MSI domain)
> >>         |----------------------|
> >>         |      map entry[1]    |
> >>         |----------------------|
> >>         |       id value       |
> >>         | output_reference-----------> ITS 2 (PCI MSI domain)
> >>         |----------------------|
> >>
> >> When the SMMU dev ID mapping index is 0, there is entry[0]
> >> to map to a ITS, we need to skip that map entry for PCI
> >> or NC (named component), or we may get the wrong ITS parent.
> >>
> >> For now we have two APIs for ID mapping, iort_node_map_id()
> >> and iort_node_map_platform_id(), and iort_node_map_id() is
> >> used for optional two steps mapping, so we just need to
> >> skip the map entry in iort_node_map_id() for non-SMMUv3
> >> devices.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> ---
> >>  drivers/acpi/arm64/iort.c | 37 ++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 36 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >> index 32bd4a4..9439f02 100644
> >> --- a/drivers/acpi/arm64/iort.c
> >> +++ b/drivers/acpi/arm64/iort.c
> >> @@ -366,6 +366,28 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
> >>       return NULL;
> >>  }
> >>
> >> +static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
> >> +                                          u32 *index)
> >> +{
> >> +     struct acpi_iort_smmu_v3 *smmu;
> >> +
> >> +     /*
> >> +      * SMMUv3 dev ID mapping index was introdueced in revision 1
> >> +      * table, not avaible in revision 0
> >> +      */
> >> +     if (node->revision < 1)
> >> +             return -EINVAL;
> >> +
> >> +     smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> >> +     /* if any of the gsi for control interrupts is not 0, ignore the MSI */
> >
> > IORT says "If all the SMMU control interrupts are GSIV based, this
> > field is ignored" - not "any". There doesn't seem to be any reason to
> > disallow a mixture of MSIs and GSIs.
> 
> Hmm, since GSIV for control interrupts are SPI, those GSIVs should not
> be zero, does it mean we need the code below?
> 
> if (smmu->event_gsiv && smmu->pri_gsiv && smmu->gerr_gsiv && smmu->sync_gsiv)
>         return -EINVAL;
> 
> This seems not consistent with the code for now (parsing
> the SMMU GSIV for SMMU platform device).
> 
> >
> >> +     if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
> >> +         || smmu->sync_gsiv)
> >> +             return -EINVAL;
> >> +
> >> +     *index = smmu->id_mapping_index;
> >> +     return 0;
> >> +}
> >> +
> >>  static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> >>                                              u32 id_in, u32 *id_out,
> >>                                              u8 type_mask)
> >> @@ -375,7 +397,9 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> >>       /* Parse the ID mapping tree to find specified node type */
> >>       while (node) {
> >>               struct acpi_iort_id_mapping *map;
> >> -             int i;
> >> +             int i, ret = -EINVAL;
> >> +             /* big enough for an invalid id index in practical */
> >> +             u32 index = U32_MAX;
> >>
> >>               if (IORT_TYPE_MASK(node->type) & type_mask) {
> >>                       if (id_out)
> >> @@ -396,8 +420,19 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> >>                       goto fail_map;
> >>               }
> >>
> >> +             /*
> >> +              *  we need to get SMMUv3 dev ID mapping index and skip its
> >> +              *  associated ID map for single mapping cases.
> >> +              */
> >> +             if (node->type == ACPI_IORT_NODE_SMMU_V3)
> >> +                     ret = iort_get_smmu_v3_id_mapping_index(node, &index);
> >> +
> >>               /* Do the ID translation */
> >>               for (i = 0; i < node->mapping_count; i++, map++) {
> >> +                     /* if it's a SMMUv3 device id mapping index, skip it */
> >> +                     if (!ret && i == index)
> >
> > Given that i is an int anyway, there doesn't seem to be much need for
> > the ret dance - iort_get_smmu_v3_id_mapping_index() could just return
> > the index/error value as an int directly. You can rely on (i == index)
> > being false if index is negative, because for node->mapping_count to
> > overflow INT_MAX the IORT would have to be over 40GB in size, which is
> > definitely impossible.
> 
> Good point, it will simplify the code, I will update this patch :)

How about:

(1) Ignoring SMMU_V3 single mappings in iort_id_map() (as we do today -
    minus the warning) - we will never need them, just ignore them all
    regarless of id_mapping_index
(2) Write some simple code that instead of relying on the
    iort_pmsi_get_dev_id() API just get the SMMU V3 IORT node mapping
    entry (at id_mapping_index), grab a reference to the ITS and
    look-up the MSI domain

?

I do not see the point in making any of this generic for IORT kernel
code, it is a one-off kludge for SMMU_V3 and can easily be
self-contained IORT code.

Thoughts ?

Lorenzo

[RFC PATCH 3/4] ACPI: IORT: Skip SMMUv3 device ID map for two steps mappings

[Hanjun Guo]

On 2017/8/10 1:12, Lorenzo Pieralisi wrote:
> On Wed, Aug 09, 2017 at 10:48:00PM +0800, Hanjun Guo wrote:
>> Hi Robin,
>>
>> On 9 August 2017 at 19:50, Robin Murphy <robin.murphy@arm.com> wrote:
>>> Hi Hanjun,
>>>
>>> It's a nice surprise to see this already; one less thing for us to do :)
>>
>> Glad to hear this patch set helps :)
>>
>>>
>>> On 09/08/17 11:53, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> IORT revision C introduced SMMUv3 MSI support which adding a
>>>> device ID mapping index in SMMUv3 sub table, to get the SMMUv3
>>>> device ID mapping for the output ID (dev ID for ITS) and the
>>>> link to which ITS.
>>>>
>>>> So if a platform supports SMMUv3 MSI for control interrupt,
>>>> there will be a additional single map entry under SMMU, this
>>>> will not introduce any difference for devices just use one
>>>> step map to get its output ID and parent (ITS or SMMU), such
>>>> as PCI/NC/PMCG ---> ITS or PCI/NC ---> SMMU, but we need to
>>>> do the spcial handling for two steps map case such as
>>>> PCI/NC--->SMMUv3--->ITS.
>>>>
>>>> Take a PCI hostbridge for example,
>>>>
>>>> |----------------------|
>>>> |  Root Complex Node   |
>>>> |----------------------|
>>>> |    map entry[x]      |
>>>> |----------------------|
>>>> |       id value       |
>>>> | output_reference     |
>>>> |---|------------------|
>>>>      |
>>>>      |   |----------------------|
>>>>      |-->|        SMMUv3        |
>>>>          |----------------------|
>>>>          |     SMMU dev ID      |
>>>>          |     mapping index 0  |
>>>>          |----------------------|
>>>>          |      map entry[0]    |
>>>>          |----------------------|
>>>>          |       id value       |
>>>>          | output_reference-----------> ITS 1 (SMMU MSI domain)
>>>>          |----------------------|
>>>>          |      map entry[1]    |
>>>>          |----------------------|
>>>>          |       id value       |
>>>>          | output_reference-----------> ITS 2 (PCI MSI domain)
>>>>          |----------------------|
>>>>
>>>> When the SMMU dev ID mapping index is 0, there is entry[0]
>>>> to map to a ITS, we need to skip that map entry for PCI
>>>> or NC (named component), or we may get the wrong ITS parent.
>>>>
>>>> For now we have two APIs for ID mapping, iort_node_map_id()
>>>> and iort_node_map_platform_id(), and iort_node_map_id() is
>>>> used for optional two steps mapping, so we just need to
>>>> skip the map entry in iort_node_map_id() for non-SMMUv3
>>>> devices.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>   drivers/acpi/arm64/iort.c | 37 ++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 36 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>> index 32bd4a4..9439f02 100644
>>>> --- a/drivers/acpi/arm64/iort.c
>>>> +++ b/drivers/acpi/arm64/iort.c
>>>> @@ -366,6 +366,28 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>>>>        return NULL;
>>>>   }
>>>>
>>>> +static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
>>>> +                                          u32 *index)
>>>> +{
>>>> +     struct acpi_iort_smmu_v3 *smmu;
>>>> +
>>>> +     /*
>>>> +      * SMMUv3 dev ID mapping index was introdueced in revision 1
>>>> +      * table, not avaible in revision 0
>>>> +      */
>>>> +     if (node->revision < 1)
>>>> +             return -EINVAL;
>>>> +
>>>> +     smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>>>> +     /* if any of the gsi for control interrupts is not 0, ignore the MSI */
>>>
>>> IORT says "If all the SMMU control interrupts are GSIV based, this
>>> field is ignored" - not "any". There doesn't seem to be any reason to
>>> disallow a mixture of MSIs and GSIs.
>>
>> Hmm, since GSIV for control interrupts are SPI, those GSIVs should not
>> be zero, does it mean we need the code below?
>>
>> if (smmu->event_gsiv && smmu->pri_gsiv && smmu->gerr_gsiv && smmu->sync_gsiv)
>>          return -EINVAL;
>>
>> This seems not consistent with the code for now (parsing
>> the SMMU GSIV for SMMU platform device).
>>
>>>
>>>> +     if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
>>>> +         || smmu->sync_gsiv)
>>>> +             return -EINVAL;
>>>> +
>>>> +     *index = smmu->id_mapping_index;
>>>> +     return 0;
>>>> +}
>>>> +
>>>>   static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>>>                                               u32 id_in, u32 *id_out,
>>>>                                               u8 type_mask)
>>>> @@ -375,7 +397,9 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>>>        /* Parse the ID mapping tree to find specified node type */
>>>>        while (node) {
>>>>                struct acpi_iort_id_mapping *map;
>>>> -             int i;
>>>> +             int i, ret = -EINVAL;
>>>> +             /* big enough for an invalid id index in practical */
>>>> +             u32 index = U32_MAX;
>>>>
>>>>                if (IORT_TYPE_MASK(node->type) & type_mask) {
>>>>                        if (id_out)
>>>> @@ -396,8 +420,19 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>>>                        goto fail_map;
>>>>                }
>>>>
>>>> +             /*
>>>> +              *  we need to get SMMUv3 dev ID mapping index and skip its
>>>> +              *  associated ID map for single mapping cases.
>>>> +              */
>>>> +             if (node->type == ACPI_IORT_NODE_SMMU_V3)
>>>> +                     ret = iort_get_smmu_v3_id_mapping_index(node, &index);
>>>> +
>>>>                /* Do the ID translation */
>>>>                for (i = 0; i < node->mapping_count; i++, map++) {
>>>> +                     /* if it's a SMMUv3 device id mapping index, skip it */
>>>> +                     if (!ret && i == index)
>>>
>>> Given that i is an int anyway, there doesn't seem to be much need for
>>> the ret dance - iort_get_smmu_v3_id_mapping_index() could just return
>>> the index/error value as an int directly. You can rely on (i == index)
>>> being false if index is negative, because for node->mapping_count to
>>> overflow INT_MAX the IORT would have to be over 40GB in size, which is
>>> definitely impossible.
>>
>> Good point, it will simplify the code, I will update this patch :)
> 
> How about:
> 
> (1) Ignoring SMMU_V3 single mappings in iort_id_map() (as we do today -
>      minus the warning) - we will never need them, just ignore them all
>      regarless of id_mapping_index

I was thinking the same when I prepare the code, but one thing stopped
me to do that, which is if we have multi single mappings under SMMU,
such as PCI/NC---->SMMU---(single mapping)-->ITS, then all the single
mappings will be skipped.

I'm didn't see such mapping (HW) in practical for now, and I'm not
sure if it's a valid mapping, from the spec, IORT doesn't forbid such
mapping.

> (2) Write some simple code that instead of relying on the
>      iort_pmsi_get_dev_id() API just get the SMMU V3 IORT node mapping
>      entry (at id_mapping_index), grab a reference to the ITS and
>      look-up the MSI domain

That will be pretty the same as iort_node_get_id() then to use the
parent to get the MSI irq domain.

> 
> ?
> 
> I do not see the point in making any of this generic for IORT kernel
> code, it is a one-off kludge for SMMU_V3 and can easily be
> self-contained IORT code.
> 
> Thoughts ?

Please see above :)

Thanks
Hanjun

[RFC PATCH 3/4] ACPI: IORT: Skip SMMUv3 device ID map for two steps mappings

[Lorenzo Pieralisi]

On Wed, Aug 09, 2017 at 06:53:36PM +0800, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
> 
> IORT revision C introduced SMMUv3 MSI support which adding a
> device ID mapping index in SMMUv3 sub table, to get the SMMUv3
> device ID mapping for the output ID (dev ID for ITS) and the
> link to which ITS.
> 
> So if a platform supports SMMUv3 MSI for control interrupt,
> there will be a additional single map entry under SMMU, this
> will not introduce any difference for devices just use one
> step map to get its output ID and parent (ITS or SMMU), such
> as PCI/NC/PMCG ---> ITS or PCI/NC ---> SMMU, but we need to
> do the spcial handling for two steps map case such as
> PCI/NC--->SMMUv3--->ITS.
> 
> Take a PCI hostbridge for example,
> 
> |----------------------|
> |  Root Complex Node   |
> |----------------------|
> |    map entry[x]      |
> |----------------------|
> |       id value       |
> | output_reference     |
> |---|------------------|
>     |
>     |   |----------------------|
>     |-->|        SMMUv3        |
>         |----------------------|
>         |     SMMU dev ID      |
>         |     mapping index 0  |
>         |----------------------|
>         |      map entry[0]    |
>         |----------------------|
>         |       id value       |
>         | output_reference-----------> ITS 1 (SMMU MSI domain)
>         |----------------------|
>         |      map entry[1]    |
>         |----------------------|
>         |       id value       |
>         | output_reference-----------> ITS 2 (PCI MSI domain)
>         |----------------------|
> 
> When the SMMU dev ID mapping index is 0, there is entry[0]
> to map to a ITS, we need to skip that map entry for PCI
> or NC (named component), or we may get the wrong ITS parent.
> 
> For now we have two APIs for ID mapping, iort_node_map_id()
> and iort_node_map_platform_id(), and iort_node_map_id() is
> used for optional two steps mapping, so we just need to
> skip the map entry in iort_node_map_id() for non-SMMUv3
> devices.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/acpi/arm64/iort.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)

Tweaked it slightly, so that you can apply my 4/4 on top of it:

-- >8 --
Subject: [PATCH 3/4] ACPI: IORT: Skip SMMUv3 device ID map for two steps
 mappings

IORT revision C introduced SMMUv3 MSI support which adding a
device ID mapping index in SMMUv3 sub table, to get the SMMUv3
device ID mapping for the output ID (dev ID for ITS) and the
link to which ITS.

So if a platform supports SMMUv3 MSI for control interrupt,
there will be a additional single map entry under SMMU, this
will not introduce any difference for devices just use one
step map to get its output ID and parent (ITS or SMMU), such
as PCI/NC/PMCG ---> ITS or PCI/NC ---> SMMU, but we need to
do the spcial handling for two steps map case such as
PCI/NC--->SMMUv3--->ITS.

Take a PCI hostbridge for example,

|----------------------|
|  Root Complex Node   |
|----------------------|
|    map entry[x]      |
|----------------------|
|       id value       |
| output_reference     |
|---|------------------|
    |
    |   |----------------------|
    |-->|        SMMUv3        |
        |----------------------|
        |     SMMU dev ID      |
        |     mapping index 0  |
        |----------------------|
        |      map entry[0]    |
        |----------------------|
        |       id value       |
        | output_reference-----------> ITS 1 (SMMU MSI domain)
        |----------------------|
        |      map entry[1]    |
        |----------------------|
        |       id value       |
        | output_reference-----------> ITS 2 (PCI MSI domain)
        |----------------------|

When the SMMU dev ID mapping index is 0, there is entry[0]
to map to a ITS, we need to skip that map entry for PCI
or NC (named component), or we may get the wrong ITS parent.

For now we have two APIs for ID mapping, iort_node_map_id()
and iort_node_map_platform_id(), and iort_node_map_id() is
used for optional two steps mapping, so we just need to
skip the map entry in iort_node_map_id() for non-SMMUv3
devices.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/arm64/iort.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 2514845..24678c3 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -366,6 +366,34 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 	return NULL;
 }
 
+static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
+					     u32 *index)
+{
+	struct acpi_iort_smmu_v3 *smmu;
+
+	/*
+	 * SMMUv3 dev ID mapping index was introdueced in revision 1
+	 * table, not avaible in revision 0
+	 */
+	if (node->revision < 1)
+		return -EINVAL;
+
+	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
+	/* if any of the gsi for control interrupts is not 0, ignore the MSI */
+	if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
+	    || smmu->sync_gsiv)
+		return -EINVAL;
+
+	if (smmu->id_mapping_index >= node->mapping_count) {
+		pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n",
+		       node, node->type);
+		return -EINVAL;
+	}
+
+	*index = smmu->id_mapping_index;
+	return 0;
+}
+
 static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 					       u32 id_in, u32 *id_out,
 					       u8 type_mask)
@@ -375,7 +403,9 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 	/* Parse the ID mapping tree to find specified node type */
 	while (node) {
 		struct acpi_iort_id_mapping *map;
-		int i;
+		int i, ret = -EINVAL;
+		/* big enough for an invalid id index in practical */
+		u32 index = U32_MAX;
 
 		if (IORT_TYPE_MASK(node->type) & type_mask) {
 			if (id_out)
@@ -396,8 +426,19 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 			goto fail_map;
 		}
 
+		/*
+		 *  we need to get SMMUv3 dev ID mapping index and skip its
+		 *  associated ID map for single mapping cases.
+		 */
+		if (node->type == ACPI_IORT_NODE_SMMU_V3)
+			ret = iort_get_smmu_v3_id_mapping_index(node, &index);
+
 		/* Do the ID translation */
 		for (i = 0; i < node->mapping_count; i++, map++) {
+			/* if it's a SMMUv3 device id mapping index, skip it */
+			if (!ret && i == index)
+				continue;
+
 			if (!iort_id_map(map, node->type, id, &id))
 				break;
 		}
-- 
2.10.0

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Since we have a mapping index for SMMUv3 MSI, we can
directly use that index to get the map entry, then
retrieve dev ID and ITS parent to add SMMUv3 MSI
support.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/arm64/iort.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 9439f02..ce03298 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -313,7 +313,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
 	/* Single mapping does not care for input id */
 	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
 		if (type == ACPI_IORT_NODE_NAMED_COMPONENT ||
-		    type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
+		    type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
+		    type == ACPI_IORT_NODE_SMMU_V3) {
 			*rid_out = map->output_base;
 			return 0;
 		}
@@ -357,7 +358,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 
 	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
 		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
-		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
+		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
+		    node->type == ACPI_IORT_NODE_SMMU_V3) {
 			*id_out = map->output_base;
 			return parent;
 		}
@@ -549,9 +551,21 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
 	if (!node)
 		return -ENODEV;
 
-	for (i = 0; i < node->mapping_count; i++) {
-		if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i))
+	if (node->type == ACPI_IORT_NODE_SMMU_V3) {
+		u32 index;
+
+		if (iort_get_smmu_v3_id_mapping_index(node, &index))
+			return -ENODEV;
+
+		if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE,
+		    index))
 			return 0;
+	} else {
+		for (i = 0; i < node->mapping_count; i++) {
+			if (iort_node_map_platform_id(node, dev_id,
+			    IORT_MSI_TYPE, i))
+				return 0;
+		}
 	}
 
 	return -ENODEV;
@@ -626,20 +640,30 @@ static struct irq_domain *iort_get_platform_device_domain(struct device *dev)
 	struct acpi_iort_node *node, *msi_parent;
 	struct fwnode_handle *iort_fwnode;
 	struct acpi_iort_its_group *its;
-	int i;
 
 	/* find its associated iort node */
-	node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
-			      iort_match_node_callback, dev);
+	node = iort_find_dev_node(dev);
 	if (!node)
 		return NULL;
 
 	/* then find its msi parent node */
-	for (i = 0; i < node->mapping_count; i++) {
+	if (node->type == ACPI_IORT_NODE_SMMU_V3) {
+		u32 index;
+
+		if (iort_get_smmu_v3_id_mapping_index(node, &index))
+			return NULL;
+
 		msi_parent = iort_node_map_platform_id(node, NULL,
+						       IORT_MSI_TYPE, index);
+	} else {
+		int i;
+
+		for (i = 0; i < node->mapping_count; i++) {
+			msi_parent = iort_node_map_platform_id(node, NULL,
 						       IORT_MSI_TYPE, i);
-		if (msi_parent)
-			break;
+			if (msi_parent)
+				break;
+		}
 	}
 
 	if (!msi_parent)
@@ -1233,6 +1257,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
 	/* Configure DMA for the page table walker */
 	acpi_dma_configure(&pdev->dev, attr);
 
+	acpi_configure_pmsi_domain(&pdev->dev);
+
 	ret = platform_device_add(pdev);
 	if (ret)
 		goto dma_deconfigure;
-- 
1.7.12.4

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Lorenzo Pieralisi]

On Wed, Aug 09, 2017 at 06:53:37PM +0800, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
> 
> Since we have a mapping index for SMMUv3 MSI, we can
> directly use that index to get the map entry, then
> retrieve dev ID and ITS parent to add SMMUv3 MSI
> support.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/acpi/arm64/iort.c | 46 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 9439f02..ce03298 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -313,7 +313,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>  	/* Single mapping does not care for input id */
>  	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>  		if (type == ACPI_IORT_NODE_NAMED_COMPONENT ||
> -		    type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
> +		    type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> +		    type == ACPI_IORT_NODE_SMMU_V3) {
>  			*rid_out = map->output_base;
>  			return 0;
>  		}
> @@ -357,7 +358,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>  
>  	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>  		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
> -		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
> +		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> +		    node->type == ACPI_IORT_NODE_SMMU_V3) {
>  			*id_out = map->output_base;
>  			return parent;
>  		}
> @@ -549,9 +551,21 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
>  	if (!node)
>  		return -ENODEV;
>  
> -	for (i = 0; i < node->mapping_count; i++) {
> -		if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i))
> +	if (node->type == ACPI_IORT_NODE_SMMU_V3) {
> +		u32 index;
> +
> +		if (iort_get_smmu_v3_id_mapping_index(node, &index))
> +			return -ENODEV;
> +
> +		if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE,
> +		    index))
>  			return 0;
> +	} else {
> +		for (i = 0; i < node->mapping_count; i++) {
> +			if (iort_node_map_platform_id(node, dev_id,
> +			    IORT_MSI_TYPE, i))
> +				return 0;
> +		}
>  	}
>  
>  	return -ENODEV;
> @@ -626,20 +640,30 @@ static struct irq_domain *iort_get_platform_device_domain(struct device *dev)
>  	struct acpi_iort_node *node, *msi_parent;
>  	struct fwnode_handle *iort_fwnode;
>  	struct acpi_iort_its_group *its;
> -	int i;
>  
>  	/* find its associated iort node */
> -	node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> -			      iort_match_node_callback, dev);
> +	node = iort_find_dev_node(dev);
>  	if (!node)
>  		return NULL;
>  
>  	/* then find its msi parent node */
> -	for (i = 0; i < node->mapping_count; i++) {
> +	if (node->type == ACPI_IORT_NODE_SMMU_V3) {
> +		u32 index;
> +
> +		if (iort_get_smmu_v3_id_mapping_index(node, &index))
> +			return NULL;
> +
>  		msi_parent = iort_node_map_platform_id(node, NULL,
> +						       IORT_MSI_TYPE, index);
> +	} else {
> +		int i;
> +
> +		for (i = 0; i < node->mapping_count; i++) {
> +			msi_parent = iort_node_map_platform_id(node, NULL,
>  						       IORT_MSI_TYPE, i);
> -		if (msi_parent)
> -			break;
> +			if (msi_parent)
> +				break;
> +		}
>  	}
>  
>  	if (!msi_parent)
> @@ -1233,6 +1257,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
>  	/* Configure DMA for the page table walker */
>  	acpi_dma_configure(&pdev->dev, attr);
>  
> +	acpi_configure_pmsi_domain(&pdev->dev);

I think this is just overkill. There are two separate things to solve
here:

1) Make single mappings valid for SMMUv3 (and PMCG); that's fair enough
   and goes with the logic to skip the ITS DeviceID index for "normal"
   mappings, I can live with that
2) Find the MSI domain for an SMMUv3 (or any other IORT table node); I
   do not think you need all this complexity to do it via
   acpi_configure_pmsi_domain(), it can be done in a easier way with
   an ad-hoc stub (it does not even have to be SMMUv3 specific)

My worry is that we are peppering the generic IORT mapping code with
node types specific kludges and it is becoming a mess.

I can rework the patch to show you what I have in mind, please let
me know.

Thanks,
Lorenzo

> +
>  	ret = platform_device_add(pdev);
>  	if (ret)
>  		goto dma_deconfigure;
> -- 
> 1.7.12.4
> 

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Hanjun Guo]

Hi Lorenzo,

On 11 August 2017 at 17:33, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Wed, Aug 09, 2017 at 06:53:37PM +0800, Hanjun Guo wrote:
>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> Since we have a mapping index for SMMUv3 MSI, we can
>> directly use that index to get the map entry, then
>> retrieve dev ID and ITS parent to add SMMUv3 MSI
>> support.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>  drivers/acpi/arm64/iort.c | 46 ++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 36 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 9439f02..ce03298 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -313,7 +313,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>>       /* Single mapping does not care for input id */
>>       if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>>               if (type == ACPI_IORT_NODE_NAMED_COMPONENT ||
>> -                 type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
>> +                 type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
>> +                 type == ACPI_IORT_NODE_SMMU_V3) {
>>                       *rid_out = map->output_base;
>>                       return 0;
>>               }
>> @@ -357,7 +358,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>>
>>       if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>>               if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
>> -                 node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
>> +                 node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
>> +                 node->type == ACPI_IORT_NODE_SMMU_V3) {
>>                       *id_out = map->output_base;
>>                       return parent;
>>               }
>> @@ -549,9 +551,21 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
>>       if (!node)
>>               return -ENODEV;
>>
>> -     for (i = 0; i < node->mapping_count; i++) {
>> -             if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i))
>> +     if (node->type == ACPI_IORT_NODE_SMMU_V3) {
>> +             u32 index;
>> +
>> +             if (iort_get_smmu_v3_id_mapping_index(node, &index))
>> +                     return -ENODEV;
>> +
>> +             if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE,
>> +                 index))
>>                       return 0;
>> +     } else {
>> +             for (i = 0; i < node->mapping_count; i++) {
>> +                     if (iort_node_map_platform_id(node, dev_id,
>> +                         IORT_MSI_TYPE, i))
>> +                             return 0;
>> +             }
>>       }
>>
>>       return -ENODEV;
>> @@ -626,20 +640,30 @@ static struct irq_domain *iort_get_platform_device_domain(struct device *dev)
>>       struct acpi_iort_node *node, *msi_parent;
>>       struct fwnode_handle *iort_fwnode;
>>       struct acpi_iort_its_group *its;
>> -     int i;
>>
>>       /* find its associated iort node */
>> -     node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
>> -                           iort_match_node_callback, dev);
>> +     node = iort_find_dev_node(dev);
>>       if (!node)
>>               return NULL;
>>
>>       /* then find its msi parent node */
>> -     for (i = 0; i < node->mapping_count; i++) {
>> +     if (node->type == ACPI_IORT_NODE_SMMU_V3) {
>> +             u32 index;
>> +
>> +             if (iort_get_smmu_v3_id_mapping_index(node, &index))
>> +                     return NULL;
>> +
>>               msi_parent = iort_node_map_platform_id(node, NULL,
>> +                                                    IORT_MSI_TYPE, index);
>> +     } else {
>> +             int i;
>> +
>> +             for (i = 0; i < node->mapping_count; i++) {
>> +                     msi_parent = iort_node_map_platform_id(node, NULL,
>>                                                      IORT_MSI_TYPE, i);
>> -             if (msi_parent)
>> -                     break;
>> +                     if (msi_parent)
>> +                             break;
>> +             }
>>       }
>>
>>       if (!msi_parent)
>> @@ -1233,6 +1257,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
>>       /* Configure DMA for the page table walker */
>>       acpi_dma_configure(&pdev->dev, attr);
>>
>> +     acpi_configure_pmsi_domain(&pdev->dev);
>
> I think this is just overkill. There are two separate things to solve
> here:
>
> 1) Make single mappings valid for SMMUv3 (and PMCG); that's fair enough
>    and goes with the logic to skip the ITS DeviceID index for "normal"
>    mappings, I can live with that
> 2) Find the MSI domain for an SMMUv3 (or any other IORT table node); I
>    do not think you need all this complexity to do it via
>    acpi_configure_pmsi_domain(), it can be done in a easier way with
>    an ad-hoc stub (it does not even have to be SMMUv3 specific)
>
> My worry is that we are peppering the generic IORT mapping code with
> node types specific kludges and it is becoming a mess.

Agreed.

>
> I can rework the patch to show you what I have in mind, please let
> me know.

Please, thank you very much for doing so.

Thanks
Hanjun

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Lorenzo Pieralisi]

On Fri, Aug 11, 2017 at 09:56:31PM +0800, Hanjun Guo wrote:
> Hi Lorenzo,
> 
> On 11 August 2017 at 17:33, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Aug 09, 2017 at 06:53:37PM +0800, Hanjun Guo wrote:
> >> From: Hanjun Guo <hanjun.guo@linaro.org>
> >>
> >> Since we have a mapping index for SMMUv3 MSI, we can
> >> directly use that index to get the map entry, then
> >> retrieve dev ID and ITS parent to add SMMUv3 MSI
> >> support.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> ---
> >>  drivers/acpi/arm64/iort.c | 46 ++++++++++++++++++++++++++++++++++++----------
> >>  1 file changed, 36 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >> index 9439f02..ce03298 100644
> >> --- a/drivers/acpi/arm64/iort.c
> >> +++ b/drivers/acpi/arm64/iort.c
> >> @@ -313,7 +313,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> >>       /* Single mapping does not care for input id */
> >>       if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
> >>               if (type == ACPI_IORT_NODE_NAMED_COMPONENT ||
> >> -                 type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
> >> +                 type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> >> +                 type == ACPI_IORT_NODE_SMMU_V3) {
> >>                       *rid_out = map->output_base;
> >>                       return 0;
> >>               }
> >> @@ -357,7 +358,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
> >>
> >>       if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
> >>               if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
> >> -                 node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
> >> +                 node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> >> +                 node->type == ACPI_IORT_NODE_SMMU_V3) {
> >>                       *id_out = map->output_base;
> >>                       return parent;
> >>               }
> >> @@ -549,9 +551,21 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
> >>       if (!node)
> >>               return -ENODEV;
> >>
> >> -     for (i = 0; i < node->mapping_count; i++) {
> >> -             if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i))
> >> +     if (node->type == ACPI_IORT_NODE_SMMU_V3) {
> >> +             u32 index;
> >> +
> >> +             if (iort_get_smmu_v3_id_mapping_index(node, &index))
> >> +                     return -ENODEV;
> >> +
> >> +             if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE,
> >> +                 index))
> >>                       return 0;
> >> +     } else {
> >> +             for (i = 0; i < node->mapping_count; i++) {
> >> +                     if (iort_node_map_platform_id(node, dev_id,
> >> +                         IORT_MSI_TYPE, i))
> >> +                             return 0;
> >> +             }
> >>       }
> >>
> >>       return -ENODEV;
> >> @@ -626,20 +640,30 @@ static struct irq_domain *iort_get_platform_device_domain(struct device *dev)
> >>       struct acpi_iort_node *node, *msi_parent;
> >>       struct fwnode_handle *iort_fwnode;
> >>       struct acpi_iort_its_group *its;
> >> -     int i;
> >>
> >>       /* find its associated iort node */
> >> -     node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> >> -                           iort_match_node_callback, dev);
> >> +     node = iort_find_dev_node(dev);
> >>       if (!node)
> >>               return NULL;
> >>
> >>       /* then find its msi parent node */
> >> -     for (i = 0; i < node->mapping_count; i++) {
> >> +     if (node->type == ACPI_IORT_NODE_SMMU_V3) {
> >> +             u32 index;
> >> +
> >> +             if (iort_get_smmu_v3_id_mapping_index(node, &index))
> >> +                     return NULL;
> >> +
> >>               msi_parent = iort_node_map_platform_id(node, NULL,
> >> +                                                    IORT_MSI_TYPE, index);
> >> +     } else {
> >> +             int i;
> >> +
> >> +             for (i = 0; i < node->mapping_count; i++) {
> >> +                     msi_parent = iort_node_map_platform_id(node, NULL,
> >>                                                      IORT_MSI_TYPE, i);
> >> -             if (msi_parent)
> >> -                     break;
> >> +                     if (msi_parent)
> >> +                             break;
> >> +             }
> >>       }
> >>
> >>       if (!msi_parent)
> >> @@ -1233,6 +1257,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
> >>       /* Configure DMA for the page table walker */
> >>       acpi_dma_configure(&pdev->dev, attr);
> >>
> >> +     acpi_configure_pmsi_domain(&pdev->dev);
> >
> > I think this is just overkill. There are two separate things to solve
> > here:
> >
> > 1) Make single mappings valid for SMMUv3 (and PMCG); that's fair enough
> >    and goes with the logic to skip the ITS DeviceID index for "normal"
> >    mappings, I can live with that
> > 2) Find the MSI domain for an SMMUv3 (or any other IORT table node); I
> >    do not think you need all this complexity to do it via
> >    acpi_configure_pmsi_domain(), it can be done in a easier way with
> >    an ad-hoc stub (it does not even have to be SMMUv3 specific)
> >
> > My worry is that we are peppering the generic IORT mapping code with
> > node types specific kludges and it is becoming a mess.
> 
> Agreed.
> 
> >
> > I can rework the patch to show you what I have in mind, please let
> > me know.
> 
> Please, thank you very much for doing so.

Here, untested just to get your opinion, please let me know.

Thanks,
Lorenzo

-- >8 --
Subject: [PATCH 4/4] ACPI/IORT: IORT nodes MSI handling

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/acpi/arm64/iort.c | 76 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 24678c3..21a0aab 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -366,7 +366,7 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 	return NULL;
 }
 
-static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
+static int iort_get_id_mapping_index(struct acpi_iort_node *node,
 					     u32 *index)
 {
 	struct acpi_iort_smmu_v3 *smmu;
@@ -378,20 +378,25 @@ static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
 	if (node->revision < 1)
 		return -EINVAL;
 
-	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
-	/* if any of the gsi for control interrupts is not 0, ignore the MSI */
-	if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
-	    || smmu->sync_gsiv)
-		return -EINVAL;
+	switch (node->type) {
+	case ACPI_IORT_NODE_SMMU_V3:
+		smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
+		/* if any of the gsi for control interrupts is not 0, ignore the MSI */
+		if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
+		    || smmu->sync_gsiv)
+			return -EINVAL;
+
+		if (smmu->id_mapping_index >= node->mapping_count) {
+			pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n",
+			       node, node->type);
+			return -EINVAL;
+		}
 
-	if (smmu->id_mapping_index >= node->mapping_count) {
-		pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n",
-		       node, node->type);
+		*index = smmu->id_mapping_index;
+		return 0;
+	default:
 		return -EINVAL;
 	}
-
-	*index = smmu->id_mapping_index;
-	return 0;
 }
 
 static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
@@ -431,7 +436,7 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 		 *  associated ID map for single mapping cases.
 		 */
 		if (node->type == ACPI_IORT_NODE_SMMU_V3)
-			ret = iort_get_smmu_v3_id_mapping_index(node, &index);
+			ret = iort_get_id_mapping_index(node, &index);
 
 		/* Do the ID translation */
 		for (i = 0; i < node->mapping_count; i++, map++) {
@@ -620,6 +625,49 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
 	return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
 }
 
+static void iort_set_device_domain(struct device *dev,
+				   struct acpi_iort_node *node)
+{
+	struct acpi_iort_its_group *its;
+	struct acpi_iort_node *msi_parent;
+	struct acpi_iort_id_mapping *map;
+	struct fwnode_handle *iort_fwnode;
+	struct irq_domain *domain;
+	int ret, index;
+
+	ret = iort_get_id_mapping_index(node, &index);
+	if (ret < 0)
+		return;
+
+	map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
+			   node->mapping_offset + index * sizeof(*map));
+
+	/* Firmware bug! */
+	if (!map->output_reference ||
+			!(map->flags & ACPI_IORT_ID_SINGLE_MAPPING)) {
+		pr_err(FW_BUG "[node %p type %d] Invalid MSI mapping\n",
+		       node, node->type);
+		return;
+	}
+
+	msi_parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
+				  map->output_reference);
+
+	if (!msi_parent || msi_parent->type != ACPI_IORT_NODE_ITS_GROUP)
+		return;
+
+	/* Move to ITS specific data */
+	its = (struct acpi_iort_its_group *)msi_parent->node_data;
+
+	iort_fwnode = iort_find_domain_token(its->identifiers[0]);
+	if (!iort_fwnode)
+		return;
+
+	domain = irq_find_matching_fwnode(iort_fwnode, DOMAIN_BUS_PLATFORM_MSI);
+	if (domain)
+		dev_set_msi_domain(dev, domain);
+}
+
 /**
  * iort_get_platform_device_domain() - Find MSI domain related to a
  * platform device
@@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
 	/* Configure DMA for the page table walker */
 	acpi_dma_configure(&pdev->dev, attr);
 
+	iort_set_device_domain(&pdev->dev, node);
+
 	ret = platform_device_add(pdev);
 	if (ret)
 		goto dma_deconfigure;
-- 
2.10.0

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Hanjun Guo]

Hi Lorenzo,

On 2017/8/16 1:15, Lorenzo Pieralisi wrote:
> On Fri, Aug 11, 2017 at 09:56:31PM +0800, Hanjun Guo wrote:
>> Hi Lorenzo,
>>
>> On 11 August 2017 at 17:33, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>>> On Wed, Aug 09, 2017 at 06:53:37PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Since we have a mapping index for SMMUv3 MSI, we can
>>>> directly use that index to get the map entry, then
>>>> retrieve dev ID and ITS parent to add SMMUv3 MSI
>>>> support.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>   drivers/acpi/arm64/iort.c | 46 ++++++++++++++++++++++++++++++++++++----------
>>>>   1 file changed, 36 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>> index 9439f02..ce03298 100644
>>>> --- a/drivers/acpi/arm64/iort.c
>>>> +++ b/drivers/acpi/arm64/iort.c
>>>> @@ -313,7 +313,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>>>>        /* Single mapping does not care for input id */
>>>>        if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>>>>                if (type == ACPI_IORT_NODE_NAMED_COMPONENT ||
>>>> -                 type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
>>>> +                 type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
>>>> +                 type == ACPI_IORT_NODE_SMMU_V3) {
>>>>                        *rid_out = map->output_base;
>>>>                        return 0;
>>>>                }
>>>> @@ -357,7 +358,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>>>>
>>>>        if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
>>>>                if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
>>>> -                 node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
>>>> +                 node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
>>>> +                 node->type == ACPI_IORT_NODE_SMMU_V3) {
>>>>                        *id_out = map->output_base;
>>>>                        return parent;
>>>>                }
>>>> @@ -549,9 +551,21 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
>>>>        if (!node)
>>>>                return -ENODEV;
>>>>
>>>> -     for (i = 0; i < node->mapping_count; i++) {
>>>> -             if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i))
>>>> +     if (node->type == ACPI_IORT_NODE_SMMU_V3) {
>>>> +             u32 index;
>>>> +
>>>> +             if (iort_get_smmu_v3_id_mapping_index(node, &index))
>>>> +                     return -ENODEV;
>>>> +
>>>> +             if (iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE,
>>>> +                 index))
>>>>                        return 0;
>>>> +     } else {
>>>> +             for (i = 0; i < node->mapping_count; i++) {
>>>> +                     if (iort_node_map_platform_id(node, dev_id,
>>>> +                         IORT_MSI_TYPE, i))
>>>> +                             return 0;
>>>> +             }
>>>>        }
>>>>
>>>>        return -ENODEV;
>>>> @@ -626,20 +640,30 @@ static struct irq_domain *iort_get_platform_device_domain(struct device *dev)
>>>>        struct acpi_iort_node *node, *msi_parent;
>>>>        struct fwnode_handle *iort_fwnode;
>>>>        struct acpi_iort_its_group *its;
>>>> -     int i;
>>>>
>>>>        /* find its associated iort node */
>>>> -     node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
>>>> -                           iort_match_node_callback, dev);
>>>> +     node = iort_find_dev_node(dev);
>>>>        if (!node)
>>>>                return NULL;
>>>>
>>>>        /* then find its msi parent node */
>>>> -     for (i = 0; i < node->mapping_count; i++) {
>>>> +     if (node->type == ACPI_IORT_NODE_SMMU_V3) {
>>>> +             u32 index;
>>>> +
>>>> +             if (iort_get_smmu_v3_id_mapping_index(node, &index))
>>>> +                     return NULL;
>>>> +
>>>>                msi_parent = iort_node_map_platform_id(node, NULL,
>>>> +                                                    IORT_MSI_TYPE, index);
>>>> +     } else {
>>>> +             int i;
>>>> +
>>>> +             for (i = 0; i < node->mapping_count; i++) {
>>>> +                     msi_parent = iort_node_map_platform_id(node, NULL,
>>>>                                                       IORT_MSI_TYPE, i);
>>>> -             if (msi_parent)
>>>> -                     break;
>>>> +                     if (msi_parent)
>>>> +                             break;
>>>> +             }
>>>>        }
>>>>
>>>>        if (!msi_parent)
>>>> @@ -1233,6 +1257,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
>>>>        /* Configure DMA for the page table walker */
>>>>        acpi_dma_configure(&pdev->dev, attr);
>>>>
>>>> +     acpi_configure_pmsi_domain(&pdev->dev);
>>>
>>> I think this is just overkill. There are two separate things to solve
>>> here:
>>>
>>> 1) Make single mappings valid for SMMUv3 (and PMCG); that's fair enough
>>>     and goes with the logic to skip the ITS DeviceID index for "normal"
>>>     mappings, I can live with that
>>> 2) Find the MSI domain for an SMMUv3 (or any other IORT table node); I
>>>     do not think you need all this complexity to do it via
>>>     acpi_configure_pmsi_domain(), it can be done in a easier way with
>>>     an ad-hoc stub (it does not even have to be SMMUv3 specific)
>>>
>>> My worry is that we are peppering the generic IORT mapping code with
>>> node types specific kludges and it is becoming a mess.
>>
>> Agreed.
>>
>>>
>>> I can rework the patch to show you what I have in mind, please let
>>> me know.
>>
>> Please, thank you very much for doing so.
> 
> Here, untested just to get your opinion, please let me know.
> 
> Thanks,
> Lorenzo
> 
> -- >8 --
> Subject: [PATCH 4/4] ACPI/IORT: IORT nodes MSI handling
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>   drivers/acpi/arm64/iort.c | 76 +++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 24678c3..21a0aab 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -366,7 +366,7 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>   	return NULL;
>   }
>   
> -static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
> +static int iort_get_id_mapping_index(struct acpi_iort_node *node,
>   					     u32 *index)
>   {
>   	struct acpi_iort_smmu_v3 *smmu;
> @@ -378,20 +378,25 @@ static int iort_get_smmu_v3_id_mapping_index(struct acpi_iort_node *node,
>   	if (node->revision < 1)
>   		return -EINVAL;
>   
> -	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> -	/* if any of the gsi for control interrupts is not 0, ignore the MSI */
> -	if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
> -	    || smmu->sync_gsiv)
> -		return -EINVAL;
> +	switch (node->type) {
> +	case ACPI_IORT_NODE_SMMU_V3:
> +		smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> +		/* if any of the gsi for control interrupts is not 0, ignore the MSI */
> +		if (smmu->event_gsiv || smmu->pri_gsiv || smmu->gerr_gsiv
> +		    || smmu->sync_gsiv)
> +			return -EINVAL;
> +
> +		if (smmu->id_mapping_index >= node->mapping_count) {
> +			pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n",
> +			       node, node->type);
> +			return -EINVAL;
> +		}
>   
> -	if (smmu->id_mapping_index >= node->mapping_count) {
> -		pr_err(FW_BUG "[node %p type %d] ID mapping index overflows valid mappings\n",
> -		       node, node->type);
> +		*index = smmu->id_mapping_index;
> +		return 0;
> +	default:
>   		return -EINVAL;
>   	}
> -
> -	*index = smmu->id_mapping_index;
> -	return 0;
>   }
>   
>   static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
> @@ -431,7 +436,7 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>   		 *  associated ID map for single mapping cases.
>   		 */
>   		if (node->type == ACPI_IORT_NODE_SMMU_V3)
> -			ret = iort_get_smmu_v3_id_mapping_index(node, &index);
> +			ret = iort_get_id_mapping_index(node, &index);
>   
>   		/* Do the ID translation */
>   		for (i = 0; i < node->mapping_count; i++, map++) {
> @@ -620,6 +625,49 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
>   	return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
>   }
>   
> +static void iort_set_device_domain(struct device *dev,
> +				   struct acpi_iort_node *node)
> +{
> +	struct acpi_iort_its_group *its;
> +	struct acpi_iort_node *msi_parent;
> +	struct acpi_iort_id_mapping *map;
> +	struct fwnode_handle *iort_fwnode;
> +	struct irq_domain *domain;
> +	int ret, index;
> +
> +	ret = iort_get_id_mapping_index(node, &index);
> +	if (ret < 0)
> +		return;
> +
> +	map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
> +			   node->mapping_offset + index * sizeof(*map));
> +
> +	/* Firmware bug! */
> +	if (!map->output_reference ||
> +			!(map->flags & ACPI_IORT_ID_SINGLE_MAPPING)) {
> +		pr_err(FW_BUG "[node %p type %d] Invalid MSI mapping\n",
> +		       node, node->type);
> +		return;
> +	}
> +
> +	msi_parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
> +				  map->output_reference);
> +
> +	if (!msi_parent || msi_parent->type != ACPI_IORT_NODE_ITS_GROUP)
> +		return;
> +
> +	/* Move to ITS specific data */
> +	its = (struct acpi_iort_its_group *)msi_parent->node_data;
> +
> +	iort_fwnode = iort_find_domain_token(its->identifiers[0]);
> +	if (!iort_fwnode)
> +		return;
> +
> +	domain = irq_find_matching_fwnode(iort_fwnode, DOMAIN_BUS_PLATFORM_MSI);
> +	if (domain)
> +		dev_set_msi_domain(dev, domain);
> +}
> +
>   /**
>    * iort_get_platform_device_domain() - Find MSI domain related to a
>    * platform device
> @@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
>   	/* Configure DMA for the page table walker */
>   	acpi_dma_configure(&pdev->dev, attr);
>   
> +	iort_set_device_domain(&pdev->dev, node);

This is fine to me, but I think we still need to retrieve
the output ID as the request id for ITS, which means we
need to do that in iort_pmsi_get_dev_id(), right?

Thanks
Hanjun

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Lorenzo Pieralisi]

On Thu, Aug 17, 2017 at 03:49:37PM +0800, Hanjun Guo wrote:

[...]

> >  /**
> >   * iort_get_platform_device_domain() - Find MSI domain related to a
> >   * platform device
> >@@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
> >  	/* Configure DMA for the page table walker */
> >  	acpi_dma_configure(&pdev->dev, attr);
> >+	iort_set_device_domain(&pdev->dev, node);
> 
> This is fine to me, but I think we still need to retrieve
> the output ID as the request id for ITS, which means we
> need to do that in iort_pmsi_get_dev_id(), right?

Yes, probably we can add code there like:

ret = iort_get_id_mapping_index(&index);
if (!ret)
	iort_node_get_id(node, &id, index);
	...

This requires updating iort_node_get_id() to allow single mappings
for SMMU/PMCG as you flagged up before.

I will think a bit more to see if there is a cleaner way to reshuffle
the mapping API.

Thanks,
Lorenzo

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Hanjun Guo]

Hi Lorenzo,

On 2017/8/17 21:01, Lorenzo Pieralisi wrote:
> On Thu, Aug 17, 2017 at 03:49:37PM +0800, Hanjun Guo wrote:
> 
> [...]
> 
>>>   /**
>>>    * iort_get_platform_device_domain() - Find MSI domain related to a
>>>    * platform device
>>> @@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
>>>   	/* Configure DMA for the page table walker */
>>>   	acpi_dma_configure(&pdev->dev, attr);
>>> +	iort_set_device_domain(&pdev->dev, node);
>>
>> This is fine to me, but I think we still need to retrieve
>> the output ID as the request id for ITS, which means we
>> need to do that in iort_pmsi_get_dev_id(), right?
> 
> Yes, probably we can add code there like:
> 
> ret = iort_get_id_mapping_index(&index);
> if (!ret)
> 	iort_node_get_id(node, &id, index);
> 	...
> 
> This requires updating iort_node_get_id() to allow single mappings
> for SMMU/PMCG as you flagged up before.
> 
> I will think a bit more to see if there is a cleaner way to reshuffle
> the mapping API.

I would like to send a updated version after the merge window to gather
more comments, and at the same time we can work on the cleaner way to
reshuffle the mapping API, what do you thing?

Thanks
Hanjun

[RFC PATCH 4/4] ACPI: IORT: Add SMMUv3 MSI support

[Lorenzo Pieralisi]

On Tue, Sep 12, 2017 at 11:27:13AM +0800, Hanjun Guo wrote:
> Hi Lorenzo,
> 
> On 2017/8/17 21:01, Lorenzo Pieralisi wrote:
> >On Thu, Aug 17, 2017 at 03:49:37PM +0800, Hanjun Guo wrote:
> >
> >[...]
> >
> >>>  /**
> >>>   * iort_get_platform_device_domain() - Find MSI domain related to a
> >>>   * platform device
> >>>@@ -1159,6 +1207,8 @@ static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
> >>>  	/* Configure DMA for the page table walker */
> >>>  	acpi_dma_configure(&pdev->dev, attr);
> >>>+	iort_set_device_domain(&pdev->dev, node);
> >>
> >>This is fine to me, but I think we still need to retrieve
> >>the output ID as the request id for ITS, which means we
> >>need to do that in iort_pmsi_get_dev_id(), right?
> >
> >Yes, probably we can add code there like:
> >
> >ret = iort_get_id_mapping_index(&index);
> >if (!ret)
> >	iort_node_get_id(node, &id, index);
> >	...
> >
> >This requires updating iort_node_get_id() to allow single mappings
> >for SMMU/PMCG as you flagged up before.
> >
> >I will think a bit more to see if there is a cleaner way to reshuffle
> >the mapping API.
> 
> I would like to send a updated version after the merge window to gather
> more comments, and at the same time we can work on the cleaner way to
> reshuffle the mapping API, what do you thing?

I think you should send an updated patch at -rc1 with comments above, we
can rework the API later or I can do it on top of your patches for the
upcoming merge window.

Thanks !
Lorenzo