Message ID | 1579004051-48797-1-git-send-email-guohanjun@huawei.com (mailing list archive) |
---|---|
State | Mainlined |
Commit | 3c23b83a88d00383e1d498cfa515249aa2fe0238 |
Headers | show |
Series | [v2] ACPI/IORT: Fix 'Number of IDs' handling in iort_id_map() | expand |
On Tue, Jan 14, 2020 at 08:14:11PM +0800, Hanjun Guo wrote: > 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> > --- I'm a bit confused about the SoB chain here and which tree this is targetting. Lorenzo? Will
On Fri, Jan 17, 2020 at 12:14:49PM +0000, Will Deacon wrote: > On Tue, Jan 14, 2020 at 08:14:11PM +0800, Hanjun Guo wrote: > > 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> > > --- > > I'm a bit confused about the SoB chain here and which tree this is > targetting. > > Lorenzo? Hi Will, sorry about that. It targets arm64 - previously wasn't addressed to you and Catalin: https://lore.kernel.org/linux-arm-kernel/1577708824-4873-1-git-send-email-guohanjun@huawei.com/ I rewrote the commit log and asked Hanjun to repost it to target an arm64 merge. Having said that, this patch makes me nervous, it can break on platforms that have non-compliant firmware, I wonder whether it is best to leave it in -next for a whole cycle (I can send it to -next) to catch any fall-out rather than targeting v5.6 given that technically is _not_ a fix (we may even have to revert it - it requires coverage on all ARM64 ACPI systems). What do you think ? Lorenzo
On Fri, Jan 17, 2020 at 12:32:26PM +0000, Lorenzo Pieralisi wrote: > On Fri, Jan 17, 2020 at 12:14:49PM +0000, Will Deacon wrote: > > On Tue, Jan 14, 2020 at 08:14:11PM +0800, Hanjun Guo wrote: > > > 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> > > > --- > > > > I'm a bit confused about the SoB chain here and which tree this is > > targetting. > > > > Lorenzo? > > sorry about that. It targets arm64 - previously wasn't addressed > to you and Catalin: > > https://lore.kernel.org/linux-arm-kernel/1577708824-4873-1-git-send-email-guohanjun@huawei.com/ > > I rewrote the commit log and asked Hanjun to repost it to target an arm64 > merge. No need to apologise, just trying to figure out what's going on. Thanks for the explanation. > Having said that, this patch makes me nervous, it can break on platforms > that have non-compliant firmware, I wonder whether it is best to leave > it in -next for a whole cycle (I can send it to -next) to catch any > fall-out rather than targeting v5.6 given that technically is _not_ a > fix (we may even have to revert it - it requires coverage on all ARM64 > ACPI systems). > > What do you think ? My experience with linux-next is that it doesn't get tonnes of runtime testing but rather lots of build testing, so I'd be inclined to queue this for 5.6 (i.e. for the upcoming merge window) and revert it the moment it causes a problem. I'll stick it on its own branch so we can also drop it if something comes up before then. Will
On Fri, 17 Jan 2020 at 13:32, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Fri, Jan 17, 2020 at 12:14:49PM +0000, Will Deacon wrote: > > On Tue, Jan 14, 2020 at 08:14:11PM +0800, Hanjun Guo wrote: > > > 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> > > > --- > > > > I'm a bit confused about the SoB chain here and which tree this is > > targetting. > > > > Lorenzo? > > Hi Will, > > sorry about that. It targets arm64 - previously wasn't addressed > to you and Catalin: > > https://lore.kernel.org/linux-arm-kernel/1577708824-4873-1-git-send-email-guohanjun@huawei.com/ > > I rewrote the commit log and asked Hanjun to repost it to target an arm64 > merge. > > Having said that, this patch makes me nervous, it can break on platforms > that have non-compliant firmware, I wonder whether it is best to leave > it in -next for a whole cycle (I can send it to -next) to catch any > fall-out rather than targeting v5.6 given that technically is _not_ a > fix (we may even have to revert it - it requires coverage on all ARM64 > ACPI systems). > > What do you think ? > I just ran into this while playing with the LX2160 I received this week. I wonder if it would be better to detect the failure case dynamically, rather than having these hardcoded quirks. It should be rather straightforward to detect overlaps at the edges of these multi-range mappings, in which case we could just let the spurious one (living at the end of the region) be superseded by the correct one (living at the start of the next region).
On Fri, May 01, 2020 at 10:30:11AM +0200, Ard Biesheuvel wrote: > On Fri, 17 Jan 2020 at 13:32, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Fri, Jan 17, 2020 at 12:14:49PM +0000, Will Deacon wrote: > > > On Tue, Jan 14, 2020 at 08:14:11PM +0800, Hanjun Guo wrote: > > > > 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> > > > > --- > > > > > > I'm a bit confused about the SoB chain here and which tree this is > > > targetting. > > > > > > Lorenzo? > > > > Hi Will, > > > > sorry about that. It targets arm64 - previously wasn't addressed > > to you and Catalin: > > > > https://lore.kernel.org/linux-arm-kernel/1577708824-4873-1-git-send-email-guohanjun@huawei.com/ > > > > I rewrote the commit log and asked Hanjun to repost it to target an arm64 > > merge. > > > > Having said that, this patch makes me nervous, it can break on platforms > > that have non-compliant firmware, I wonder whether it is best to leave > > it in -next for a whole cycle (I can send it to -next) to catch any > > fall-out rather than targeting v5.6 given that technically is _not_ a > > fix (we may even have to revert it - it requires coverage on all ARM64 > > ACPI systems). > > > > What do you think ? > > > > I just ran into this while playing with the LX2160 I received this week. Side note: that firmware must be updatable or there is something I am missing in relation to the ongoing ITS/SMMU mapping discussions. > I wonder if it would be better to detect the failure case dynamically, > rather than having these hardcoded quirks. It should be rather > straightforward to detect overlaps at the edges of these multi-range > mappings, in which case we could just let the spurious one (living at > the end of the region) be superseded by the correct one (living at the > start of the next region). This could be done I think but probably requires some boot time parsing to create some structure defining ranges (to avoid running the logic you describe above every time a device has to be mapped). Given that I have not heard of any regressions on the existing crop of platforms and the one you mentioned has FW that is not set in stone I think we can live with the quirk mechanism for the time being, what do you think ? Thanks, Lorenzo
On Fri, 1 May 2020 at 10:54, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Fri, May 01, 2020 at 10:30:11AM +0200, Ard Biesheuvel wrote: > > On Fri, 17 Jan 2020 at 13:32, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > On Fri, Jan 17, 2020 at 12:14:49PM +0000, Will Deacon wrote: > > > > On Tue, Jan 14, 2020 at 08:14:11PM +0800, Hanjun Guo wrote: > > > > > 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> > > > > > --- > > > > > > > > I'm a bit confused about the SoB chain here and which tree this is > > > > targetting. > > > > > > > > Lorenzo? > > > > > > Hi Will, > > > > > > sorry about that. It targets arm64 - previously wasn't addressed > > > to you and Catalin: > > > > > > https://lore.kernel.org/linux-arm-kernel/1577708824-4873-1-git-send-email-guohanjun@huawei.com/ > > > > > > I rewrote the commit log and asked Hanjun to repost it to target an arm64 > > > merge. > > > > > > Having said that, this patch makes me nervous, it can break on platforms > > > that have non-compliant firmware, I wonder whether it is best to leave > > > it in -next for a whole cycle (I can send it to -next) to catch any > > > fall-out rather than targeting v5.6 given that technically is _not_ a > > > fix (we may even have to revert it - it requires coverage on all ARM64 > > > ACPI systems). > > > > > > What do you think ? > > > > > > > I just ran into this while playing with the LX2160 I received this week. > > Side note: that firmware must be updatable or there is something I am > missing in relation to the ongoing ITS/SMMU mapping discussions. > Sure. But they are following the spec, and they use num_ids = 0x0 for regions consisting of a single mapping. This is completely broken without this patch. > > I wonder if it would be better to detect the failure case dynamically, > > rather than having these hardcoded quirks. It should be rather > > straightforward to detect overlaps at the edges of these multi-range > > mappings, in which case we could just let the spurious one (living at > > the end of the region) be superseded by the correct one (living at the > > start of the next region). > > This could be done I think but probably requires some boot time parsing > to create some structure defining ranges (to avoid running the logic you > describe above every time a device has to be mapped). > It could be done much simpler than that: if iort_id_map() matches on the last value of a region with size > 1 (so num_ids > 0), we return the mapping but don't exit the loop. If we match it again, we use that value, otherwise we use the tentative value from the first iteration. > Given that I have not heard of any regressions on the existing crop > of platforms and the one you mentioned has FW that is not set in stone > I think we can live with the quirk mechanism for the time being, > what do you think ? > The more quirks we keep, the harder it becomes to push back on new ones. So if we can fix this cleanly, I'd much prefer it.
On Fri, May 01, 2020 at 11:02:48AM +0200, Ard Biesheuvel wrote: > On Fri, 1 May 2020 at 10:54, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Fri, May 01, 2020 at 10:30:11AM +0200, Ard Biesheuvel wrote: > > > On Fri, 17 Jan 2020 at 13:32, Lorenzo Pieralisi > > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > > > On Fri, Jan 17, 2020 at 12:14:49PM +0000, Will Deacon wrote: > > > > > On Tue, Jan 14, 2020 at 08:14:11PM +0800, Hanjun Guo wrote: > > > > > > 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. [...] > > > I just ran into this while playing with the LX2160 I received this week. > > > > Side note: that firmware must be updatable or there is something I am > > missing in relation to the ongoing ITS/SMMU mapping discussions. > > > > Sure. But they are following the spec, and they use num_ids = 0x0 for > regions consisting of a single mapping. This is completely broken > without this patch. Yes sure - I thought you were saying that the FW has issues with the *current* kernel whereas you are asking if this fix can be rewritten to remove the quirking mechanism - that's always a good aim :) > > > I wonder if it would be better to detect the failure case dynamically, > > > rather than having these hardcoded quirks. It should be rather > > > straightforward to detect overlaps at the edges of these multi-range > > > mappings, in which case we could just let the spurious one (living at > > > the end of the region) be superseded by the correct one (living at the > > > start of the next region). > > > > This could be done I think but probably requires some boot time parsing > > to create some structure defining ranges (to avoid running the logic you > > describe above every time a device has to be mapped). > > > > It could be done much simpler than that: if iort_id_map() matches on > the last value of a region with size > 1 (so num_ids > 0), we return > the mapping but don't exit the loop. If we match it again, we use that > value, otherwise we use the tentative value from the first iteration. We need to see how the code will look like in both cases, again, I am not a priori against implementing it. Instead of relying on ranges, we can precompute structures to define for a device which mapping+mapping_index should be ignored, this will save us from doing it dynamically (still require some additional data). > > Given that I have not heard of any regressions on the existing crop > > of platforms and the one you mentioned has FW that is not set in stone > > I think we can live with the quirk mechanism for the time being, > > what do you think ? > > > > The more quirks we keep, the harder it becomes to push back on new ones. > > So if we can fix this cleanly, I'd much prefer it. I agree with that, more so given that this is not impossible to rewrite more cleanly. Thanks, Lorenzo
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 33f7198..6078064 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -298,6 +298,59 @@ 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 inline u32 iort_get_map_max(struct acpi_iort_id_mapping *map) +{ + u32 map_max = map->input_base + map->id_count; + + /* + * The IORT specification revision D (Section 3, table 4, page 9) says + * Number of IDs = The number of IDs in the range minus one, but the + * IORT code ignored the "minus one", and some firmware did that too, + * so apply a workaround here to keep compatible with both the spec + * compliant and non-spec compliant firmwares. + */ + if (apply_id_count_workaround) + map_max--; + + return map_max; +} + static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, u32 *rid_out) { @@ -314,8 +367,7 @@ 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)) + if (rid_in < map->input_base || rid_in > iort_get_map_max(map)) return -ENXIO; *rid_out = map->output_base + (rid_in - map->input_base); @@ -1631,5 +1683,6 @@ void __init acpi_iort_init(void) return; } + iort_check_id_count_workaround(iort_table); iort_init_platform_devices(); }