| Message ID | 1502276017-63108-4-git-send-email-guohanjun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | SMMUv3 MSI support | expand |
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; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-) -- 2.10.0 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.htmldiff --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; }
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; }