On 2020/1/7 1:19, 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 >> 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 >> >> 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/ >> [2]: https://github.com/tianocore/edk2-platforms >> >> Cc: Pankaj Bansal <pankaj.bansal@xxxxxxx> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> >> Signed-off-by: Hanjun Guo <guohanjun@xxxxxxxxxx> >> --- >> >> 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 >> + * one, but the IORT code ingored the "minus one", and some firmware >> + * did that too, so apply a workaround here to keep compatible with >> + * both new and old versions of the firmware. >> + */ >> + 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: > > map_max = map->input_base + map->id_count; > if (apply_id_count_workaround) > map_max--; Much better, thanks! I will update my patch. Thanks Hanjun
