[RFC PATCH 1/2] ACPICA/IORT: Correct the comment for id_count

[RFC PATCH 1/2] ACPICA/IORT: Correct the comment for id_count

[Hanjun Guo]

In IORT spec
(http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf),
id_num means Number of IDs minus one, update the comment.

Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
---

This patch just for comments, needs to be upstream in ACPICA first.

 include/acpi/actbl2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index e45ced2..382642f 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -104,7 +104,7 @@ enum acpi_iort_node_type {
 
 struct acpi_iort_id_mapping {
 	u32 input_base;		/* Lowest value in input range */
-	u32 id_count;		/* Number of IDs */
+	u32 id_count;		/* Number of IDs in the range minus one */
 	u32 output_base;	/* Lowest value in output range */
 	u32 output_reference;	/* A reference to the output node */
 	u32 flags;
-- 
1.7.12.4

[RFC PATCH 1/2] ACPICA/IORT: Correct the comment for id_count

[Hanjun Guo]

In IORT spec
(http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf),
id_num means Number of IDs minus one, update the comment.

Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
---

This patch just for comments, needs to be upstream in ACPICA first.

 include/acpi/actbl2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index e45ced2..382642f 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -104,7 +104,7 @@ enum acpi_iort_node_type {
 
 struct acpi_iort_id_mapping {
 	u32 input_base;		/* Lowest value in input range */
-	u32 id_count;		/* Number of IDs */
+	u32 id_count;		/* Number of IDs in the range minus one */
 	u32 output_base;	/* Lowest value in output range */
 	u32 output_reference;	/* A reference to the output node */
 	u32 flags;
-- 
1.7.12.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 2/2] 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>
---
 drivers/acpi/arm64/iort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 33f7198..112b1b0 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -298,6 +298,41 @@ 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;
+			break;
+		}
+	}
+}
+
 static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
 		       u32 *rid_out)
 {
@@ -314,9 +349,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 +1678,6 @@ void __init acpi_iort_init(void)
 		return;
 	}
 
+	iort_check_id_count_workaround(iort_table);
 	iort_init_platform_devices();
 }
-- 
1.7.12.4

[RFC PATCH 2/2] 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>
---
 drivers/acpi/arm64/iort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 33f7198..112b1b0 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -298,6 +298,41 @@ 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;
+			break;
+		}
+	}
+}
+
 static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
 		       u32 *rid_out)
 {
@@ -314,9 +349,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 +1678,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: [RFC PATCH 1/2] ACPICA/IORT: Correct the comment for id_count

[John Garry]

On 23/12/2019 09:23, Hanjun Guo wrote:
> In IORT spec
> (http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf),
> id_num means Number of IDs minus one, update the comment.
> 
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
> 
> This patch just for comments, needs to be upstream in ACPICA first.
> 
>   include/acpi/actbl2.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index e45ced2..382642f 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -104,7 +104,7 @@ enum acpi_iort_node_type {
>   
>   struct acpi_iort_id_mapping {
>   	u32 input_base;		/* Lowest value in input range */
> -	u32 id_count;		/* Number of IDs */
> +	u32 id_count;		/* Number of IDs in the range minus one */

The IORT spec also uses the term "Length" in the examples...

>   	u32 output_base;	/* Lowest value in output range */
>   	u32 output_reference;	/* A reference to the output node */
>   	u32 flags;
> 

Re: [RFC PATCH 1/2] ACPICA/IORT: Correct the comment for id_count

[John Garry]

On 23/12/2019 09:23, Hanjun Guo wrote:
> In IORT spec
> (http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf),
> id_num means Number of IDs minus one, update the comment.
> 
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
> 
> This patch just for comments, needs to be upstream in ACPICA first.
> 
>   include/acpi/actbl2.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index e45ced2..382642f 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -104,7 +104,7 @@ enum acpi_iort_node_type {
>   
>   struct acpi_iort_id_mapping {
>   	u32 input_base;		/* Lowest value in input range */
> -	u32 id_count;		/* Number of IDs */
> +	u32 id_count;		/* Number of IDs in the range minus one */

The IORT spec also uses the term "Length" in the examples...

>   	u32 output_base;	/* Lowest value in output range */
>   	u32 output_reference;	/* A reference to the output node */
>   	u32 flags;
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Pankaj Bansal]

​
Can we put a warning of the sort to update the ACPI table with correct values?
eventually i would like that this workaround be removed from linux, when sufficient time has passed and all the platforms' ACPI tables have been updated.?





From: Hanjun Guo <guohanjun@huawei.com>

Sent: Monday, December 23, 2019 2:53 PM

To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; Rafael J. Wysocki <rafael@kernel.org>; Pankaj Bansal <pankaj.bansal@nxp.com>; Erik Schmauss <erik.schmauss@intel.com>

Cc: linux-acpi@vger.kernel.org <linux-acpi@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linuxarm@huawei.com <linuxarm@huawei.com>; Hanjun Guo <guohanjun@huawei.com>

Subject: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus one" issue

 


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]: 
https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfocenter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0049d%2FDEN0049D_IO_Remapping_Table.pdf&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cb4823c8648a74a3781c908d7878a80e7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637126901245015642&amp;sdata=y2hx8g10J73ngUv5ZFIsfsXxfoOWGZN8%2B7PmIARKgJI%3D&amp;reserved=0

[1]: 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11292823%2F&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cb4823c8648a74a3781c908d7878a80e7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637126901245015642&amp;sdata=XmpNP688fed2bbjm8Wu4L5ftxECRzmkr7vp1Y4QXziQ%3D&amp;reserved=0

[2]: 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-platforms&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cb4823c8648a74a3781c908d7878a80e7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637126901245015642&amp;sdata=xlmD8Xu6Wc5adwhouzDKRGlwCAwRwsD0cEyW4ioa7Xg%3D&amp;reserved=0



Cc: Pankaj Bansal <pankaj.bansal@nxp.com>

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Signed-off-by: Hanjun Guo <guohanjun@huawei.com>

---

 drivers/acpi/arm64/iort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++---

 1 file changed, 51 insertions(+), 3 deletions(-)



diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

index 33f7198..112b1b0 100644

--- a/drivers/acpi/arm64/iort.c

+++ b/drivers/acpi/arm64/iort.c

@@ -298,6 +298,41 @@ 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;

+                       break;

+               }

+       }

+}

+

 static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,

                        u32 *rid_out)

 {

@@ -314,9 +349,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 +1678,6 @@ void __init acpi_iort_init(void)

                 return;

         }

 

+       iort_check_id_count_workaround(iort_table);

         iort_init_platform_devices();

 }

-- 

1.7.12.4

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

[Pankaj Bansal]

​
Can we put a warning of the sort to update the ACPI table with correct values?
eventually i would like that this workaround be removed from linux, when sufficient time has passed and all the platforms' ACPI tables have been updated.?





From: Hanjun Guo <guohanjun@huawei.com>

Sent: Monday, December 23, 2019 2:53 PM

To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; Rafael J. Wysocki <rafael@kernel.org>; Pankaj Bansal <pankaj.bansal@nxp.com>; Erik Schmauss <erik.schmauss@intel.com>

Cc: linux-acpi@vger.kernel.org <linux-acpi@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linuxarm@huawei.com <linuxarm@huawei.com>; Hanjun Guo <guohanjun@huawei.com>

Subject: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus one" issue

 


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]: 
https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfocenter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0049d%2FDEN0049D_IO_Remapping_Table.pdf&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cb4823c8648a74a3781c908d7878a80e7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637126901245015642&amp;sdata=y2hx8g10J73ngUv5ZFIsfsXxfoOWGZN8%2B7PmIARKgJI%3D&amp;reserved=0

[1]: 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11292823%2F&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cb4823c8648a74a3781c908d7878a80e7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637126901245015642&amp;sdata=XmpNP688fed2bbjm8Wu4L5ftxECRzmkr7vp1Y4QXziQ%3D&amp;reserved=0

[2]: 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-platforms&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7Cb4823c8648a74a3781c908d7878a80e7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637126901245015642&amp;sdata=xlmD8Xu6Wc5adwhouzDKRGlwCAwRwsD0cEyW4ioa7Xg%3D&amp;reserved=0



Cc: Pankaj Bansal <pankaj.bansal@nxp.com>

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Signed-off-by: Hanjun Guo <guohanjun@huawei.com>

---

 drivers/acpi/arm64/iort.c | 54 ++++++++++++++++++++++++++++++++++++++++++++---

 1 file changed, 51 insertions(+), 3 deletions(-)



diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

index 33f7198..112b1b0 100644

--- a/drivers/acpi/arm64/iort.c

+++ b/drivers/acpi/arm64/iort.c

@@ -298,6 +298,41 @@ 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;

+                       break;

+               }

+       }

+}

+

 static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,

                        u32 *rid_out)

 {

@@ -314,9 +349,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 +1678,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: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Hanjun Guo]

On 2019/12/23 20:17, Pankaj Bansal wrote:
> ​
> Can we put a warning of the sort to update the> ACPI table with correct values?

Good point, I will add the firmware bug warning
in iort_check_id_count_workaround().

> eventually i would like that this workaround be removed from
> linux, when sufficient time has passed and all the
> platforms' ACPI tables have been updated.?

I agree with you but it will take much longer time than expected.

Thanks
Hanjun

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

[Hanjun Guo]

On 2019/12/23 20:17, Pankaj Bansal wrote:
> ​
> Can we put a warning of the sort to update the> ACPI table with correct values?

Good point, I will add the firmware bug warning
in iort_check_id_count_workaround().

> eventually i would like that this workaround be removed from
> linux, when sufficient time has passed and all the
> platforms' ACPI tables have been updated.?

I agree with you but it will take much longer time than expected.

Thanks
Hanjun


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/2] ACPICA/IORT: Correct the comment for id_count

[Hanjun Guo]

On 2019/12/23 18:35, John Garry wrote:
> On 23/12/2019 09:23, Hanjun Guo wrote:
>> In IORT spec
>> (http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf),
>> id_num means Number of IDs minus one, update the comment.
>>
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>> ---
>>
>> This patch just for comments, needs to be upstream in ACPICA first.
>>
>>   include/acpi/actbl2.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
>> index e45ced2..382642f 100644
>> --- a/include/acpi/actbl2.h
>> +++ b/include/acpi/actbl2.h
>> @@ -104,7 +104,7 @@ enum acpi_iort_node_type {
>>     struct acpi_iort_id_mapping {
>>       u32 input_base;        /* Lowest value in input range */
>> -    u32 id_count;        /* Number of IDs */
>> +    u32 id_count;        /* Number of IDs in the range minus one */
> 
> The IORT spec also uses the term "Length" in the examples...

More confusing...

Re: [RFC PATCH 1/2] ACPICA/IORT: Correct the comment for id_count

[Hanjun Guo]

On 2019/12/23 18:35, John Garry wrote:
> On 23/12/2019 09:23, Hanjun Guo wrote:
>> In IORT spec
>> (http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf),
>> id_num means Number of IDs minus one, update the comment.
>>
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>> ---
>>
>> This patch just for comments, needs to be upstream in ACPICA first.
>>
>>   include/acpi/actbl2.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
>> index e45ced2..382642f 100644
>> --- a/include/acpi/actbl2.h
>> +++ b/include/acpi/actbl2.h
>> @@ -104,7 +104,7 @@ enum acpi_iort_node_type {
>>     struct acpi_iort_id_mapping {
>>       u32 input_base;        /* Lowest value in input range */
>> -    u32 id_count;        /* Number of IDs */
>> +    u32 id_count;        /* Number of IDs in the range minus one */
> 
> The IORT spec also uses the term "Length" in the examples...

More confusing...


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Shameerali Kolothum Thodi]

Hi Hanjun,

> -----Original Message-----
> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of Hanjun
> Guo
> Sent: 23 December 2019 09:23
> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Sudeep Holla
> <sudeep.holla@arm.com>; Rafael J. Wysocki <rafael@kernel.org>; Pankaj
> Bansal <pankaj.bansal@nxp.com>; Erik Schmauss <erik.schmauss@intel.com>
> Cc: linux-acpi@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus
> one" issue
> 
> 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>
> ---
>  drivers/acpi/arm64/iort.c | 54
> ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 33f7198..112b1b0 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -298,6 +298,41 @@ 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;
> +			break;
> +		}
> +	}
> +}
> +

Can we get rid of the above and instead use acpi_match_platform_list() ? Please 
take a look at the pmcg_plat_info used for the HIP08 SMMUv3 PMCG erratum.

Thanks,
Shameer

>  static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>  		       u32 *rid_out)
>  {
> @@ -314,9 +349,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 +1678,6 @@ void __init acpi_iort_init(void)
>  		return;
>  	}
> 
> +	iort_check_id_count_workaround(iort_table);
>  	iort_init_platform_devices();
>  }
> --
> 1.7.12.4
> 
> _______________________________________________
> Linuxarm mailing list
> Linuxarm@huawei.com
> http://hulk.huawei.com/mailman/listinfo/linuxarm

RE: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus one" issue

[Shameerali Kolothum Thodi]

Hi Hanjun,

> -----Original Message-----
> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of Hanjun
> Guo
> Sent: 23 December 2019 09:23
> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Sudeep Holla
> <sudeep.holla@arm.com>; Rafael J. Wysocki <rafael@kernel.org>; Pankaj
> Bansal <pankaj.bansal@nxp.com>; Erik Schmauss <erik.schmauss@intel.com>
> Cc: linux-acpi@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus
> one" issue
> 
> 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>
> ---
>  drivers/acpi/arm64/iort.c | 54
> ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 33f7198..112b1b0 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -298,6 +298,41 @@ 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;
> +			break;
> +		}
> +	}
> +}
> +

Can we get rid of the above and instead use acpi_match_platform_list() ? Please 
take a look at the pmcg_plat_info used for the HIP08 SMMUv3 PMCG erratum.

Thanks,
Shameer

>  static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>  		       u32 *rid_out)
>  {
> @@ -314,9 +349,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 +1678,6 @@ void __init acpi_iort_init(void)
>  		return;
>  	}
> 
> +	iort_check_id_count_workaround(iort_table);
>  	iort_init_platform_devices();
>  }
> --
> 1.7.12.4
> 
> _______________________________________________
> Linuxarm mailing list
> Linuxarm@huawei.com
> http://hulk.huawei.com/mailman/listinfo/linuxarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Hanjun Guo]

On 2020/1/2 18:20, Shameerali Kolothum Thodi wrote:
> Hi Hanjun,
> 
>> -----Original Message-----
>> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of Hanjun
>> Guo
>> Sent: 23 December 2019 09:23
>> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Sudeep Holla
>> <sudeep.holla@arm.com>; Rafael J. Wysocki <rafael@kernel.org>; Pankaj
>> Bansal <pankaj.bansal@nxp.com>; Erik Schmauss <erik.schmauss@intel.com>
>> Cc: linux-acpi@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
>> linux-arm-kernel@lists.infradead.org
>> Subject: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus
>> one" issue
>>
>> 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>
>> ---
>>  drivers/acpi/arm64/iort.c | 54
>> ++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 33f7198..112b1b0 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -298,6 +298,41 @@ 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;
>> +			break;
>> +		}
>> +	}
>> +}
>> +
> 
> Can we get rid of the above and instead use acpi_match_platform_list() ? Please 
> take a look at the pmcg_plat_info used for the HIP08 SMMUv3 PMCG erratum.

Thanks for the reminding, I noticed acpi_match_platform_list() before but I
thought it was a little overkill (get the table header for each OEM info),
I will take a look further to see if I can consolidate the OEM information retrieve.

Thanks
Hanjun

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

[Hanjun Guo]

On 2020/1/2 18:20, Shameerali Kolothum Thodi wrote:
> Hi Hanjun,
> 
>> -----Original Message-----
>> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of Hanjun
>> Guo
>> Sent: 23 December 2019 09:23
>> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Sudeep Holla
>> <sudeep.holla@arm.com>; Rafael J. Wysocki <rafael@kernel.org>; Pankaj
>> Bansal <pankaj.bansal@nxp.com>; Erik Schmauss <erik.schmauss@intel.com>
>> Cc: linux-acpi@vger.kernel.org; Linuxarm <linuxarm@huawei.com>;
>> linux-arm-kernel@lists.infradead.org
>> Subject: [RFC PATCH 2/2] ACPI/IORT: Workaround for IORT ID count "minus
>> one" issue
>>
>> 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>
>> ---
>>  drivers/acpi/arm64/iort.c | 54
>> ++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 33f7198..112b1b0 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -298,6 +298,41 @@ 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;
>> +			break;
>> +		}
>> +	}
>> +}
>> +
> 
> Can we get rid of the above and instead use acpi_match_platform_list() ? Please 
> take a look at the pmcg_plat_info used for the HIP08 SMMUv3 PMCG erratum.

Thanks for the reminding, I noticed acpi_match_platform_list() before but I
thought it was a little overkill (get the table header for each OEM info),
I will take a look further to see if I can consolidate the OEM information retrieve.

Thanks
Hanjun


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel