Hi Pankaj, On 2019/12/17 19:58, Pankaj Bansal wrote: [...] >>>>>> Number of IDs = The number of IDs in the range minus one. >>>>> >>>>> Hmm, the spec is confusing, the spec may need to be updated, for >>>>> example, for a PCI bus, device ID + function ID will take 8 bits >>>>> and will be 256 IDs for that PCI bus, not sure why we need to minus one. >>>>> >>>> >>>> I agree that this "minus one" thing is confusing. Not sure why It >>>> was put in the spec like that. I guess they wanted the number of IDs >>>> to be 0 >>> based instead of 1 based. >>>> >>>>>> >>>>>> Therefore, it's valid for ID mapping to contain single device >>>>>> mapping which would have Number of IDs field 0. >>>>> >>>>> Why not use single mapping flag for this case? >>>> >>>> Actually single mapping flag doesn't mean that there is single >>>> mapping in >>> an ID mapping. >>>> It means that Input ID should not be considered when looking for >>>> Output >>> ID. >>>> If we put single id flag, then we cannot have a case where we have >>>> an array of single mappings for one device. >>>> e.g. an array of single mappings for one PCIe Root Complex, where we >>>> have a unique output ID for a unique BDF(Input ID) >>> >>> Agreed, single mapping flag is not working for multi-entris of single >> mappings. >>> >>> Do you have a real use case for this fix? I'm thinking if we will >>> break any delivered platforms with this patch applied, since this code >>> is not changed from 2016, and it's the key logic for mapping the IDs. >> >> We have this use case in our platform NXP LX2160A, where we provide the >> array of single mappings in IORT table. Actually we can only have limited >> number of output IDs for PCIe devices, so we allocate unique output ID to >> each BDF connected to a PCIe root complex and pass these IDs in IORT table. Thanks for the detail information, it's quite useful. >> >>> >>> I checked Hisilicon's ARM64 server platform, Number of IDs is equal to >>> the number of IDs in the range in the firmware, which is not doing the >>> same as the spec said, but (rid_in > map->input_base + map->id_count) >>> is still valid with this patch applied, not sure for other platforms. >> >> I don't think that this patch would break any platform which has IORT table >> defined as per spec. > > Let me rephase this to persuade you. This patch is *increasing* the allowed input > IDs. Therefore, this patch would *include* more platforms and none of the existing > Platforms can be affected by it, because already their Input IDs would fall in the allowed IDs. Unfortunately it breaks systems (Number of IDs = The number of IDs in the range) in this way: PCI hostbridge mapping entry 1: Input base: 0x1000 ID Count: 0x100 Output base: 0x1000 Output reference: 0xC4 PCI hostbridge mapping entry 2: Input base: 0x1100 ID Count: 0x100 Output base: 0x2000 Output reference: 0xD4 Without your patch, Requester ID 0x1100 will be excluded to mapping entry1, and will map to entry 2, pointing to ITS or SMMU 0xD4; With your patch, will mapping to ITS or SMMU 0xC4. Correct me if I'm wrong. Also will not work if we update the firmware but leave the kernel not updated. Your patch is doing the right thing, but I need to figure out how to avoid breaking the exiting system as well, the basic idea is to workaroud the firmware issue with some OEM information such as OEM ID/Table ID/Oem Revision in IORT table, please shout if you have some other ideas. Thanks Hanjun
