hw/arm/virt-acpi-build.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
The IORT doc defines "Number of IDs" ("id_count" in the virt-acpi-build)
to be "the number of IDs in the range minus one". Otherwise, Linux kernel
reports "conflicting mapping for input ID" FW_BUG at the overlapped ID.
Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
Changelog
v2:
* Moved "-1" to the same line of id_count calculation
* Added "+1" to the next_range.input_base calculation
v1:
https://lore.kernel.org/all/20240613234802.828265-1-nicolinc@nvidia.com/
hw/arm/virt-acpi-build.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index c3ccfef026..631f2c6d04 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -243,7 +243,8 @@ iort_host_bridges(Object *obj, void *opaque)
AcpiIortIdMapping idmap = {
.input_base = min_bus << 8,
- .id_count = (max_bus - min_bus + 1) << 8,
+ /* id_count is the number of IDs in the range minus one */
+ .id_count = ((max_bus - min_bus + 1) << 8) - 1,
};
g_array_append_val(idmap_blob, idmap);
}
@@ -298,11 +299,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
if (next_range.input_base < idmap->input_base) {
- next_range.id_count = idmap->input_base - next_range.input_base;
+ /* id_count is the number of IDs in the range minus one */
+ next_range.id_count = idmap->input_base -
+ next_range.input_base - 1;
g_array_append_val(its_idmaps, next_range);
}
- next_range.input_base = idmap->input_base + idmap->id_count;
+ next_range.input_base = idmap->input_base + idmap->id_count + 1;
}
/* Append the last RC -> ITS ID mapping */
--
2.43.0On Mon, Jun 17, 2024 at 03:39:45PM -0700, Nicolin Chen wrote:
> The IORT doc defines "Number of IDs" ("id_count" in the virt-acpi-build)
> to be "the number of IDs in the range minus one". Otherwise, Linux kernel
> reports "conflicting mapping for input ID" FW_BUG at the overlapped ID.
>
> Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> Changelog
> v2:
> * Moved "-1" to the same line of id_count calculation
> * Added "+1" to the next_range.input_base calculation
> v1:
> https://lore.kernel.org/all/20240613234802.828265-1-nicolinc@nvidia.com/
>
> hw/arm/virt-acpi-build.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index c3ccfef026..631f2c6d04 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -243,7 +243,8 @@ iort_host_bridges(Object *obj, void *opaque)
>
> AcpiIortIdMapping idmap = {
> .input_base = min_bus << 8,
> - .id_count = (max_bus - min_bus + 1) << 8,
> + /* id_count is the number of IDs in the range minus one */
> + .id_count = ((max_bus - min_bus + 1) << 8) - 1,
> };
> g_array_append_val(idmap_blob, idmap);
> }
> @@ -298,11 +299,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>
> if (next_range.input_base < idmap->input_base) {
> - next_range.id_count = idmap->input_base - next_range.input_base;
> + /* id_count is the number of IDs in the range minus one */
> + next_range.id_count = idmap->input_base -
> + next_range.input_base - 1;
> g_array_append_val(its_idmaps, next_range);
> }
>
> - next_range.input_base = idmap->input_base + idmap->id_count;
> + next_range.input_base = idmap->input_base + idmap->id_count + 1;
> }
All this has to be written in the way that actually refers to the
spec. id_count is nowhere in the spec and one has to know that
in the end this is used by build_iort_id_mapping to figure out
where this comes from. Not good.
I think the best way is to fix build_iort_id_mapping:
make it subtract 1 from id_count.
Then change text from "Number of IDs" to "Number of IDs - The number of IDs in the range minus one"
You should also add the reference to IO Remapping Table document
near build_iort_id_mapping, it is currently unclear which table this
refers to.
Of couse this means the only correct use has to be tweaked so the change
to build_iort_id_mapping does not break it: 0xFFFF -> 0x10000 - but
that's good for readability, anyway.
> /* Append the last RC -> ITS ID mapping */
> --
> 2.43.0
On Tue, Jun 18, 2024 at 04:03:34PM -0400, Michael S. Tsirkin wrote: > On Mon, Jun 17, 2024 at 03:39:45PM -0700, Nicolin Chen wrote: > > - next_range.input_base = idmap->input_base + idmap->id_count; > > + next_range.input_base = idmap->input_base + idmap->id_count + 1; > > } > > > All this has to be written in the way that actually refers to the > spec. id_count is nowhere in the spec and one has to know that > in the end this is used by build_iort_id_mapping to figure out > where this comes from. Not good. > > I think the best way is to fix build_iort_id_mapping: > make it subtract 1 from id_count. > > Then change text from "Number of IDs" to "Number of IDs - The number of IDs in the range minus one" > > You should also add the reference to IO Remapping Table document > near build_iort_id_mapping, it is currently unclear which table this > refers to. > > Of couse this means the only correct use has to be tweaked so the change > to build_iort_id_mapping does not break it: 0xFFFF -> 0x10000 - but > that's good for readability, anyway. OK. I will send a v3 following that. Thanks Nicolin
On Mon, Jun 17, 2024 at 03:39:45PM -0700, Nicolin Chen wrote:
> The IORT doc defines "Number of IDs" ("id_count" in the virt-acpi-build)
> to be "the number of IDs in the range minus one". Otherwise, Linux kernel
> reports "conflicting mapping for input ID" FW_BUG at the overlapped ID.
>
> Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> Changelog
> v2:
> * Moved "-1" to the same line of id_count calculation
> * Added "+1" to the next_range.input_base calculation
> v1:
> https://lore.kernel.org/all/20240613234802.828265-1-nicolinc@nvidia.com/
>
> hw/arm/virt-acpi-build.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index c3ccfef026..631f2c6d04 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -243,7 +243,8 @@ iort_host_bridges(Object *obj, void *opaque)
>
> AcpiIortIdMapping idmap = {
> .input_base = min_bus << 8,
> - .id_count = (max_bus - min_bus + 1) << 8,
> + /* id_count is the number of IDs in the range minus one */
> + .id_count = ((max_bus - min_bus + 1) << 8) - 1,
> };
> g_array_append_val(idmap_blob, idmap);
> }
> @@ -298,11 +299,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>
> if (next_range.input_base < idmap->input_base) {
> - next_range.id_count = idmap->input_base - next_range.input_base;
> + /* id_count is the number of IDs in the range minus one */
> + next_range.id_count = idmap->input_base -
> + next_range.input_base - 1;
> g_array_append_val(its_idmaps, next_range);
What about other places where id_count is set?
> }
>
> - next_range.input_base = idmap->input_base + idmap->id_count;
> + next_range.input_base = idmap->input_base + idmap->id_count + 1;
> }
>
Given this was different previously, did you actually test with multiple ranges?
> /* Append the last RC -> ITS ID mapping */
> --
> 2.43.0
On Tue, Jun 18, 2024 at 05:49:58AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 17, 2024 at 03:39:45PM -0700, Nicolin Chen wrote:
> > The IORT doc defines "Number of IDs" ("id_count" in the virt-acpi-build)
> > to be "the number of IDs in the range minus one". Otherwise, Linux kernel
> > reports "conflicting mapping for input ID" FW_BUG at the overlapped ID.
> >
> > Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> > Changelog
> > v2:
> > * Moved "-1" to the same line of id_count calculation
> > * Added "+1" to the next_range.input_base calculation
> > v1:
> > https://lore.kernel.org/all/20240613234802.828265-1-nicolinc@nvidia.com/
> >
> > hw/arm/virt-acpi-build.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index c3ccfef026..631f2c6d04 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -243,7 +243,8 @@ iort_host_bridges(Object *obj, void *opaque)
> >
> > AcpiIortIdMapping idmap = {
> > .input_base = min_bus << 8,
> > - .id_count = (max_bus - min_bus + 1) << 8,
> > + /* id_count is the number of IDs in the range minus one */
> > + .id_count = ((max_bus - min_bus + 1) << 8) - 1,
> > };
> > g_array_append_val(idmap_blob, idmap);
> > }
> > @@ -298,11 +299,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> >
> > if (next_range.input_base < idmap->input_base) {
> > - next_range.id_count = idmap->input_base - next_range.input_base;
> > + /* id_count is the number of IDs in the range minus one */
> > + next_range.id_count = idmap->input_base -
> > + next_range.input_base - 1;
> > g_array_append_val(its_idmaps, next_range);
>
>
> What about other places where id_count is set?
There are only three places where id_count could be set:
hw/arm/virt-acpi-build.c:296: .id_count = ((max_bus - min_bus + 1) << 8) - 1,
hw/arm/virt-acpi-build.c:422: next_range.id_count = idmap->input_base -
hw/arm/virt-acpi-build.c:435: next_range.id_count = 0xFFFF - next_range.input_base;
This patch fixes two, while the last one is correct using 0xFFFF.
> > }
> >
> > - next_range.input_base = idmap->input_base + idmap->id_count;
> > + next_range.input_base = idmap->input_base + idmap->id_count + 1;
> > }
> >
>
> Given this was different previously, did you actually test with multiple ranges?
Tested by creating 5 buses: input_base increases by 0x400 while
id_count=0x2ff (0x300 - 1). ITS results look correct to me:
--------------build_iort: smmu_idmaps
DEBUG: build_iort_id_mapping: input_base=0xec00, id_count=0x2ff, out_ref=0x48, flags=0
DEBUG: build_iort_id_mapping: input_base=0xf000, id_count=0x2ff, out_ref=0xa0, flags=0
DEBUG: build_iort_id_mapping: input_base=0xf400, id_count=0x2ff, out_ref=0xf8, flags=0
DEBUG: build_iort_id_mapping: input_base=0xf800, id_count=0x2ff, out_ref=0x150, flags=0
DEBUG: build_iort_id_mapping: input_base=0xfc00, id_count=0x2ff, out_ref=0x1a8, flags=0
--------------build_iort: its_idmaps
DEBUG: build_iort_id_mapping: input_base=0x0, id_count=0xebff, out_ref=0x30, flags=0
DEBUG: build_iort_id_mapping: input_base=0xef00, id_count=0xff, out_ref=0x30, flags=0
DEBUG: build_iort_id_mapping: input_base=0xf300, id_count=0xff, out_ref=0x30, flags=0
DEBUG: build_iort_id_mapping: input_base=0xf700, id_count=0xff, out_ref=0x30, flags=0
DEBUG: build_iort_id_mapping: input_base=0xfb00, id_count=0xff, out_ref=0x30, flags=0
DEBUG: build_iort_id_mapping: input_base=0xff00, id_count=0xff, out_ref=0x30, flags=0
Thanks
Nicolin
On Tue, Jun 18, 2024 at 12:04:20PM -0700, Nicolin Chen wrote:
> On Tue, Jun 18, 2024 at 05:49:58AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 17, 2024 at 03:39:45PM -0700, Nicolin Chen wrote:
> > > The IORT doc defines "Number of IDs" ("id_count" in the virt-acpi-build)
> > > to be "the number of IDs in the range minus one". Otherwise, Linux kernel
> > > reports "conflicting mapping for input ID" FW_BUG at the overlapped ID.
> > >
> > > Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > ---
> > > Changelog
> > > v2:
> > > * Moved "-1" to the same line of id_count calculation
> > > * Added "+1" to the next_range.input_base calculation
> > > v1:
> > > https://lore.kernel.org/all/20240613234802.828265-1-nicolinc@nvidia.com/
> > >
> > > hw/arm/virt-acpi-build.c | 9 ++++++---
> > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index c3ccfef026..631f2c6d04 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -243,7 +243,8 @@ iort_host_bridges(Object *obj, void *opaque)
> > >
> > > AcpiIortIdMapping idmap = {
> > > .input_base = min_bus << 8,
> > > - .id_count = (max_bus - min_bus + 1) << 8,
> > > + /* id_count is the number of IDs in the range minus one */
> > > + .id_count = ((max_bus - min_bus + 1) << 8) - 1,
> > > };
> > > g_array_append_val(idmap_blob, idmap);
> > > }
> > > @@ -298,11 +299,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> > >
> > > if (next_range.input_base < idmap->input_base) {
> > > - next_range.id_count = idmap->input_base - next_range.input_base;
> > > + /* id_count is the number of IDs in the range minus one */
> > > + next_range.id_count = idmap->input_base -
> > > + next_range.input_base - 1;
> > > g_array_append_val(its_idmaps, next_range);
> >
> >
> > What about other places where id_count is set?
>
> There are only three places where id_count could be set:
> hw/arm/virt-acpi-build.c:296: .id_count = ((max_bus - min_bus + 1) << 8) - 1,
> hw/arm/virt-acpi-build.c:422: next_range.id_count = idmap->input_base -
> hw/arm/virt-acpi-build.c:435: next_range.id_count = 0xFFFF - next_range.input_base;
>
> This patch fixes two, while the last one is correct using 0xFFFF.
>
> > > }
> > >
> > > - next_range.input_base = idmap->input_base + idmap->id_count;
> > > + next_range.input_base = idmap->input_base + idmap->id_count + 1;
> > > }
> > >
> >
> > Given this was different previously, did you actually test with multiple ranges?
>
> Tested by creating 5 buses: input_base increases by 0x400 while
> id_count=0x2ff (0x300 - 1). ITS results look correct to me:
> --------------build_iort: smmu_idmaps
> DEBUG: build_iort_id_mapping: input_base=0xec00, id_count=0x2ff, out_ref=0x48, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xf000, id_count=0x2ff, out_ref=0xa0, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xf400, id_count=0x2ff, out_ref=0xf8, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xf800, id_count=0x2ff, out_ref=0x150, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xfc00, id_count=0x2ff, out_ref=0x1a8, flags=0
> --------------build_iort: its_idmaps
> DEBUG: build_iort_id_mapping: input_base=0x0, id_count=0xebff, out_ref=0x30, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xef00, id_count=0xff, out_ref=0x30, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xf300, id_count=0xff, out_ref=0x30, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xf700, id_count=0xff, out_ref=0x30, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xfb00, id_count=0xff, out_ref=0x30, flags=0
> DEBUG: build_iort_id_mapping: input_base=0xff00, id_count=0xff, out_ref=0x30, flags=0
>
> Thanks
> Nicolin
Okay. Is it the case that none of these effect the IORT in
./tests/data/acpi/virt/IORT then?
--
MST
On Tue, Jun 18, 2024 at 03:38:31PM -0400, Michael S. Tsirkin wrote:
> > > > - next_range.input_base = idmap->input_base + idmap->id_count;
> > > > + next_range.input_base = idmap->input_base + idmap->id_count + 1;
> > > > }
> > > >
> > >
> > > Given this was different previously, did you actually test with multiple ranges?
> >
> > Tested by creating 5 buses: input_base increases by 0x400 while
> > id_count=0x2ff (0x300 - 1). ITS results look correct to me:
> > --------------build_iort: smmu_idmaps
> > DEBUG: build_iort_id_mapping: input_base=0xec00, id_count=0x2ff, out_ref=0x48, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xf000, id_count=0x2ff, out_ref=0xa0, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xf400, id_count=0x2ff, out_ref=0xf8, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xf800, id_count=0x2ff, out_ref=0x150, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xfc00, id_count=0x2ff, out_ref=0x1a8, flags=0
> > --------------build_iort: its_idmaps
> > DEBUG: build_iort_id_mapping: input_base=0x0, id_count=0xebff, out_ref=0x30, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xef00, id_count=0xff, out_ref=0x30, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xf300, id_count=0xff, out_ref=0x30, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xf700, id_count=0xff, out_ref=0x30, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xfb00, id_count=0xff, out_ref=0x30, flags=0
> > DEBUG: build_iort_id_mapping: input_base=0xff00, id_count=0xff, out_ref=0x30, flags=0
> Okay. Is it the case that none of these effect the IORT in
> ./tests/data/acpi/virt/IORT then?
I wasn't aware of that. I checked the dsl content of that IORT
(attaching at the EOM). Only one section of ID mapping, and it
covers the entire 0xFFFF. So, I would say it's not affected?
Or is there anything else that we should try with that IORT?
Thanks
Nicolin
-----
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20200925 (64-bit version)
* Copyright (c) 2000 - 2020 Intel Corporation
*
* Disassembly of ../tests/data/acpi/virt/IORT, Tue Jun 18 19:49:04 2024
*
* ACPI Data Table [IORT]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
*/
[000h 0000 4] Signature : "IORT" [IO Remapping Table]
[004h 0004 4] Table Length : 00000080
[008h 0008 1] Revision : 03
[009h 0009 1] Checksum : B3
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
[024h 0036 4] Node Count : 00000002
[028h 0040 4] Node Offset : 00000030
[02Ch 0044 4] Reserved : 00000000
[030h 0048 1] Type : 00
[031h 0049 2] Length : 0018
[033h 0051 1] Revision : 01
[034h 0052 4] Reserved : 00000000
[038h 0056 4] Mapping Count : 00000000
[03Ch 0060 4] Mapping Offset : 00000000
[040h 0064 4] ItsCount : 00000001
[044h 0068 4] Identifiers : 00000000
[048h 0072 1] Type : 02
[049h 0073 2] Length : 0038
[04Bh 0075 1] Revision : 03
[04Ch 0076 4] Reserved : 00000001
[050h 0080 4] Mapping Count : 00000001
[054h 0084 4] Mapping Offset : 00000024
[058h 0088 8] Memory Properties : [IORT Memory Access Properties]
[058h 0088 4] Cache Coherency : 00000001
[05Ch 0092 1] Hints (decoded below) : 00
Transient : 0
Write Allocate : 0
Read Allocate : 0
Override : 0
[05Dh 0093 2] Reserved : 0000
[05Fh 0095 1] Memory Flags (decoded below) : 03
Coherency : 1
Device Attribute : 1
[060h 0096 4] ATS Attribute : 00000000
[064h 0100 4] PCI Segment Number : 00000000
[068h 0104 1] Memory Size Limit : 40
[069h 0105 3] Reserved : 000000
[06Ch 0108 4] Input base : 00000000
[070h 0112 4] ID Count : 0000FFFF
[074h 0116 4] Output Base : 00000000
[078h 0120 4] Output Reference : 00000030
[07Ch 0124 4] Flags (decoded below) : 00000000
Single Mapping : 0
Raw Table Data: Length 128 (0x80)
0000: 49 4F 52 54 80 00 00 00 03 B3 42 4F 43 48 53 20 // IORT......BOCHS
0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
0020: 01 00 00 00 02 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
0030: 00 18 00 01 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0040: 01 00 00 00 00 00 00 00 02 38 00 03 01 00 00 00 // .........8......
0050: 01 00 00 00 24 00 00 00 01 00 00 00 00 00 00 03 // ....$...........
0060: 00 00 00 00 00 00 00 00 40 00 00 00 00 00 00 00 // ........@.......
0070: FF FF 00 00 00 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
© 2016 - 2024 Red Hat, Inc.