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

[Nicolin Chen <nicolinc@nvidia.com>]

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