[PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[John Garry]

+

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Hanjun Guo]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Robin Murphy]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Hanjun Guo]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Lorenzo Pieralisi]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Hanjun Guo]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Lorenzo Pieralisi]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Robin Murphy]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Lorenzo Pieralisi]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Hanjun Guo]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[John Garry]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Hanjun Guo]

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

Re: [PATCH v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[John Garry]

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