diff mbox series

[v2] hw/arm/virt-acpi-build: Fix IORT id_count

Message ID 20240617223945.906996-1-nicolinc@nvidia.com (mailing list archive)
State New, archived
Headers show
Series [v2] hw/arm/virt-acpi-build: Fix IORT id_count | expand

Commit Message

Nicolin Chen June 17, 2024, 10:39 p.m. UTC 
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(-)

Comments

Michael S. Tsirkin June 18, 2024, 9:49 a.m. UTC | #1 
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
Nicolin Chen June 18, 2024, 7:04 p.m. UTC | #2 
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
Michael S. Tsirkin June 18, 2024, 7:38 p.m. UTC | #3 
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?
Nicolin Chen June 18, 2024, 7:57 p.m. UTC | #4 
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.......
Michael S. Tsirkin June 18, 2024, 8:03 p.m. UTC | #5 
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);
>              }
>  
> -            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
Nicolin Chen June 18, 2024, 8:12 p.m. UTC | #6 
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
diff mbox series

Patch

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 */