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

Lorenzo Pieralisi
2020-01-09T16:02:33Z

On Mon, Jan 06, 2020 at 05:19:32PM +0000, Robin Murphy wrote:
> On 30/12/2019 12:27 pm, Hanjun Guo wrote:
> > 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

Why is it confusing ? Because we botched the kernel code :) ?

> > 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

Add a Link: tag, when I read a commit log I want to have a reference
to the patches relevant to the commit in question (which in turn
will help understand what Pankaj suggested).

> > 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/

Add a Link: tag to a message-ID

> > [2]: https://github.com/tianocore/edk2-platforms

It is useless in a commit log - this is a moving target.

I can rewrite this commit log if you think it is faster.

> >
> > Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> > ---
> >
> > RFC->v1:
> > - Print warning when matched the workaround info, suggested by Pankaj.
> >
> > drivers/acpi/arm64/iort.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 33f7198..60eb10d 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -298,6 +298,42 @@ 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 int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> > u32 *rid_out)
> > {
> > @@ -314,9 +350,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

Section, page, table number please, "IORT spec says" is too vague.

> > + * one, but the IORT code ingored the "minus one", and some firmware

s/ingored/ignored/

> > + * did that too, so apply a workaround here to keep compatible with
> > + * both new and old versions of the firmware.

It is not "new" vs "old" it is spec compliant vs non-spec compliant.

> > + */
> > + 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;
> > + }
>
> This seems needlessly repetitive and convoluted... how about refactoring to
> something like:

+1

>
> map_max = map->input_base + map->id_count;
> if (apply_id_count_workaround)
> map_max--;

You can even turn it into an inline function (ie iort_get_map_max())
with the comment above in it so that the quirk is isolated instead
of having it in the middle of iort_id_map().

I am fine either way. We need test coverage since I feel this may
break a number of systems (ie I don't think it should be merged as
a fix).

Lorenzo