ACPI/IORT: rework num_ids off-by-one quirk

The ID mapping table structure of the IORT table describes the size of a range using a num_ids field carrying the number of IDs in the region minus one. This has been misinterpreted in the past in the parsing code, and firmware is known to have shipped where this results in an ambiguity, where regions that should be adjacent have an overlap of one value.

So let's work around this by detecting this case specifically: when resolving an ID translation, allow one that matches right at the end of a multi-ID region to be superseded by a subsequent one.

To prevent potential regressions on broken firmware that happened to work before, only take the subsequent match into account if it occurs at the start of a mapping region.

Signed-off-by Ard Biesheuvel <[email protected]>

---
  drivers/acpi/arm64/iort.c | 40 +++++++++++++++++---
  1 file changed, 34 insertions(+), 6 deletions(-)

drivers/acpi/arm64/iort.c +3 -2

index 98be18266a73..9f139a94a1d3 100644
@@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
  }
 
  static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
-		       u32 *rid_out)
+		       u32 *rid_out, bool check_overlap)
  {
  	/* Single mapping does not care for input id */
  	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {

@@ -316,10 +316,34 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
  	}
 
  	if (rid_in < map->input_base ||
-	    (rid_in >= map->input_base + map->id_count))
+	    (rid_in > map->input_base + map->id_count))
  		return -ENXIO;
 
+	if (check_overlap) {

Quoted 49 lines · Ard Biesheuvel

The ID mapping table structure of the IORT table describes the size of a range using a num_ids field carrying the number of IDs in the region minus one. This has been misinterpreted in the past in the parsing code, and firmware is known to have shipped where this results in an ambiguity, where regions that should be adjacent have an overlap of one value.

So let's work around this by detecting this case specifically: when resolving an ID translation, allow one that matches right at the end of a multi-ID region to be superseded by a subsequent one.

To prevent potential regressions on broken firmware that happened to work before, only take the subsequent match into account if it occurs at the start of a mapping region.

Signed-off-by Ard Biesheuvel <[email protected]>

---
  drivers/acpi/arm64/iort.c | 40 +++++++++++++++++---
  1 file changed, 34 insertions(+), 6 deletions(-)

drivers/acpi/arm64/iort.c +11 -2

index 98be18266a73..9f139a94a1d3 100644
@@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
  }
 
  static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
-                    u32 *rid_out)
+                    u32 *rid_out, bool check_overlap)
  {
      /* Single mapping does not care for input id */
      if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {

@@ -316,10 +316,34 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
      }
 
      if (rid_in < map->input_base ||
-         (rid_in >= map->input_base + map->id_count))
+         (rid_in > map->input_base + map->id_count))
              return -ENXIO;
 
+     if (check_overlap) {
+             /*
+              * We already found a mapping for this input ID at the end of
+              * another region. If it coincides with the start of this
+              * region, we assume the prior match was due to the off-by-1
+              * issue mentioned below, and allow it to be superseded.
+              * Otherwise, things are *really* broken, and we just disregard
+              * duplicate matches entirely to retain compatibility.
+              */

drivers/acpi/arm64/iort.c static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, · line 332

Quoted 2 lines

drivers/acpi/arm64/iort.c +2 -0

+             pr_err(FW_BUG "[map %p] conflicting mapping for input ID 0x%x\n",
+                    map, rid_in);

As we already applied a workaround here, can we add "applying workaround" in the error message? This will make the customers less uneasy to see such message in the boot log. Once the product was deliveried to customers, it's not that easy to update all the firmwares entirely.

I'm out of reach for D05/D06 hardware as it's holidays in China, please allow me to test this patch set in Wednesday this week.

Quoted 51 lines · Hanjun Guo

Quoted 49 lines · Ard Biesheuvel

The ID mapping table structure of the IORT table describes the size of a range using a num_ids field carrying the number of IDs in the region minus one. This has been misinterpreted in the past in the parsing code, and firmware is known to have shipped where this results in an ambiguity, where regions that should be adjacent have an overlap of one value.

So let's work around this by detecting this case specifically: when resolving an ID translation, allow one that matches right at the end of a multi-ID region to be superseded by a subsequent one.

To prevent potential regressions on broken firmware that happened to work before, only take the subsequent match into account if it occurs at the start of a mapping region.

Signed-off-by Ard Biesheuvel <[email protected]>

---
   drivers/acpi/arm64/iort.c | 40 +++++++++++++++++---
   1 file changed, 34 insertions(+), 6 deletions(-)

drivers/acpi/arm64/iort.c +11 -2

index 98be18266a73..9f139a94a1d3 100644
@@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
   }
 
   static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
-                    u32 *rid_out)
+                    u32 *rid_out, bool check_overlap)
   {
       /* Single mapping does not care for input id */
       if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {

@@ -316,10 +316,34 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
       }
 
       if (rid_in < map->input_base ||
-         (rid_in >= map->input_base + map->id_count))
+         (rid_in > map->input_base + map->id_count))
               return -ENXIO;
 
+     if (check_overlap) {
+             /*
+              * We already found a mapping for this input ID at the end of
+              * another region. If it coincides with the start of this
+              * region, we assume the prior match was due to the off-by-1
+              * issue mentioned below, and allow it to be superseded.
+              * Otherwise, things are *really* broken, and we just disregard
+              * duplicate matches entirely to retain compatibility.
+              */

Quoted 9 lines

drivers/acpi/arm64/iort.c static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, · line 332

Quoted 2 lines

drivers/acpi/arm64/iort.c +2 -0

+             pr_err(FW_BUG "[map %p] conflicting mapping for input ID 0x%x\n",
+                    map, rid_in);

As we already applied a workaround here, can we add "applying workaround" in the error message? This will make the customers less uneasy to see such message in the boot log. Once the product was deliveried to customers, it's not that easy to update all the firmwares entirely.

Sure.

Quoted 42 lines · Ard Biesheuvel

Quoted 40 lines · Hanjun Guo

Quoted 22 lines · Ard Biesheuvel

drivers/acpi/arm64/iort.c +3 -2

index 98be18266a73..9f139a94a1d3 100644
@@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
   }
 
   static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
-                    u32 *rid_out)
+                    u32 *rid_out, bool check_overlap)
   {
       /* Single mapping does not care for input id */
       if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {

@@ -316,10 +316,34 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
       }
 
       if (rid_in < map->input_base ||
-         (rid_in >= map->input_base + map->id_count))
+         (rid_in > map->input_base + map->id_count))
               return -ENXIO;
 
+     if (check_overlap) {

drivers/acpi/arm64/iort.c static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, · line 332

Quoted 10 lines

drivers/acpi/arm64/iort.c +10 -0

+             /*
+              * We already found a mapping for this input ID at the end of
+              * another region. If it coincides with the start of this
+              * region, we assume the prior match was due to the off-by-1
+              * issue mentioned below, and allow it to be superseded.
+              * Otherwise, things are *really* broken, and we just disregard
+              * duplicate matches entirely to retain compatibility.
+              */
+             pr_err(FW_BUG "[map %p] conflicting mapping for input ID 0x%x\n",
+                    map, rid_in);

As we already applied a workaround here, can we add "applying workaround" in the error message? This will make the customers less uneasy to see such message in the boot log. Once the product was deliveried to customers, it's not that easy to update all the firmwares entirely.

Quoted 1 line

Sure.

Since Will already merged this patchset, I would like to send a patch on top of it, what do you think?

Quoted 43 lines · Hanjun Guo

Quoted 42 lines · Ard Biesheuvel

Quoted 40 lines · Hanjun Guo

Quoted 22 lines · Ard Biesheuvel

drivers/acpi/arm64/iort.c +3 -2

index 98be18266a73..9f139a94a1d3 100644
@@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
    }
 
    static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
-                    u32 *rid_out)
+                    u32 *rid_out, bool check_overlap)
    {
        /* Single mapping does not care for input id */
        if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {

@@ -316,10 +316,34 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
        }
 
        if (rid_in < map->input_base ||
-         (rid_in >= map->input_base + map->id_count))
+         (rid_in > map->input_base + map->id_count))
                return -ENXIO;
 
+     if (check_overlap) {

drivers/acpi/arm64/iort.c static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, · line 332

Quoted 10 lines

drivers/acpi/arm64/iort.c +10 -0

+             /*
+              * We already found a mapping for this input ID at the end of
+              * another region. If it coincides with the start of this
+              * region, we assume the prior match was due to the off-by-1
+              * issue mentioned below, and allow it to be superseded.
+              * Otherwise, things are *really* broken, and we just disregard
+              * duplicate matches entirely to retain compatibility.
+              */
+             pr_err(FW_BUG "[map %p] conflicting mapping for input ID 0x%x\n",
+                    map, rid_in);

As we already applied a workaround here, can we add "applying workaround" in the error message? This will make the customers less uneasy to see such message in the boot log. Once the product was deliveried to customers, it's not that easy to update all the firmwares entirely.

Quoted 4 lines

Quoted 1 line

Sure.

Since Will already merged this patchset, I would like to send a patch on top of it, what do you think?

Yes, please! I figured I'd queue it, as I could always revert it if your testing came back negative but extra stuff on top is always fine.

Quoted 43 lines · Ard Biesheuvel

The ID mapping table structure of the IORT table describes the size of a range using a num_ids field carrying the number of IDs in the region minus one. This has been misinterpreted in the past in the parsing code, and firmware is known to have shipped where this results in an ambiguity, where regions that should be adjacent have an overlap of one value.

So let's work around this by detecting this case specifically: when resolving an ID translation, allow one that matches right at the end of a multi-ID region to be superseded by a subsequent one.

To prevent potential regressions on broken firmware that happened to work before, only take the subsequent match into account if it occurs at the start of a mapping region.

Signed-off-by Ard Biesheuvel <[email protected]>

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

  1 file changed, 34 insertions(+), 6 deletions(-)

drivers/acpi/arm64/iort.c +1 -1

index 98be18266a73..9f139a94a1d3 100644
@@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct 
 acpi_iort_node *node,
   }
   static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
-               u32 *rid_out)
+               u32 *rid_out, bool check_overlap)
   {
       /* Single mapping does not care for input id */
       if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {

@@ -316,10 +316,34 @@ static int iort_id_map(struct 
 acpi_iort_id_mapping *map, u8 type, u32 rid_in,
       }

      if (rid_in < map->input_base ||
-        (rid_in >= map->input_base + map->id_count))
+        (rid_in > map->input_base + map->id_count))
          return -ENXIO;
+    if (check_overlap) {
+        /*
+         * We already found a mapping for this input ID at the end of +         * another region. If it coincides with the start of this

Quoted 8 lines

+         * region, we assume the prior match was due to the off-by-1 +         * issue mentioned below, and allow it to be superseded. +         * Otherwise, things are *really* broken, and we just disregard +         * duplicate matches entirely to retain compatibility. +         */
+        pr_err(FW_BUG "[map %p] conflicting mapping for input ID 0x%x\n",
+               map, rid_in);

As we already applied a workaround here, can we add "applying workaround" in the error message? This will make the customers less uneasy to see such message in the boot log. Once the product was deliveried to customers, it's not that easy to update all the firmwares entirely.

I'm out of reach for D05/D06 hardware as it's holidays in China, please allow me to test this patch set in Wednesday this week.

The ID mapping table structure of the IORT table describes the size of a range using a num_ids field carrying the number of IDs in the region minus one. This has been misinterpreted in the past in the parsing code, and firmware is known to have shipped where this results in an ambiguity, where regions that should be adjacent have an overlap of one value.

So let's work around this by detecting this case specifically: when resolving an ID translation, allow one that matches right at the end of a multi-ID region to be superseded by a subsequent one.

To prevent potential regressions on broken firmware that happened to work before, only take the subsequent match into account if it occurs at the start of a mapping region.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/acpi/arm64/iort.c | 40 +++++++++++++++++---
 1 file changed, 34 insertions(+), 6 deletions(-)

drivers/acpi/arm64/iort.c +34 -6

index 98be18266a73..9f139a94a1d3 100644
@@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
 }
 
 static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
-		       u32 *rid_out)
+		       u32 *rid_out, bool check_overlap)
 {
 	/* Single mapping does not care for input id */
 	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {

@@ -316,10 +316,34 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
 	}
 
 	if (rid_in < map->input_base ||
-	    (rid_in >= map->input_base + map->id_count))
+	    (rid_in > map->input_base + map->id_count))
 		return -ENXIO;
 
+	if (check_overlap) {
+		/*
+		 * We already found a mapping for this input ID at the end of
+		 * another region. If it coincides with the start of this
+		 * region, we assume the prior match was due to the off-by-1
+		 * issue mentioned below, and allow it to be superseded.
+		 * Otherwise, things are *really* broken, and we just disregard
+		 * duplicate matches entirely to retain compatibility.
+		 */
+		pr_err(FW_BUG "[map %p] conflicting mapping for input ID 0x%x\n",
+		       map, rid_in);
+		if (rid_in != map->input_base)
+			return -ENXIO;
+	}
+
 	*rid_out = map->output_base + (rid_in - map->input_base);
+
+	/*
+	 * Due to confusion regarding the meaning of the id_count field (which
+	 * carries the number of IDs *minus 1*), we may have to disregard this
+	 * match if it is at the end of the range, and overlaps with the start
+	 * of another one.
+	 */
+	if (map->id_count > 0 && rid_in == map->input_base + map->id_count)
+		return -EAGAIN;
 	return 0;
 }
 

@@ -404,7 +428,8 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 	/* Parse the ID mapping tree to find specified node type */
 	while (node) {
 		struct acpi_iort_id_mapping *map;
-		int i, index;
+		int i, index, rc = 0;
+		u32 out_ref = 0, map_id = id;
 
 		if (IORT_TYPE_MASK(node->type) & type_mask) {
 			if (id_out)

@@ -438,15 +463,18 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
 			if (i == index)
 				continue;
 
-			if (!iort_id_map(map, node->type, id, &id))
+			rc = iort_id_map(map, node->type, map_id, &id, out_ref);
+			if (!rc)
 				break;
+			if (rc == -EAGAIN)
+				out_ref = map->output_reference;
 		}
 
-		if (i == node->mapping_count)
+		if (i == node->mapping_count && !out_ref)
 			goto fail_map;
 
 		node = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
-				    map->output_reference);
+				    rc ? out_ref : map->output_reference);
 	}
 
 fail_map:

Signature · 1 line

2.17.1

Replace the ACPI OEM ID matching based IORT quirk for the ID region size ambiguity with runtime handling of this condition.

This is based on the observation that we only care about this when it causes ambiguity regarding the output reference, which means that we will have more than one match for the input ID. In this case, we can just disregard the one at the end of a multi-ID region: if we hit it first, we record it tentatively but allow a subsequent match to supersede it. If we hit the correct match first, there is nothing we need to do.

[...]