| Message ID | 20191215203303.29811-1-pankaj.bansal@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPI/IORT: fix the iort_id_map function | expand |
Hi Pankaj, On 2019/12/15 23:12, Pankaj Bansal wrote: > As per http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf > in ID mappings: > 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. > > 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? Thanks Hanjun
Hi Hanjun, Thanks for replying. Please find my response inline > -----Original Message----- > From: Hanjun Guo <guohanjun@huawei.com> > Sent: Monday, 16 December, 2019 09:19 AM > To: Pankaj Bansal <pankaj.bansal@nxp.com>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; > Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > Jonathan Cameron <jonathan.cameron@huawei.com> > Subject: Re: [PATCH] ACPI/IORT: fix the iort_id_map function > > Hi Pankaj, > > On 2019/12/15 23:12, Pankaj Bansal wrote: > > As per > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfoc > > > enter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0049d%2FDEN0049D_IO > _Rema > > > pping_Table.pdf&data=02%7C01%7Cpankaj.bansal%40nxp.com%7C78d > 82a560 > > > 5714219196008d781db06a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0% > 7C1%7C6 > > > 37120650018983814&sdata=%2FRhATUKx%2FA2gPEx%2BNY9X%2F7kqV > CrEeRnbE% > > 2B2qlTkdGDc%3D&reserved=0 > > in ID mappings: > > 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) > > Thanks > Hanjun Regards, Pankaj Bansal
On 2019/12/16 13:14, Pankaj Bansal wrote: > Hi Hanjun, > > Thanks for replying. Please find my response inline > >> Hi Pankaj, >> >> On 2019/12/15 23:12, Pankaj Bansal wrote: >>> As per >>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfoc >>> >> enter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0049d%2FDEN0049D_IO >> _Rema >>> >> pping_Table.pdf&data=02%7C01%7Cpankaj.bansal%40nxp.com%7C78d >> 82a560 >>> >> 5714219196008d781db06a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0% >> 7C1%7C6 >>> >> 37120650018983814&sdata=%2FRhATUKx%2FA2gPEx%2BNY9X%2F7kqV >> CrEeRnbE% >>> 2B2qlTkdGDc%3D&reserved=0 >>> in ID mappings: >>> 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. 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. Thanks Hanjun
> -----Original Message----- > From: Hanjun Guo <guohanjun@huawei.com> > Sent: Monday, 16 December, 2019 05:24 PM > To: Pankaj Bansal <pankaj.bansal@nxp.com>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; > Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > Jonathan Cameron <jonathan.cameron@huawei.com> > Subject: Re: [PATCH] ACPI/IORT: fix the iort_id_map function > > On 2019/12/16 13:14, Pankaj Bansal wrote: > > Hi Hanjun, > > > > Thanks for replying. Please find my response inline > > > >> Hi Pankaj, > >> > >> On 2019/12/15 23:12, Pankaj Bansal wrote: > >>> As per > >>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finf > >>> oc > >>> > >> > enter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0049d%2FDEN0049D_IO > >> _Rema > >>> > >> > pping_Table.pdf&data=02%7C01%7Cpankaj.bansal%40nxp.com%7C78d > >> 82a560 > >>> > >> > 5714219196008d781db06a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0% > >> 7C1%7C6 > >>> > >> > 37120650018983814&sdata=%2FRhATUKx%2FA2gPEx%2BNY9X%2F7kqV > >> CrEeRnbE% > >>> 2B2qlTkdGDc%3D&reserved=0 > >>> in ID mappings: > >>> 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. > > 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. > > Thanks > Hanjun
> -----Original Message----- > From: Pankaj Bansal > Sent: Monday, 16 December, 2019 10:48 PM > To: Hanjun Guo <guohanjun@huawei.com>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; > Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > Jonathan Cameron <jonathan.cameron@huawei.com> > Subject: RE: [PATCH] ACPI/IORT: fix the iort_id_map function > > > -----Original Message----- > > From: Hanjun Guo <guohanjun@huawei.com> > > Sent: Monday, 16 December, 2019 05:24 PM > > To: Pankaj Bansal <pankaj.bansal@nxp.com>; Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com>; Sudeep Holla <sudeep.holla@arm.com>; > > Rafael J . Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org> > > Cc: linux-acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > Jonathan Cameron <jonathan.cameron@huawei.com> > > Subject: Re: [PATCH] ACPI/IORT: fix the iort_id_map function > > > > On 2019/12/16 13:14, Pankaj Bansal wrote: > > > Hi Hanjun, > > > > > > Thanks for replying. Please find my response inline > > > > > >> Hi Pankaj, > > >> > > >> On 2019/12/15 23:12, Pankaj Bansal wrote: > > >>> As per > > >>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fi > > >>> nf > > >>> oc > > >>> > > >> > > > enter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0049d%2FDEN0049D_IO > > >> _Rema > > >>> > > >> > > > pping_Table.pdf&data=02%7C01%7Cpankaj.bansal%40nxp.com%7C78d > > >> 82a560 > > >>> > > >> > > 5714219196008d781db06a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0% > > >> 7C1%7C6 > > >>> > > >> > > > 37120650018983814&sdata=%2FRhATUKx%2FA2gPEx%2BNY9X%2F7kqV > > >> CrEeRnbE% > > >>> 2B2qlTkdGDc%3D&reserved=0 > > >>> in ID mappings: > > >>> 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. > > > > > 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. > > > > > Thanks > > Hanjun
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
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 33f71983e001..b9b108d0ca0b 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -315,7 +315,7 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in, } if (rid_in < map->input_base || - (rid_in >= map->input_base + map->id_count)) + (rid_in > map->input_base + map->id_count)) return -ENXIO; *rid_out = map->output_base + (rid_in - map->input_base);
As per http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf in ID mappings: Number of IDs = The number of IDs in the range minus one. Therefore, it's valid for ID mapping to contain single device mapping which would have Number of IDs field 0. The iort_id_map doesn't handle this case, fix this case. Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com> --- drivers/acpi/arm64/iort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)