* [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk @ 2020-05-01 16:10 Ard Biesheuvel 2020-05-01 16:10 ` [PATCH v2 1/2] Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" Ard Biesheuvel ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ard Biesheuvel @ 2020-05-01 16:10 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-acpi, Ard Biesheuvel, Hanjun Guo, Lorenzo Pieralisi, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy 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. Changes since v1: - print FW_BUG error to the kernel log when a duplicate match is found - ignore duplicate matches unless they occur at the start of a region (for compatibility with broken systems that might exist that happen to work today because the first match is always chosen) Cc: Hanjun Guo <guohanjun@huawei.com> Cc: 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> Ard Biesheuvel (2): Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" ACPI/IORT: work around num_ids ambiguity drivers/acpi/arm64/iort.c | 95 ++++++++------------ 1 file changed, 35 insertions(+), 60 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" 2020-05-01 16:10 [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk Ard Biesheuvel @ 2020-05-01 16:10 ` Ard Biesheuvel 2020-05-01 16:10 ` [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity Ard Biesheuvel 2020-05-04 11:34 ` [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk Will Deacon 2 siblings, 0 replies; 11+ messages in thread From: Ard Biesheuvel @ 2020-05-01 16:10 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-acpi, Ard Biesheuvel, Hanjun Guo, Lorenzo Pieralisi, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy This reverts commit 3c23b83a88d00383e1d498cfa515249aa2fe0238. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/acpi/arm64/iort.c | 57 +------------------- 1 file changed, 2 insertions(+), 55 deletions(-) diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 7d04424189df..98be18266a73 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -299,59 +299,6 @@ 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 inline u32 iort_get_map_max(struct acpi_iort_id_mapping *map) -{ - u32 map_max = map->input_base + map->id_count; - - /* - * The IORT specification revision D (Section 3, table 4, page 9) says - * Number of IDs = The number of IDs in the range minus one, but the - * IORT code ignored the "minus one", and some firmware did that too, - * so apply a workaround here to keep compatible with both the spec - * compliant and non-spec compliant firmwares. - */ - if (apply_id_count_workaround) - map_max--; - - return map_max; -} - static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, u32 *rid_out) { @@ -368,7 +315,8 @@ 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 > iort_get_map_max(map)) + 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); @@ -1703,6 +1651,5 @@ void __init acpi_iort_init(void) return; } - iort_check_id_count_workaround(iort_table); iort_init_platform_devices(); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-01 16:10 [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk Ard Biesheuvel 2020-05-01 16:10 ` [PATCH v2 1/2] Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" Ard Biesheuvel @ 2020-05-01 16:10 ` Ard Biesheuvel 2020-05-04 4:32 ` Hanjun Guo 2020-05-04 11:00 ` Lorenzo Pieralisi 2020-05-04 11:34 ` [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk Will Deacon 2 siblings, 2 replies; 11+ messages in thread From: Ard Biesheuvel @ 2020-05-01 16:10 UTC (permalink / raw) To: linux-arm-kernel Cc: linux-acpi, Ard Biesheuvel, Hanjun Guo, Lorenzo Pieralisi, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy 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 <ardb@kernel.org> --- drivers/acpi/arm64/iort.c | 40 +++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 98be18266a73..9f139a94a1d3 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -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: -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-01 16:10 ` [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity Ard Biesheuvel @ 2020-05-04 4:32 ` Hanjun Guo 2020-05-04 7:36 ` Ard Biesheuvel 2020-05-06 12:43 ` Hanjun Guo 2020-05-04 11:00 ` Lorenzo Pieralisi 1 sibling, 2 replies; 11+ messages in thread From: Hanjun Guo @ 2020-05-04 4:32 UTC (permalink / raw) To: Ard Biesheuvel, linux-arm-kernel Cc: linux-acpi, Lorenzo Pieralisi, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy, Linuxarm On 2020/5/2 0:10, Ard Biesheuvel wrote: > 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 <ardb@kernel.org> > --- > drivers/acpi/arm64/iort.c | 40 +++++++++++++++++--- > 1 file changed, 34 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 98be18266a73..9f139a94a1d3 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -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); 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. Thanks Hanjun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-04 4:32 ` Hanjun Guo @ 2020-05-04 7:36 ` Ard Biesheuvel 2020-05-06 12:44 ` Hanjun Guo 2020-05-06 12:43 ` Hanjun Guo 1 sibling, 1 reply; 11+ messages in thread From: Ard Biesheuvel @ 2020-05-04 7:36 UTC (permalink / raw) To: Hanjun Guo Cc: Linux ARM, ACPI Devel Maling List, Lorenzo Pieralisi, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy, Linuxarm On Mon, 4 May 2020 at 06:32, Hanjun Guo <guohanjun@huawei.com> wrote: > > On 2020/5/2 0:10, Ard Biesheuvel wrote: > > 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 <ardb@kernel.org> > > --- > > drivers/acpi/arm64/iort.c | 40 +++++++++++++++++--- > > 1 file changed, 34 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index 98be18266a73..9f139a94a1d3 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -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); > > 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. > 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. > Yes please ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-04 7:36 ` Ard Biesheuvel @ 2020-05-06 12:44 ` Hanjun Guo 2020-05-06 12:55 ` Will Deacon 0 siblings, 1 reply; 11+ messages in thread From: Hanjun Guo @ 2020-05-06 12:44 UTC (permalink / raw) To: Ard Biesheuvel Cc: Linux ARM, ACPI Devel Maling List, Lorenzo Pieralisi, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy, Linuxarm On 2020/5/4 15:36, Ard Biesheuvel wrote: > On Mon, 4 May 2020 at 06:32, Hanjun Guo <guohanjun@huawei.com> wrote: >> >> On 2020/5/2 0:10, Ard Biesheuvel wrote: >>> 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 <ardb@kernel.org> >>> --- >>> drivers/acpi/arm64/iort.c | 40 +++++++++++++++++--- >>> 1 file changed, 34 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >>> index 98be18266a73..9f139a94a1d3 100644 >>> --- a/drivers/acpi/arm64/iort.c >>> +++ b/drivers/acpi/arm64/iort.c >>> @@ -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); >> >> 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. Since Will already merged this patchset, I would like to send a patch on top of it, what do you think? Thanks Hanjun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-06 12:44 ` Hanjun Guo @ 2020-05-06 12:55 ` Will Deacon 2020-05-07 13:18 ` Hanjun Guo 0 siblings, 1 reply; 11+ messages in thread From: Will Deacon @ 2020-05-06 12:55 UTC (permalink / raw) To: Hanjun Guo Cc: Ard Biesheuvel, Linux ARM, ACPI Devel Maling List, Lorenzo Pieralisi, Pankaj Bansal, Sudeep Holla, Catalin Marinas, Robin Murphy, Linuxarm On Wed, May 06, 2020 at 08:44:55PM +0800, Hanjun Guo wrote: > On 2020/5/4 15:36, Ard Biesheuvel wrote: > > On Mon, 4 May 2020 at 06:32, Hanjun Guo <guohanjun@huawei.com> wrote: > > > On 2020/5/2 0:10, Ard Biesheuvel wrote: > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > > > index 98be18266a73..9f139a94a1d3 100644 > > > > --- a/drivers/acpi/arm64/iort.c > > > > +++ b/drivers/acpi/arm64/iort.c > > > > @@ -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); > > > > > > 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. > > 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. Will ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-06 12:55 ` Will Deacon @ 2020-05-07 13:18 ` Hanjun Guo 0 siblings, 0 replies; 11+ messages in thread From: Hanjun Guo @ 2020-05-07 13:18 UTC (permalink / raw) To: Will Deacon Cc: Ard Biesheuvel, Linux ARM, ACPI Devel Maling List, Lorenzo Pieralisi, Pankaj Bansal, Sudeep Holla, Catalin Marinas, Robin Murphy, Linuxarm On 2020/5/6 20:55, Will Deacon wrote: > On Wed, May 06, 2020 at 08:44:55PM +0800, Hanjun Guo wrote: >> On 2020/5/4 15:36, Ard Biesheuvel wrote: >>> On Mon, 4 May 2020 at 06:32, Hanjun Guo <guohanjun@huawei.com> wrote: >>>> On 2020/5/2 0:10, Ard Biesheuvel wrote: >>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >>>>> index 98be18266a73..9f139a94a1d3 100644 >>>>> --- a/drivers/acpi/arm64/iort.c >>>>> +++ b/drivers/acpi/arm64/iort.c >>>>> @@ -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); >>>> >>>> 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. >> >> 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. OK, I will prepare a patch and send out for review. Thanks Hanjun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-04 4:32 ` Hanjun Guo 2020-05-04 7:36 ` Ard Biesheuvel @ 2020-05-06 12:43 ` Hanjun Guo 1 sibling, 0 replies; 11+ messages in thread From: Hanjun Guo @ 2020-05-06 12:43 UTC (permalink / raw) To: Ard Biesheuvel, linux-arm-kernel Cc: linux-acpi, Lorenzo Pieralisi, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy, Linuxarm On 2020/5/4 12:32, Hanjun Guo wrote: > On 2020/5/2 0:10, Ard Biesheuvel wrote: >> 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 <ardb@kernel.org> >> --- >> drivers/acpi/arm64/iort.c | 40 +++++++++++++++++--- >> 1 file changed, 34 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >> index 98be18266a73..9f139a94a1d3 100644 >> --- a/drivers/acpi/arm64/iort.c >> +++ b/drivers/acpi/arm64/iort.c >> @@ -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); > > 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. Tested this patchset on D06 and I comapared each sucessful mapped ID, no regressions, and also no function regressions as well, Tested-by: Hanjun Guo <guohanjun@huawei.com> Thanks Hanjun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity 2020-05-01 16:10 ` [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity Ard Biesheuvel 2020-05-04 4:32 ` Hanjun Guo @ 2020-05-04 11:00 ` Lorenzo Pieralisi 1 sibling, 0 replies; 11+ messages in thread From: Lorenzo Pieralisi @ 2020-05-04 11:00 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-arm-kernel, linux-acpi, Hanjun Guo, Pankaj Bansal, Will Deacon, Sudeep Holla, Catalin Marinas, Robin Murphy On Fri, May 01, 2020 at 06:10:14PM +0200, Ard Biesheuvel wrote: > 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 <ardb@kernel.org> > --- > drivers/acpi/arm64/iort.c | 40 +++++++++++++++++--- > 1 file changed, 34 insertions(+), 6 deletions(-) The patch logic is sound - I still think that the resulting code can benefit from a one-off boot time mapping data initialisation but we can address that later as a clean-up, first thing is to remove the quirk mechanism. Goes without saying, this needs extensive testing on existing platforms before sending it to stable kernels. Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 98be18266a73..9f139a94a1d3 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -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: > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk 2020-05-01 16:10 [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk Ard Biesheuvel 2020-05-01 16:10 ` [PATCH v2 1/2] Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" Ard Biesheuvel 2020-05-01 16:10 ` [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity Ard Biesheuvel @ 2020-05-04 11:34 ` Will Deacon 2 siblings, 0 replies; 11+ messages in thread From: Will Deacon @ 2020-05-04 11:34 UTC (permalink / raw) To: Ard Biesheuvel, linux-arm-kernel Cc: catalin.marinas, Will Deacon, Robin Murphy, Sudeep Holla, Lorenzo Pieralisi, Pankaj Bansal, linux-acpi, Hanjun Guo On Fri, 1 May 2020 18:10:12 +0200, Ard Biesheuvel wrote: > 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. > > [...] Applied to arm64 (for-next/acpi), thanks! [1/2] Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" https://git.kernel.org/arm64/c/6d3b29d07c3c [2/2] ACPI/IORT: work around num_ids ambiguity https://git.kernel.org/arm64/c/539979b6ec62 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-07 13:18 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-01 16:10 [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk Ard Biesheuvel 2020-05-01 16:10 ` [PATCH v2 1/2] Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" Ard Biesheuvel 2020-05-01 16:10 ` [PATCH v2 2/2] ACPI/IORT: work around num_ids ambiguity Ard Biesheuvel 2020-05-04 4:32 ` Hanjun Guo 2020-05-04 7:36 ` Ard Biesheuvel 2020-05-06 12:44 ` Hanjun Guo 2020-05-06 12:55 ` Will Deacon 2020-05-07 13:18 ` Hanjun Guo 2020-05-06 12:43 ` Hanjun Guo 2020-05-04 11:00 ` Lorenzo Pieralisi 2020-05-04 11:34 ` [PATCH v2 0/2] ACPI/IORT: rework num_ids off-by-one quirk Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).