On Fri, 1 May 2020 at 10:54, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> > > > > > Link: https://lore.kernel.org/linux-acpi/20191215203303.29811-1-pankaj.bansal@xxxxxxx/ > > > > > Signed-off-by: Hanjun Guo <guohanjun@xxxxxxxxxx> > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> > > > > > Cc: Pankaj Bansal <pankaj.bansal@xxxxxxx> > > > > > Cc: Will Deacon <will@xxxxxxxxxx> > > > > > Cc: Sudeep Holla <sudeep.holla@xxxxxxx> > > > > > Cc: Catalin Marinas <catalin.marinas@xxxxxxx> > > > > > Cc: Robin Murphy <robin.murphy@xxxxxxx> > > > > > --- > > > > > > > > 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@xxxxxxxxxx/ > > > > > > 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.
