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
