On Mon, 4 May 2020 at 06:32, Hanjun Guo <guohanjun@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
