From: Robin Murphy <robin.murphy@arm.com>
To: Ard Biesheuvel <ardb@kernel.org>, linux-arm-kernel@lists.infradead.org
Cc: linux-acpi@vger.kernel.org, Hanjun Guo <guohanjun@huawei.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Pankaj Bansal <pankaj.bansal@nxp.com>,
Will Deacon <will@kernel.org>,
Sudeep Holla <sudeep.holla@arm.com>,
Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH RFC 2/2] ACPI/IORT: work around num_ids ambiguity
Date: Fri, 1 May 2020 11:55:13 +0100 [thread overview]
Message-ID: <e3c7bdab-a2b0-d7c9-5c7b-eee680509338@arm.com> (raw)
In-Reply-To: <20200501095843.25401-3-ardb@kernel.org>
On 2020-05-01 10:58 am, 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.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> drivers/acpi/arm64/iort.c | 23 +++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 98be18266a73..d826dd9dc4c5 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -316,10 +316,19 @@ 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;
>
> *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 +413,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 +448,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);
> + if (!rc)
> break;
This needs a big FW_BUG splat in the case where it did find an overlap.
Ideally we'd also enforce that the other half of must be the first entry
of another range, but perhaps we're into diminishing returns by that point.
If we silently fix things up, then people will continue to write broken
tables without even realising, new OSes will have to implement the same
mechanism because vendors will have little interest in changing things
that have worked "correctly" with Linux for years, and we've effectively
achieved a de-facto redefinition of the spec. Making our end of the
interface robust is obviously desirable, but there still needs to be
*some* incentive for the folks on the other end to get it right.
Robin.
> + if (rc == -EAGAIN)
> + out_ref = map->output_reference;
> }
>
> - if (i == node->mapping_count)
> + if (i == node->mapping_count && rc != -EAGAIN)
> goto fail_map;
>
> node = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
> - map->output_reference);
> + rc ? out_ref : map->output_reference);
> }
>
> fail_map:
>
next prev parent reply other threads:[~2020-05-01 10:55 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-01 9:58 [PATCH RFC 0/2] ACPI/IORT: rework num_ids off-by-one quirk Ard Biesheuvel
2020-05-01 9:58 ` [PATCH RFC 1/2] Revert "ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map()" Ard Biesheuvel
2020-05-01 9:58 ` [PATCH RFC 2/2] ACPI/IORT: work around num_ids ambiguity Ard Biesheuvel
2020-05-01 10:55 ` Robin Murphy [this message]
2020-05-01 11:41 ` Ard Biesheuvel
2020-05-01 12:31 ` Robin Murphy
2020-05-01 13:10 ` Ard Biesheuvel
2020-05-01 13:49 ` Lorenzo Pieralisi
2020-05-01 14:35 ` Ard Biesheuvel
2020-05-01 14:57 ` Robin Murphy
2020-05-01 14:13 ` Robin Murphy
2020-05-01 14:26 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e3c7bdab-a2b0-d7c9-5c7b-eee680509338@arm.com \
--to=robin.murphy@arm.com \
--cc=ardb@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=guohanjun@huawei.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=pankaj.bansal@nxp.com \
--cc=sudeep.holla@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).