[PATCH] ACPI/IORT: fix the iort_id_map function

[PATCH] ACPI/IORT: fix the iort_id_map function

[Pankaj Bansal]

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(-)

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);
-- 
2.17.1

Re: [PATCH] ACPI/IORT: fix the iort_id_map function

[Hanjun Guo]

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

RE: [PATCH] ACPI/IORT: fix the iort_id_map function

[Pankaj Bansal]

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&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7C78d
> 82a560
> >
> 5714219196008d781db06a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C1%7C6
> >
> 37120650018983814&amp;sdata=%2FRhATUKx%2FA2gPEx%2BNY9X%2F7kqV
> CrEeRnbE%
> > 2B2qlTkdGDc%3D&amp;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

Re: [PATCH] ACPI/IORT: fix the iort_id_map function

[Hanjun Guo]

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

RE: [PATCH] ACPI/IORT: fix the iort_id_map function

[Pankaj Bansal]

> -----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&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7C78d
> >> 82a560
> >>>
> >>
> 5714219196008d781db06a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> >> 7C1%7C6
> >>>
> >>
> 37120650018983814&amp;sdata=%2FRhATUKx%2FA2gPEx%2BNY9X%2F7kqV
> >> CrEeRnbE%
> >>> 2B2qlTkdGDc%3D&amp;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

RE: [PATCH] ACPI/IORT: fix the iort_id_map function

[Pankaj Bansal]

> -----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&amp;data=02%7C01%7Cpankaj.bansal%40nxp.com%7C78d
> > >> 82a560
> > >>>
> > >>
> > 5714219196008d781db06a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> > >> 7C1%7C6
> > >>>
> > >>
> >
> 37120650018983814&amp;sdata=%2FRhATUKx%2FA2gPEx%2BNY9X%2F7kqV
> > >> CrEeRnbE%
> > >>> 2B2qlTkdGDc%3D&amp;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

Re: [PATCH] ACPI/IORT: fix the iort_id_map function

[Hanjun Guo]

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