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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel