Message ID | 1577708824-4873-1-git-send-email-guohanjun@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v1] ACPI/IORT: Workaround for IORT ID count "minus one" issue | expand |
+ On 30/12/2019 12:27, Guohanjun (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], Hi Hanjun, only > HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround > info table, Are you asserting that other platforms are ok on the basis that NumIds = large power of 2 - 1, e.g. 0xffff? Is this strictly proper? if we break other platforms, we can add that later. > I think that it would be better to audit others now as well as best as reasonably possible. There is somewhat limited coverage in [2]. Thanks, John > [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@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 > + * 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; > + } > > *rid_out = map->output_base + (rid_in - map->input_base); > return 0; > @@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void) > return; > } > > + iort_check_id_count_workaround(iort_table); > iort_init_platform_devices(); > } >
On 2020/1/2 19:18, John Garry wrote: > + > > On 30/12/2019 12:27, Guohanjun (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], > > Hi Hanjun, > > only >> HiSilicon HIP07/08 did wrong, so just add HIP07/08 to the workaround >> info table, > > Are you asserting that other platforms are ok on the basis that NumIds = large power of 2 - 1, e.g. 0xffff? Is this strictly proper? No, some platforms with no opensource ACPI tables, are not covered. > > if we break other platforms, we can add that later. >> > > I think that it would be better to audit others now as well as best as reasonably possible. There is somewhat limited coverage in [2]. I will Cc people form Mavell, Ampere, and Ard who is know Socionext very well, that's the best I can do. Thanks Hanjun
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@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 > + * 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--; ? Robin. > *rid_out = map->output_base + (rid_in - map->input_base); > return 0; > @@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void) > return; > } > > + iort_check_id_count_workaround(iort_table); > iort_init_platform_devices(); > } >
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@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 >> + * 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
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
On 2020/1/10 0:02, Lorenzo Pieralisi wrote: > 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 :) ? I think 'minus one' is not bringing any benefit :) > >>> 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. That will be very helpful, please do so, thanks! > >>> >>> 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. Agreed. > >>> + */ >>> + 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 vote for this one, it's self-contained. > > 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). Will you resend this patch with commit log and the updated code? or let me do that? Both are ok to me, let's get it tested for longer time if we merge it ASAP. Thanks Hanjun
On Fri, Jan 10, 2020 at 02:22:22PM +0800, Hanjun Guo wrote: [...] > > 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). > > Will you resend this patch with commit log and the updated code? or > let me do that? Both are ok to me, let's get it tested for longer time > if we merge it ASAP. I will write the commit log in reply to the original patch, please update the code and repost the whole thing (it is better you repost it to avoid confusing Catalin and Will). This technically is not a fix; we can try to get it into v5.6 but I am a bit nervous since it can break other platforms, we have to fix it though so better sooner than later. Thanks, Lorenzo
On 2020-01-10 6:22 am, Hanjun Guo wrote: > On 2020/1/10 0:02, Lorenzo Pieralisi wrote: >> 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 :) ? > > I think 'minus one' is not bringing any benefit :) Well, in order to describe a 1:1 mapping of the entire possible ID space, the alternative would have to be to overload the otherwise-nonsensical value of 0 to mean 2^32, which I would argue is an even more non-obvious inconsistency. Encoding strictly positive values as 'value - 1' is a relatively common thing (at least in hardware design). Robin.
On Mon, Dec 30, 2019 at 08:27:04PM +0800, 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@nxp.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Signed-off-by: Hanjun Guo <guohanjun@huawei.com> Commit log rewritten below - please update the code (and check the log) as per this thread discussion. Thanks, Lorenzo -- >8 -- From bc766b2913008519bdb59bbc38907451e8bac0d4 Mon Sep 17 00:00:00 2001 From: Hanjun Guo <guohanjun@huawei.com> Date: Mon, 30 Dec 2019 20:27:04 +0800 Subject: [PATCH] ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map() The IORT specification [0] (Section 3, table 4, page 9) defines the 'Number of IDs' as 'The number of IDs in the range minus one'. However, the IORT ID mapping function iort_id_map() treats the 'Number of IDs' field as if it were the full IDs mapping count, with the following check in place to detect out of boundary input IDs: InputID >= Input base + Number of IDs This check is flawed in that it considers the 'Number of IDs' field as the full number of IDs mapping and disregards the 'minus one' from the IDs count. The correct check in iort_id_map() should be implemented as: InputID > Input base + Number of IDs this implements the specification correctly but unfortunately it breaks existing firmwares that erroneously set the 'Number of IDs' as the full IDs mapping count rather than IDs mapping count minus one. e.g. 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 InputID 0x1100 and with the correct InputID check in place in iort_id_map() the kernel would map the InputID to ITS 0xC4 not 0xD4 as it would be expected. Therefore, to keep supporting existing flawed firmwares, introduce a workaround that instructs the kernel to use the old InputID range check logic in iort_id_map(), so that we can support both firmwares written with the flawed 'Number of IDs' logic and the correct one as defined in the specifications. [0]: http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf Reported-by: Pankaj Bansal <pankaj.bansal@nxp.com> Link: https://lore.kernel.org/linux-acpi/20191215203303.29811-1-pankaj.bansal@nxp.com/ Signed-off-by: Hanjun Guo <guohanjun@huawei.com> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Pankaj Bansal <pankaj.bansal@nxp.com> Cc: Will Deacon <will@kernel.org> Cc: Sudeep Holla <sudeep.holla@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Robin Murphy <robin.murphy@arm.com> --- 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 33f71983e001..60eb10d46d2b 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; + } *rid_out = map->output_base + (rid_in - map->input_base); return 0; @@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void) return; } + iort_check_id_count_workaround(iort_table); iort_init_platform_devices(); }
On 2020/1/10 20:11, Lorenzo Pieralisi wrote: > On Mon, Dec 30, 2019 at 08:27:04PM +0800, 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@nxp.com> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Signed-off-by: Hanjun Guo <guohanjun@huawei.com> > > Commit log rewritten below - please update the code (and check the > log) as per this thread discussion. Thank you, I will resend it ASAP. Thanks Hanjun
On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote: > +}; > + > +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, > + } > +}; Am I right in saying that any future BIOS for these chipsets will have to continue to have buggy firmware? If so, it's unfortunate. Thanks, John > + > +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; > + } > + } > +}
On 2020/1/13 17:34, John Garry wrote: > On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote: >> +}; >> + >> +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, >> + } >> +}; > > Am I right in saying that any future BIOS for these chipsets will have to continue to have buggy firmware? If so, it's unfortunate. For better compatibility, I would say yes :( For example, if you fix that in the firmware, and update the IORT revision number, then it will run pretty good on new version of the kernel, but not on old version of kernel without the backporting of this patch. Thanks Hanjun
On 14/01/2020 07:19, Hanjun Guo wrote: > On 2020/1/13 17:34, John Garry wrote: >> On 30/12/2019 12:27, Guohanjun (Hanjun Guo) wrote: >>> +}; >>> + >>> +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, >>> + } >>> +}; >> >> Am I right in saying that any future BIOS for these chipsets will have to continue to have buggy firmware? If so, it's unfortunate. > > For better compatibility, I would say yes :( > > For example, if you fix that in the firmware, and update > the IORT revision number, then it will run pretty good > on new version of the kernel, but not on old version of > kernel without the backporting of this patch. ok, so that seems to be a trade off then. Having to backport introduces a risk. So then it might be good to add a comment to ID count members in open-source edk2-platforms hip07 and hip08 IORTs to mention it is buggy, so not to be copied as a reference. Cheers, John
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; + } *rid_out = map->output_base + (rid_in - map->input_base); return 0; @@ -1631,5 +1679,6 @@ void __init acpi_iort_init(void) return; } + iort_check_id_count_workaround(iort_table); iort_init_platform_devices(); }
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@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(-)