* [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
@ 2019-12-30 12:27 Hanjun Guo
2020-01-02 11:18 ` John Garry
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Hanjun Guo @ 2019-12-30 12:27 UTC (permalink / raw)
To: Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki, Pankaj Bansal
Cc: linux-acpi, John Garry, linuxarm, linux-arm-kernel, Hanjun Guo
The IORT spec [0] says Number of IDs = The number of IDs in the range minus
one, it is confusing but it was written down in the first version of the
IORT spec. But the IORT ID mapping function iort_id_map() did something
wrong from the start, which bails out if:
the request ID >= the input base + number of IDs
This is wrong because it ignored the "minus one", and breaks some valid
usecases such as ID mapping to contain single device mapping without
single mapping flag set.
Pankaj Bansal proposed a solution to fix the issue [1], which bails
out if:
the request ID > the input base + number of IDs
This works as the spec defined, unfortunately some firmware didn't
minus one for the number of IDs in the range, and the propoased
solution will break those systems in this way:
PCI hostbridge mapping entry 1:
Input base: 0x1000
ID Count: 0x100
Output base: 0x1000
Output reference: 0xC4 //ITS reference
PCI hostbridge mapping entry 2:
Input base: 0x1100
ID Count: 0x100
Output base: 0x2000
Output reference: 0xD4 //ITS reference
Two mapping entries which the second entry's Input base = the first
entry's Input base + ID count, so for requester ID 0x1100 will map
to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
So introduce a workaround to match the IORT's OEM information for
the broken firmware, also update the logic of the ID mapping for
firmwares report the number of IDs as the IORT spec defined, to
make the code compatible for both kinds of system.
I checked the ACPI tables in the tianocore/edk2-platforms [2], only
HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
info table, if we break other platforms, we can add that later.
[0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
[1]: https://patchwork.kernel.org/patch/11292823/
[2]: https://github.com/tianocore/edk2-platforms
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
---
RFC->v1:
- Print warning when matched the workaround info, suggested by Pankaj.
drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 52 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 33f7198..60eb10d 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
return status;
}
+struct iort_workaround_oem_info {
+ char oem_id[ACPI_OEM_ID_SIZE + 1];
+ char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+ u32 oem_revision;
+};
+
+static bool apply_id_count_workaround;
+
+static struct iort_workaround_oem_info wa_info[] __initdata = {
+ {
+ .oem_id = "HISI ",
+ .oem_table_id = "HIP07 ",
+ .oem_revision = 0,
+ }, {
+ .oem_id = "HISI ",
+ .oem_table_id = "HIP08 ",
+ .oem_revision = 0,
+ }
+};
+
+static void __init
+iort_check_id_count_workaround(struct acpi_table_header *tbl)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
+ if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
+ !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+ wa_info[i].oem_revision == tbl->oem_revision) {
+ apply_id_count_workaround = true;
+ pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
+ break;
+ }
+ }
+}
+
static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
u32 *rid_out)
{
@@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
return -ENXIO;
}
- if (rid_in < map->input_base ||
- (rid_in >= map->input_base + map->id_count))
- return -ENXIO;
+ /*
+ * IORT spec says Number of IDs = The number of IDs in the range minus
+ * one, but the IORT code ingored the "minus one", and some firmware
+ * did that too, so apply a workaround here to keep compatible with
+ * both new and old versions of the firmware.
+ */
+ if (apply_id_count_workaround) {
+ if (rid_in < map->input_base ||
+ (rid_in >= map->input_base + map->id_count))
+ return -ENXIO;
+ } else {
+ if (rid_in < map->input_base ||
+ (rid_in > map->input_base + map->id_count))
+ return -ENXIO;
+ }
*rid_out = map->output_base + (rid_in - map->input_base);
return 0;
@@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void)
return;
}
+ iort_check_id_count_workaround(iort_table);
iort_init_platform_devices();
}
--
1.7.12.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2019-12-30 12:27 [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue Hanjun Guo
@ 2020-01-02 11:18 ` John Garry
2020-01-03 10:20 ` Hanjun Guo
2020-01-06 17:19 ` Robin Murphy
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: John Garry @ 2020-01-02 11:18 UTC (permalink / raw)
To: Guohanjun (Hanjun Guo),
Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki,
Pankaj Bansal
Cc: linux-acpi, Robert Richter, Linuxarm, linux-arm-kernel, Shameer Kolothum
+
On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote:
> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
> one, it is confusing but it was written down in the first version of the
> IORT spec. But the IORT ID mapping function iort_id_map() did something
> wrong from the start, which bails out if:
>
> the request ID >= the input base + number of IDs
>
> This is wrong because it ignored the "minus one", and breaks some valid
> usecases such as ID mapping to contain single device mapping without
> single mapping flag set.
>
> Pankaj Bansal proposed a solution to fix the issue [1], which bails
> out if:
>
> the request ID > the input base + number of IDs
>
> This works as the spec defined, unfortunately some firmware didn't
> minus one for the number of IDs in the range, and the propoased
> solution will break those systems in this way:
>
> PCI hostbridge mapping entry 1:
> Input base: 0x1000
> ID Count: 0x100
> Output base: 0x1000
> Output reference: 0xC4 //ITS reference
>
> PCI hostbridge mapping entry 2:
> Input base: 0x1100
> ID Count: 0x100
> Output base: 0x2000
> Output reference: 0xD4 //ITS reference
>
> Two mapping entries which the second entry's Input base = the first
> entry's Input base + ID count, so for requester ID 0x1100 will map
> to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
>
> So introduce a workaround to match the IORT's OEM information for
> the broken firmware, also update the logic of the ID mapping for
> firmwares report the number of IDs as the IORT spec defined, to
> make the code compatible for both kinds of system.
>
> I checked the ACPI tables in the tianocore/edk2-platforms [2],
Hi Hanjun,
only
> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
> info table,
Are you asserting that other platforms are ok on the basis that NumIds =
large power of 2 - 1, e.g. 0xffff? Is this strictly proper?
if we break other platforms, we can add that later.
>
I think that it would be better to audit others now as well as best as
reasonably possible. There is somewhat limited coverage in [2].
Thanks,
John
> [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
> [1]: https://patchwork.kernel.org/patch/11292823/
> [2]: https://github.com/tianocore/edk2-platforms
>
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
>
> RFC->v1:
> - Print warning when matched the workaround info, suggested by Pankaj.
>
> drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 33f7198..60eb10d 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
> return status;
> }
>
> +struct iort_workaround_oem_info {
> + char oem_id[ACPI_OEM_ID_SIZE + 1];
> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> + u32 oem_revision;
> +};
> +
> +static bool apply_id_count_workaround;
> +
> +static struct iort_workaround_oem_info wa_info[] __initdata = {
> + {
> + .oem_id = "HISI ",
> + .oem_table_id = "HIP07 ",
> + .oem_revision = 0,
> + }, {
> + .oem_id = "HISI ",
> + .oem_table_id = "HIP08 ",
> + .oem_revision = 0,
> + }
> +};
> +
> +static void __init
> +iort_check_id_count_workaround(struct acpi_table_header *tbl)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> + wa_info[i].oem_revision == tbl->oem_revision) {
> + apply_id_count_workaround = true;
> + pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
> + break;
> + }
> + }
> +}
> +
> static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> u32 *rid_out)
> {
> @@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> return -ENXIO;
> }
>
> - if (rid_in < map->input_base ||
> - (rid_in >= map->input_base + map->id_count))
> - return -ENXIO;
> + /*
> + * IORT spec says Number of IDs = The number of IDs in the range minus
> + * one, but the IORT code ingored the "minus one", and some firmware
> + * did that too, so apply a workaround here to keep compatible with
> + * both new and old versions of the firmware.
> + */
> + if (apply_id_count_workaround) {
> + if (rid_in < map->input_base ||
> + (rid_in >= map->input_base + map->id_count))
> + return -ENXIO;
> + } else {
> + if (rid_in < map->input_base ||
> + (rid_in > map->input_base + map->id_count))
> + return -ENXIO;
> + }
>
> *rid_out = map->output_base + (rid_in - map->input_base);
> return 0;
> @@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void)
> return;
> }
>
> + iort_check_id_count_workaround(iort_table);
> iort_init_platform_devices();
> }
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-02 11:18 ` John Garry
@ 2020-01-03 10:20 ` Hanjun Guo
0 siblings, 0 replies; 14+ messages in thread
From: Hanjun Guo @ 2020-01-03 10:20 UTC (permalink / raw)
To: John Garry, Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki,
Pankaj Bansal
Cc: linux-acpi, Robert Richter, Linuxarm, linux-arm-kernel, Shameer Kolothum
On 2020/1/2 19:18, John Garry wrote:
> +
>
> On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote:
>> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
>> one, it is confusing but it was written down in the first version of the
>> IORT spec. But the IORT ID mapping function iort_id_map() did something
>> wrong from the start, which bails out if:
>>
>> the request ID >= the input base + number of IDs
>>
>> This is wrong because it ignored the "minus one", and breaks some valid
>> usecases such as ID mapping to contain single device mapping without
>> single mapping flag set.
>>
>> Pankaj Bansal proposed a solution to fix the issue [1], which bails
>> out if:
>>
>> the request ID > the input base + number of IDs
>>
>> This works as the spec defined, unfortunately some firmware didn't
>> minus one for the number of IDs in the range, and the propoased
>> solution will break those systems in this way:
>>
>> PCI hostbridge mapping entry 1:
>> Input base: 0x1000
>> ID Count: 0x100
>> Output base: 0x1000
>> Output reference: 0xC4 //ITS reference
>>
>> PCI hostbridge mapping entry 2:
>> Input base: 0x1100
>> ID Count: 0x100
>> Output base: 0x2000
>> Output reference: 0xD4 //ITS reference
>>
>> Two mapping entries which the second entry's Input base = the first
>> entry's Input base + ID count, so for requester ID 0x1100 will map
>> to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
>>
>> So introduce a workaround to match the IORT's OEM information for
>> the broken firmware, also update the logic of the ID mapping for
>> firmwares report the number of IDs as the IORT spec defined, to
>> make the code compatible for both kinds of system.
>>
>> I checked the ACPI tables in the tianocore/edk2-platforms [2],
>
> Hi Hanjun,
>
> only
>> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
>> info table,
>
> Are you asserting that other platforms are ok on the basis that NumIds = large power of 2 - 1, e.g. 0xffff? Is this strictly proper?
No, some platforms with no opensource ACPI tables, are
not covered.
>
> if we break other platforms, we can add that later.
>>
>
> I think that it would be better to audit others now as well as best as reasonably possible. There is somewhat limited coverage in [2].
I will Cc people form Mavell, Ampere, and Ard who is know Socionext very well,
that's the best I can do.
Thanks
Hanjun
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2019-12-30 12:27 [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue Hanjun Guo
2020-01-02 11:18 ` John Garry
@ 2020-01-06 17:19 ` Robin Murphy
2020-01-07 12:03 ` Hanjun Guo
2020-01-09 16:02 ` Lorenzo Pieralisi
2020-01-10 12:11 ` Lorenzo Pieralisi
2020-01-13 9:34 ` John Garry
3 siblings, 2 replies; 14+ messages in thread
From: Robin Murphy @ 2020-01-06 17:19 UTC (permalink / raw)
To: Hanjun Guo, Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki,
Pankaj Bansal
Cc: linux-acpi, John Garry, linuxarm, linux-arm-kernel
On 30/12/2019 12:27 pm, Hanjun Guo wrote:
> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
> one, it is confusing but it was written down in the first version of the
> IORT spec. But the IORT ID mapping function iort_id_map() did something
> wrong from the start, which bails out if:
>
> the request ID >= the input base + number of IDs
>
> This is wrong because it ignored the "minus one", and breaks some valid
> usecases such as ID mapping to contain single device mapping without
> single mapping flag set.
>
> Pankaj Bansal proposed a solution to fix the issue [1], which bails
> out if:
>
> the request ID > the input base + number of IDs
>
> This works as the spec defined, unfortunately some firmware didn't
> minus one for the number of IDs in the range, and the propoased
> solution will break those systems in this way:
>
> PCI hostbridge mapping entry 1:
> Input base: 0x1000
> ID Count: 0x100
> Output base: 0x1000
> Output reference: 0xC4 //ITS reference
>
> PCI hostbridge mapping entry 2:
> Input base: 0x1100
> ID Count: 0x100
> Output base: 0x2000
> Output reference: 0xD4 //ITS reference
>
> Two mapping entries which the second entry's Input base = the first
> entry's Input base + ID count, so for requester ID 0x1100 will map
> to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
>
> So introduce a workaround to match the IORT's OEM information for
> the broken firmware, also update the logic of the ID mapping for
> firmwares report the number of IDs as the IORT spec defined, to
> make the code compatible for both kinds of system.
>
> I checked the ACPI tables in the tianocore/edk2-platforms [2], only
> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
> info table, if we break other platforms, we can add that later.
>
> [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
> [1]: https://patchwork.kernel.org/patch/11292823/
> [2]: https://github.com/tianocore/edk2-platforms
>
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
>
> RFC->v1:
> - Print warning when matched the workaround info, suggested by Pankaj.
>
> drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 33f7198..60eb10d 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
> return status;
> }
>
> +struct iort_workaround_oem_info {
> + char oem_id[ACPI_OEM_ID_SIZE + 1];
> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> + u32 oem_revision;
> +};
> +
> +static bool apply_id_count_workaround;
> +
> +static struct iort_workaround_oem_info wa_info[] __initdata = {
> + {
> + .oem_id = "HISI ",
> + .oem_table_id = "HIP07 ",
> + .oem_revision = 0,
> + }, {
> + .oem_id = "HISI ",
> + .oem_table_id = "HIP08 ",
> + .oem_revision = 0,
> + }
> +};
> +
> +static void __init
> +iort_check_id_count_workaround(struct acpi_table_header *tbl)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> + wa_info[i].oem_revision == tbl->oem_revision) {
> + apply_id_count_workaround = true;
> + pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
> + break;
> + }
> + }
> +}
> +
> static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> u32 *rid_out)
> {
> @@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> return -ENXIO;
> }
>
> - if (rid_in < map->input_base ||
> - (rid_in >= map->input_base + map->id_count))
> - return -ENXIO;
> + /*
> + * IORT spec says Number of IDs = The number of IDs in the range minus
> + * one, but the IORT code ingored the "minus one", and some firmware
> + * did that too, so apply a workaround here to keep compatible with
> + * both new and old versions of the firmware.
> + */
> + if (apply_id_count_workaround) {
> + if (rid_in < map->input_base ||
> + (rid_in >= map->input_base + map->id_count))
> + return -ENXIO;
> + } else {
> + if (rid_in < map->input_base ||
> + (rid_in > map->input_base + map->id_count))
> + return -ENXIO;
> + }
This seems needlessly repetitive and convoluted... how about refactoring
to something like:
map_max = map->input_base + map->id_count;
if (apply_id_count_workaround)
map_max--;
?
Robin.
> *rid_out = map->output_base + (rid_in - map->input_base);
> return 0;
> @@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void)
> return;
> }
>
> + iort_check_id_count_workaround(iort_table);
> iort_init_platform_devices();
> }
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-06 17:19 ` Robin Murphy
@ 2020-01-07 12:03 ` Hanjun Guo
2020-01-09 16:02 ` Lorenzo Pieralisi
1 sibling, 0 replies; 14+ messages in thread
From: Hanjun Guo @ 2020-01-07 12:03 UTC (permalink / raw)
To: Robin Murphy, Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki,
Pankaj Bansal
Cc: linux-acpi, John Garry, linuxarm, linux-arm-kernel
On 2020/1/7 1:19, Robin Murphy wrote:
> On 30/12/2019 12:27 pm, Hanjun Guo wrote:
>> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
>> one, it is confusing but it was written down in the first version of the
>> IORT spec. But the IORT ID mapping function iort_id_map() did something
>> wrong from the start, which bails out if:
>>
>> the request ID >= the input base + number of IDs
>>
>> This is wrong because it ignored the "minus one", and breaks some valid
>> usecases such as ID mapping to contain single device mapping without
>> single mapping flag set.
>>
>> Pankaj Bansal proposed a solution to fix the issue [1], which bails
>> out if:
>>
>> the request ID > the input base + number of IDs
>>
>> This works as the spec defined, unfortunately some firmware didn't
>> minus one for the number of IDs in the range, and the propoased
>> solution will break those systems in this way:
>>
>> PCI hostbridge mapping entry 1:
>> Input base: 0x1000
>> ID Count: 0x100
>> Output base: 0x1000
>> Output reference: 0xC4 //ITS reference
>>
>> PCI hostbridge mapping entry 2:
>> Input base: 0x1100
>> ID Count: 0x100
>> Output base: 0x2000
>> Output reference: 0xD4 //ITS reference
>>
>> Two mapping entries which the second entry's Input base = the first
>> entry's Input base + ID count, so for requester ID 0x1100 will map
>> to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
>>
>> So introduce a workaround to match the IORT's OEM information for
>> the broken firmware, also update the logic of the ID mapping for
>> firmwares report the number of IDs as the IORT spec defined, to
>> make the code compatible for both kinds of system.
>>
>> I checked the ACPI tables in the tianocore/edk2-platforms [2], only
>> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
>> info table, if we break other platforms, we can add that later.
>>
>> [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
>> [1]: https://patchwork.kernel.org/patch/11292823/
>> [2]: https://github.com/tianocore/edk2-platforms
>>
>> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>> ---
>>
>> RFC->v1:
>> - Print warning when matched the workaround info, suggested by Pankaj.
>>
>> drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 52 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 33f7198..60eb10d 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>> return status;
>> }
>> +struct iort_workaround_oem_info {
>> + char oem_id[ACPI_OEM_ID_SIZE + 1];
>> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>> + u32 oem_revision;
>> +};
>> +
>> +static bool apply_id_count_workaround;
>> +
>> +static struct iort_workaround_oem_info wa_info[] __initdata = {
>> + {
>> + .oem_id = "HISI ",
>> + .oem_table_id = "HIP07 ",
>> + .oem_revision = 0,
>> + }, {
>> + .oem_id = "HISI ",
>> + .oem_table_id = "HIP08 ",
>> + .oem_revision = 0,
>> + }
>> +};
>> +
>> +static void __init
>> +iort_check_id_count_workaround(struct acpi_table_header *tbl)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
>> + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
>> + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
>> + wa_info[i].oem_revision == tbl->oem_revision) {
>> + apply_id_count_workaround = true;
>> + pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
>> + break;
>> + }
>> + }
>> +}
>> +
>> static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>> u32 *rid_out)
>> {
>> @@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>> return -ENXIO;
>> }
>> - if (rid_in < map->input_base ||
>> - (rid_in >= map->input_base + map->id_count))
>> - return -ENXIO;
>> + /*
>> + * IORT spec says Number of IDs = The number of IDs in the range minus
>> + * one, but the IORT code ingored the "minus one", and some firmware
>> + * did that too, so apply a workaround here to keep compatible with
>> + * both new and old versions of the firmware.
>> + */
>> + if (apply_id_count_workaround) {
>> + if (rid_in < map->input_base ||
>> + (rid_in >= map->input_base + map->id_count))
>> + return -ENXIO;
>> + } else {
>> + if (rid_in < map->input_base ||
>> + (rid_in > map->input_base + map->id_count))
>> + return -ENXIO;
>> + }
>
> This seems needlessly repetitive and convoluted... how about refactoring to something like:
>
> map_max = map->input_base + map->id_count;
> if (apply_id_count_workaround)
> map_max--;
Much better, thanks! I will update my patch.
Thanks
Hanjun
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-06 17:19 ` Robin Murphy
2020-01-07 12:03 ` Hanjun Guo
@ 2020-01-09 16:02 ` Lorenzo Pieralisi
2020-01-10 6:22 ` Hanjun Guo
1 sibling, 1 reply; 14+ messages in thread
From: Lorenzo Pieralisi @ 2020-01-09 16:02 UTC (permalink / raw)
To: Robin Murphy
Cc: Rafael J. Wysocki, Pankaj Bansal, John Garry, Hanjun Guo,
linuxarm, linux-acpi, Sudeep Holla, linux-arm-kernel
On Mon, Jan 06, 2020 at 05:19:32PM +0000, Robin Murphy wrote:
> On 30/12/2019 12:27 pm, Hanjun Guo wrote:
> > The IORT spec [0] says Number of IDs = The number of IDs in the range minus
> > one, it is confusing but it was written down in the first version of the
Why is it confusing ? Because we botched the kernel code :) ?
> > IORT spec. But the IORT ID mapping function iort_id_map() did something
> > wrong from the start, which bails out if:
> >
> > the request ID >= the input base + number of IDs
> >
> > This is wrong because it ignored the "minus one", and breaks some valid
> > usecases such as ID mapping to contain single device mapping without
> > single mapping flag set.
> >
> > Pankaj Bansal proposed a solution to fix the issue [1], which bails
> > out if:
> >
> > the request ID > the input base + number of IDs
Add a Link: tag, when I read a commit log I want to have a reference
to the patches relevant to the commit in question (which in turn
will help understand what Pankaj suggested).
> > This works as the spec defined, unfortunately some firmware didn't
> > minus one for the number of IDs in the range, and the propoased
> > solution will break those systems in this way:
> >
> > PCI hostbridge mapping entry 1:
> > Input base: 0x1000
> > ID Count: 0x100
> > Output base: 0x1000
> > Output reference: 0xC4 //ITS reference
> >
> > PCI hostbridge mapping entry 2:
> > Input base: 0x1100
> > ID Count: 0x100
> > Output base: 0x2000
> > Output reference: 0xD4 //ITS reference
> >
> > Two mapping entries which the second entry's Input base = the first
> > entry's Input base + ID count, so for requester ID 0x1100 will map
> > to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
> >
> > So introduce a workaround to match the IORT's OEM information for
> > the broken firmware, also update the logic of the ID mapping for
> > firmwares report the number of IDs as the IORT spec defined, to
> > make the code compatible for both kinds of system.
> >
> > I checked the ACPI tables in the tianocore/edk2-platforms [2], only
> > HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
> > info table, if we break other platforms, we can add that later.
> >
> > [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
> > [1]: https://patchwork.kernel.org/patch/11292823/
Add a Link: tag to a message-ID
> > [2]: https://github.com/tianocore/edk2-platforms
It is useless in a commit log - this is a moving target.
I can rewrite this commit log if you think it is faster.
> >
> > Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> > ---
> >
> > RFC->v1:
> > - Print warning when matched the workaround info, suggested by Pankaj.
> >
> > drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 33f7198..60eb10d 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
> > return status;
> > }
> > +struct iort_workaround_oem_info {
> > + char oem_id[ACPI_OEM_ID_SIZE + 1];
> > + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> > + u32 oem_revision;
> > +};
> > +
> > +static bool apply_id_count_workaround;
> > +
> > +static struct iort_workaround_oem_info wa_info[] __initdata = {
> > + {
> > + .oem_id = "HISI ",
> > + .oem_table_id = "HIP07 ",
> > + .oem_revision = 0,
> > + }, {
> > + .oem_id = "HISI ",
> > + .oem_table_id = "HIP08 ",
> > + .oem_revision = 0,
> > + }
> > +};
> > +
> > +static void __init
> > +iort_check_id_count_workaround(struct acpi_table_header *tbl)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> > + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> > + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> > + wa_info[i].oem_revision == tbl->oem_revision) {
> > + apply_id_count_workaround = true;
> > + pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
> > + break;
> > + }
> > + }
> > +}
> > +
> > static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> > u32 *rid_out)
> > {
> > @@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> > return -ENXIO;
> > }
> > - if (rid_in < map->input_base ||
> > - (rid_in >= map->input_base + map->id_count))
> > - return -ENXIO;
> > + /*
> > + * IORT spec says Number of IDs = The number of IDs in the range minus
Section, page, table number please, "IORT spec says" is too vague.
> > + * one, but the IORT code ingored the "minus one", and some firmware
s/ingored/ignored/
> > + * did that too, so apply a workaround here to keep compatible with
> > + * both new and old versions of the firmware.
It is not "new" vs "old" it is spec compliant vs non-spec compliant.
> > + */
> > + if (apply_id_count_workaround) {
> > + if (rid_in < map->input_base ||
> > + (rid_in >= map->input_base + map->id_count))
> > + return -ENXIO;
> > + } else {
> > + if (rid_in < map->input_base ||
> > + (rid_in > map->input_base + map->id_count))
> > + return -ENXIO;
> > + }
>
> This seems needlessly repetitive and convoluted... how about refactoring to
> something like:
+1
>
> map_max = map->input_base + map->id_count;
> if (apply_id_count_workaround)
> map_max--;
You can even turn it into an inline function (ie iort_get_map_max())
with the comment above in it so that the quirk is isolated instead
of having it in the middle of iort_id_map().
I am fine either way. We need test coverage since I feel this may
break a number of systems (ie I don't think it should be merged as
a fix).
Lorenzo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-09 16:02 ` Lorenzo Pieralisi
@ 2020-01-10 6:22 ` Hanjun Guo
2020-01-10 10:39 ` Lorenzo Pieralisi
2020-01-10 10:51 ` Robin Murphy
0 siblings, 2 replies; 14+ messages in thread
From: Hanjun Guo @ 2020-01-10 6:22 UTC (permalink / raw)
To: Lorenzo Pieralisi, Robin Murphy
Cc: Rafael J. Wysocki, Pankaj Bansal, John Garry, linuxarm,
linux-acpi, Sudeep Holla, linux-arm-kernel
On 2020/1/10 0:02, Lorenzo Pieralisi wrote:
> On Mon, Jan 06, 2020 at 05:19:32PM +0000, Robin Murphy wrote:
>> On 30/12/2019 12:27 pm, Hanjun Guo wrote:
>>> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
>>> one, it is confusing but it was written down in the first version of the
>
> Why is it confusing ? Because we botched the kernel code :) ?
I think 'minus one' is not bringing any benefit :)
>
>>> IORT spec. But the IORT ID mapping function iort_id_map() did something
>>> wrong from the start, which bails out if:
>>>
>>> the request ID >= the input base + number of IDs
>>>
>>> This is wrong because it ignored the "minus one", and breaks some valid
>>> usecases such as ID mapping to contain single device mapping without
>>> single mapping flag set.
>>>
>>> Pankaj Bansal proposed a solution to fix the issue [1], which bails
>>> out if:
>>>
>>> the request ID > the input base + number of IDs
>
> Add a Link: tag, when I read a commit log I want to have a reference
> to the patches relevant to the commit in question (which in turn
> will help understand what Pankaj suggested).
>
>>> This works as the spec defined, unfortunately some firmware didn't
>>> minus one for the number of IDs in the range, and the propoased
>>> solution will break those systems in this way:
>>>
>>> PCI hostbridge mapping entry 1:
>>> Input base: 0x1000
>>> ID Count: 0x100
>>> Output base: 0x1000
>>> Output reference: 0xC4 //ITS reference
>>>
>>> PCI hostbridge mapping entry 2:
>>> Input base: 0x1100
>>> ID Count: 0x100
>>> Output base: 0x2000
>>> Output reference: 0xD4 //ITS reference
>>>
>>> Two mapping entries which the second entry's Input base = the first
>>> entry's Input base + ID count, so for requester ID 0x1100 will map
>>> to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
>>>
>>> So introduce a workaround to match the IORT's OEM information for
>>> the broken firmware, also update the logic of the ID mapping for
>>> firmwares report the number of IDs as the IORT spec defined, to
>>> make the code compatible for both kinds of system.
>>>
>>> I checked the ACPI tables in the tianocore/edk2-platforms [2], only
>>> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
>>> info table, if we break other platforms, we can add that later.
>>>
>>> [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
>>> [1]: https://patchwork.kernel.org/patch/11292823/
>
> Add a Link: tag to a message-ID
>
>>> [2]: https://github.com/tianocore/edk2-platforms
>
> It is useless in a commit log - this is a moving target.
>
> I can rewrite this commit log if you think it is faster.
That will be very helpful, please do so, thanks!
>
>>>
>>> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>>> ---
>>>
>>> RFC->v1:
>>> - Print warning when matched the workaround info, suggested by Pankaj.
>>>
>>> drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 52 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>> index 33f7198..60eb10d 100644
>>> --- a/drivers/acpi/arm64/iort.c
>>> +++ b/drivers/acpi/arm64/iort.c
>>> @@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>>> return status;
>>> }
>>> +struct iort_workaround_oem_info {
>>> + char oem_id[ACPI_OEM_ID_SIZE + 1];
>>> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>>> + u32 oem_revision;
>>> +};
>>> +
>>> +static bool apply_id_count_workaround;
>>> +
>>> +static struct iort_workaround_oem_info wa_info[] __initdata = {
>>> + {
>>> + .oem_id = "HISI ",
>>> + .oem_table_id = "HIP07 ",
>>> + .oem_revision = 0,
>>> + }, {
>>> + .oem_id = "HISI ",
>>> + .oem_table_id = "HIP08 ",
>>> + .oem_revision = 0,
>>> + }
>>> +};
>>> +
>>> +static void __init
>>> +iort_check_id_count_workaround(struct acpi_table_header *tbl)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
>>> + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
>>> + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
>>> + wa_info[i].oem_revision == tbl->oem_revision) {
>>> + apply_id_count_workaround = true;
>>> + pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
>>> + break;
>>> + }
>>> + }
>>> +}
>>> +
>>> static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>>> u32 *rid_out)
>>> {
>>> @@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>>> return -ENXIO;
>>> }
>>> - if (rid_in < map->input_base ||
>>> - (rid_in >= map->input_base + map->id_count))
>>> - return -ENXIO;
>>> + /*
>>> + * IORT spec says Number of IDs = The number of IDs in the range minus
>
> Section, page, table number please, "IORT spec says" is too vague.
>
>>> + * one, but the IORT code ingored the "minus one", and some firmware
>
> s/ingored/ignored/
>
>>> + * did that too, so apply a workaround here to keep compatible with
>>> + * both new and old versions of the firmware.
>
> It is not "new" vs "old" it is spec compliant vs non-spec compliant.
Agreed.
>
>>> + */
>>> + if (apply_id_count_workaround) {
>>> + if (rid_in < map->input_base ||
>>> + (rid_in >= map->input_base + map->id_count))
>>> + return -ENXIO;
>>> + } else {
>>> + if (rid_in < map->input_base ||
>>> + (rid_in > map->input_base + map->id_count))
>>> + return -ENXIO;
>>> + }
>>
>> This seems needlessly repetitive and convoluted... how about refactoring to
>> something like:
>
> +1
>
>>
>> map_max = map->input_base + map->id_count;
>> if (apply_id_count_workaround)
>> map_max--;
>
> You can even turn it into an inline function (ie iort_get_map_max())
> with the comment above in it so that the quirk is isolated instead
> of having it in the middle of iort_id_map().
I vote for this one, it's self-contained.
>
> I am fine either way. We need test coverage since I feel this may
> break a number of systems (ie I don't think it should be merged as
> a fix).
Will you resend this patch with commit log and the updated code? or
let me do that? Both are ok to me, let's get it tested for longer time
if we merge it ASAP.
Thanks
Hanjun
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-10 6:22 ` Hanjun Guo
@ 2020-01-10 10:39 ` Lorenzo Pieralisi
2020-01-10 10:51 ` Robin Murphy
1 sibling, 0 replies; 14+ messages in thread
From: Lorenzo Pieralisi @ 2020-01-10 10:39 UTC (permalink / raw)
To: Hanjun Guo
Cc: Rafael J. Wysocki, Pankaj Bansal, John Garry, linuxarm,
linux-acpi, Sudeep Holla, Robin Murphy, linux-arm-kernel
On Fri, Jan 10, 2020 at 02:22:22PM +0800, Hanjun Guo wrote:
[...]
> > I am fine either way. We need test coverage since I feel this may
> > break a number of systems (ie I don't think it should be merged as
> > a fix).
>
> Will you resend this patch with commit log and the updated code? or
> let me do that? Both are ok to me, let's get it tested for longer time
> if we merge it ASAP.
I will write the commit log in reply to the original patch, please
update the code and repost the whole thing (it is better you
repost it to avoid confusing Catalin and Will).
This technically is not a fix; we can try to get it into v5.6 but
I am a bit nervous since it can break other platforms, we have to
fix it though so better sooner than later.
Thanks,
Lorenzo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-10 6:22 ` Hanjun Guo
2020-01-10 10:39 ` Lorenzo Pieralisi
@ 2020-01-10 10:51 ` Robin Murphy
1 sibling, 0 replies; 14+ messages in thread
From: Robin Murphy @ 2020-01-10 10:51 UTC (permalink / raw)
To: Hanjun Guo, Lorenzo Pieralisi
Cc: Rafael J. Wysocki, Pankaj Bansal, John Garry, linuxarm,
linux-acpi, Sudeep Holla, linux-arm-kernel
On 2020-01-10 6:22 am, Hanjun Guo wrote:
> On 2020/1/10 0:02, Lorenzo Pieralisi wrote:
>> On Mon, Jan 06, 2020 at 05:19:32PM +0000, Robin Murphy wrote:
>>> On 30/12/2019 12:27 pm, Hanjun Guo wrote:
>>>> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
>>>> one, it is confusing but it was written down in the first version of the
>>
>> Why is it confusing ? Because we botched the kernel code :) ?
>
> I think 'minus one' is not bringing any benefit :)
Well, in order to describe a 1:1 mapping of the entire possible ID
space, the alternative would have to be to overload the
otherwise-nonsensical value of 0 to mean 2^32, which I would argue is an
even more non-obvious inconsistency. Encoding strictly positive values
as 'value - 1' is a relatively common thing (at least in hardware design).
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2019-12-30 12:27 [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue Hanjun Guo
2020-01-02 11:18 ` John Garry
2020-01-06 17:19 ` Robin Murphy
@ 2020-01-10 12:11 ` Lorenzo Pieralisi
2020-01-13 7:04 ` Hanjun Guo
2020-01-13 9:34 ` John Garry
3 siblings, 1 reply; 14+ messages in thread
From: Lorenzo Pieralisi @ 2020-01-10 12:11 UTC (permalink / raw)
To: Hanjun Guo
Cc: Rafael J. Wysocki, Pankaj Bansal, John Garry, linuxarm,
linux-acpi, Sudeep Holla, linux-arm-kernel
On Mon, Dec 30, 2019 at 08:27:04PM +0800, Hanjun Guo wrote:
> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
> one, it is confusing but it was written down in the first version of the
> IORT spec. But the IORT ID mapping function iort_id_map() did something
> wrong from the start, which bails out if:
>
> the request ID >= the input base + number of IDs
>
> This is wrong because it ignored the "minus one", and breaks some valid
> usecases such as ID mapping to contain single device mapping without
> single mapping flag set.
>
> Pankaj Bansal proposed a solution to fix the issue [1], which bails
> out if:
>
> the request ID > the input base + number of IDs
>
> This works as the spec defined, unfortunately some firmware didn't
> minus one for the number of IDs in the range, and the propoased
> solution will break those systems in this way:
>
> PCI hostbridge mapping entry 1:
> Input base: 0x1000
> ID Count: 0x100
> Output base: 0x1000
> Output reference: 0xC4 //ITS reference
>
> PCI hostbridge mapping entry 2:
> Input base: 0x1100
> ID Count: 0x100
> Output base: 0x2000
> Output reference: 0xD4 //ITS reference
>
> Two mapping entries which the second entry's Input base = the first
> entry's Input base + ID count, so for requester ID 0x1100 will map
> to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
>
> So introduce a workaround to match the IORT's OEM information for
> the broken firmware, also update the logic of the ID mapping for
> firmwares report the number of IDs as the IORT spec defined, to
> make the code compatible for both kinds of system.
>
> I checked the ACPI tables in the tianocore/edk2-platforms [2], only
> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
> info table, if we break other platforms, we can add that later.
>
> [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
> [1]: https://patchwork.kernel.org/patch/11292823/
> [2]: https://github.com/tianocore/edk2-platforms
>
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
Commit log rewritten below - please update the code (and check the
log) as per this thread discussion.
Thanks,
Lorenzo
-- >8 --
From bc766b2913008519bdb59bbc38907451e8bac0d4 Mon Sep 17 00:00:00 2001
From: Hanjun Guo <guohanjun@huawei.com>
Date: Mon, 30 Dec 2019 20:27:04 +0800
Subject: [PATCH] ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()
The IORT specification [0] (Section 3, table 4, page 9) defines the
'Number of IDs' as 'The number of IDs in the range minus one'.
However, the IORT ID mapping function iort_id_map() treats the 'Number
of IDs' field as if it were the full IDs mapping count, with the
following check in place to detect out of boundary input IDs:
InputID >= Input base + Number of IDs
This check is flawed in that it considers the 'Number of IDs' field as
the full number of IDs mapping and disregards the 'minus one' from
the IDs count.
The correct check in iort_id_map() should be implemented as:
InputID > Input base + Number of IDs
this implements the specification correctly but unfortunately it breaks
existing firmwares that erroneously set the 'Number of IDs' as the full
IDs mapping count rather than IDs mapping count minus one.
e.g.
PCI hostbridge mapping entry 1:
Input base: 0x1000
ID Count: 0x100
Output base: 0x1000
Output reference: 0xC4 //ITS reference
PCI hostbridge mapping entry 2:
Input base: 0x1100
ID Count: 0x100
Output base: 0x2000
Output reference: 0xD4 //ITS reference
Two mapping entries which the second entry's Input base = the first
entry's Input base + ID count, so for InputID 0x1100 and with the
correct InputID check in place in iort_id_map() the kernel would map
the InputID to ITS 0xC4 not 0xD4 as it would be expected.
Therefore, to keep supporting existing flawed firmwares, introduce a
workaround that instructs the kernel to use the old InputID range check
logic in iort_id_map(), so that we can support both firmwares written
with the flawed 'Number of IDs' logic and the correct one as defined in
the specifications.
[0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
Reported-by: Pankaj Bansal <pankaj.bansal@nxp.com>
Link: https://lore.kernel.org/linux-acpi/20191215203303.29811-1-pankaj.bansal@nxp.com/
Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Cc: Will Deacon <will@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
---
drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++---
1 file changed, 52 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 33f71983e001..60eb10d46d2b 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -298,6 +298,42 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
return status;
}
+struct iort_workaround_oem_info {
+ char oem_id[ACPI_OEM_ID_SIZE + 1];
+ char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+ u32 oem_revision;
+};
+
+static bool apply_id_count_workaround;
+
+static struct iort_workaround_oem_info wa_info[] __initdata = {
+ {
+ .oem_id = "HISI ",
+ .oem_table_id = "HIP07 ",
+ .oem_revision = 0,
+ }, {
+ .oem_id = "HISI ",
+ .oem_table_id = "HIP08 ",
+ .oem_revision = 0,
+ }
+};
+
+static void __init
+iort_check_id_count_workaround(struct acpi_table_header *tbl)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
+ if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
+ !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+ wa_info[i].oem_revision == tbl->oem_revision) {
+ apply_id_count_workaround = true;
+ pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
+ break;
+ }
+ }
+}
+
static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
u32 *rid_out)
{
@@ -314,9 +350,21 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
return -ENXIO;
}
- if (rid_in < map->input_base ||
- (rid_in >= map->input_base + map->id_count))
- return -ENXIO;
+ /*
+ * IORT spec says Number of IDs = The number of IDs in the range minus
+ * one, but the IORT code ingored the "minus one", and some firmware
+ * did that too, so apply a workaround here to keep compatible with
+ * both new and old versions of the firmware.
+ */
+ if (apply_id_count_workaround) {
+ if (rid_in < map->input_base ||
+ (rid_in >= map->input_base + map->id_count))
+ return -ENXIO;
+ } else {
+ if (rid_in < map->input_base ||
+ (rid_in > map->input_base + map->id_count))
+ return -ENXIO;
+ }
*rid_out = map->output_base + (rid_in - map->input_base);
return 0;
@@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void)
return;
}
+ iort_check_id_count_workaround(iort_table);
iort_init_platform_devices();
}
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-10 12:11 ` Lorenzo Pieralisi
@ 2020-01-13 7:04 ` Hanjun Guo
0 siblings, 0 replies; 14+ messages in thread
From: Hanjun Guo @ 2020-01-13 7:04 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Rafael J. Wysocki, Pankaj Bansal, John Garry, linuxarm,
linux-acpi, Sudeep Holla, linux-arm-kernel
On 2020/1/10 20:11, Lorenzo Pieralisi wrote:
> On Mon, Dec 30, 2019 at 08:27:04PM +0800, Hanjun Guo wrote:
>> The IORT spec [0] says Number of IDs = The number of IDs in the range minus
>> one, it is confusing but it was written down in the first version of the
>> IORT spec. But the IORT ID mapping function iort_id_map() did something
>> wrong from the start, which bails out if:
>>
>> the request ID >= the input base + number of IDs
>>
>> This is wrong because it ignored the "minus one", and breaks some valid
>> usecases such as ID mapping to contain single device mapping without
>> single mapping flag set.
>>
>> Pankaj Bansal proposed a solution to fix the issue [1], which bails
>> out if:
>>
>> the request ID > the input base + number of IDs
>>
>> This works as the spec defined, unfortunately some firmware didn't
>> minus one for the number of IDs in the range, and the propoased
>> solution will break those systems in this way:
>>
>> PCI hostbridge mapping entry 1:
>> Input base: 0x1000
>> ID Count: 0x100
>> Output base: 0x1000
>> Output reference: 0xC4 //ITS reference
>>
>> PCI hostbridge mapping entry 2:
>> Input base: 0x1100
>> ID Count: 0x100
>> Output base: 0x2000
>> Output reference: 0xD4 //ITS reference
>>
>> Two mapping entries which the second entry's Input base = the first
>> entry's Input base + ID count, so for requester ID 0x1100 will map
>> to ITS 0xC4 not 0xD4 if we update '>=' to '>'.
>>
>> So introduce a workaround to match the IORT's OEM information for
>> the broken firmware, also update the logic of the ID mapping for
>> firmwares report the number of IDs as the IORT spec defined, to
>> make the code compatible for both kinds of system.
>>
>> I checked the ACPI tables in the tianocore/edk2-platforms [2], only
>> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround
>> info table, if we break other platforms, we can add that later.
>>
>> [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
>> [1]: https://patchwork.kernel.org/patch/11292823/
>> [2]: https://github.com/tianocore/edk2-platforms
>>
>> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>
> Commit log rewritten below - please update the code (and check the
> log) as per this thread discussion.
Thank you, I will resend it ASAP.
Thanks
Hanjun
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2019-12-30 12:27 [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue Hanjun Guo
` (2 preceding siblings ...)
2020-01-10 12:11 ` Lorenzo Pieralisi
@ 2020-01-13 9:34 ` John Garry
2020-01-14 7:19 ` Hanjun Guo
3 siblings, 1 reply; 14+ messages in thread
From: John Garry @ 2020-01-13 9:34 UTC (permalink / raw)
To: Guohanjun (Hanjun Guo),
Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki,
Pankaj Bansal
Cc: linux-acpi, Linuxarm, linux-arm-kernel
On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote:
> +};
> +
> +static bool apply_id_count_workaround;
> +
> +static struct iort_workaround_oem_info wa_info[] __initdata = {
> + {
> + .oem_id = "HISI ",
> + .oem_table_id = "HIP07 ",
> + .oem_revision = 0,
> + }, {
> + .oem_id = "HISI ",
> + .oem_table_id = "HIP08 ",
> + .oem_revision = 0,
> + }
> +};
Am I right in saying that any future BIOS for these chipsets will have
to continue to have buggy firmware? If so, it's unfortunate.
Thanks,
John
> +
> +static void __init
> +iort_check_id_count_workaround(struct acpi_table_header *tbl)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> + wa_info[i].oem_revision == tbl->oem_revision) {
> + apply_id_count_workaround = true;
> + pr_warn(FW_BUG "ID count for ID mapping entry is wrong, applying workaround\n");
> + break;
> + }
> + }
> +}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-13 9:34 ` John Garry
@ 2020-01-14 7:19 ` Hanjun Guo
2020-01-14 9:47 ` John Garry
0 siblings, 1 reply; 14+ messages in thread
From: Hanjun Guo @ 2020-01-14 7:19 UTC (permalink / raw)
To: John Garry, Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki,
Pankaj Bansal
Cc: linux-acpi, Linuxarm, linux-arm-kernel
On 2020/1/13 17:34, John Garry wrote:
> On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote:
>> +};
>> +
>> +static bool apply_id_count_workaround;
>> +
>> +static struct iort_workaround_oem_info wa_info[] __initdata = {
>> + {
>> + .oem_id = "HISI ",
>> + .oem_table_id = "HIP07 ",
>> + .oem_revision = 0,
>> + }, {
>> + .oem_id = "HISI ",
>> + .oem_table_id = "HIP08 ",
>> + .oem_revision = 0,
>> + }
>> +};
>
> Am I right in saying that any future BIOS for these chipsets will have to continue to have buggy firmware? If so, it's unfortunate.
For better compatibility, I would say yes :(
For example, if you fix that in the firmware, and update
the IORT revision number, then it will run pretty good
on new version of the kernel, but not on old version of
kernel without the backporting of this patch.
Thanks
Hanjun
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue
2020-01-14 7:19 ` Hanjun Guo
@ 2020-01-14 9:47 ` John Garry
0 siblings, 0 replies; 14+ messages in thread
From: John Garry @ 2020-01-14 9:47 UTC (permalink / raw)
To: Hanjun Guo, Lorenzo Pieralisi, Sudeep Holla, Rafael J. Wysocki,
Pankaj Bansal
Cc: linux-acpi, Linuxarm, linux-arm-kernel
On 14/01/2020 07:19, Hanjun Guo wrote:
> On 2020/1/13 17:34, John Garry wrote:
>> On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote:
>>> +};
>>> +
>>> +static bool apply_id_count_workaround;
>>> +
>>> +static struct iort_workaround_oem_info wa_info[] __initdata = {
>>> + {
>>> + .oem_id = "HISI ",
>>> + .oem_table_id = "HIP07 ",
>>> + .oem_revision = 0,
>>> + }, {
>>> + .oem_id = "HISI ",
>>> + .oem_table_id = "HIP08 ",
>>> + .oem_revision = 0,
>>> + }
>>> +};
>>
>> Am I right in saying that any future BIOS for these chipsets will have to continue to have buggy firmware? If so, it's unfortunate.
>
> For better compatibility, I would say yes :(
>
> For example, if you fix that in the firmware, and update
> the IORT revision number, then it will run pretty good
> on new version of the kernel, but not on old version of
> kernel without the backporting of this patch.
ok, so that seems to be a trade off then. Having to backport introduces
a risk.
So then it might be good to add a comment to ID count members in
open-source edk2-platforms hip07 and hip08 IORTs to mention it is buggy,
so not to be copied as a reference.
Cheers,
John
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-01-14 9:47 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-30 12:27 [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue Hanjun Guo
2020-01-02 11:18 ` John Garry
2020-01-03 10:20 ` Hanjun Guo
2020-01-06 17:19 ` Robin Murphy
2020-01-07 12:03 ` Hanjun Guo
2020-01-09 16:02 ` Lorenzo Pieralisi
2020-01-10 6:22 ` Hanjun Guo
2020-01-10 10:39 ` Lorenzo Pieralisi
2020-01-10 10:51 ` Robin Murphy
2020-01-10 12:11 ` Lorenzo Pieralisi
2020-01-13 7:04 ` Hanjun Guo
2020-01-13 9:34 ` John Garry
2020-01-14 7:19 ` Hanjun Guo
2020-01-14 9:47 ` John Garry
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).