On Thu, Jun 13, 2024 at 04:48:02PM -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 <nicol...@nvidia.com> > --- > hw/arm/virt-acpi-build.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index c3ccfef026..b9343dde0f 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,7 +299,9 @@ 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) { > + /* id_count is the number of IDs in the range minus one */ > next_range.id_count = idmap->input_base - > next_range.input_base; > + next_range.id_count -= 1;
I would just add - 1 on the previous line, instead of making it incorrect then correcting it. > g_array_append_val(its_idmaps, next_range); > } But the value is used later: next_range.input_base = idmap->input_base + idmap->id_count; Wouldn't that make next_range incorrect? I also note that static void build_iort_id_mapping(GArray *table_data, uint32_t input_base, uint32_t id_count, uint32_t out_ref) { /* Table 4 ID mapping format */ build_append_int_noprefix(table_data, input_base, 4); /* Input base */ build_append_int_noprefix(table_data, id_count, 4); /* Number of IDs */ build_append_int_noprefix(table_data, input_base, 4); /* Output base */ build_append_int_noprefix(table_data, out_ref, 4); /* Output Reference */ /* Flags */ build_append_int_noprefix(table_data, 0 /* Single mapping (disabled) */, 4); } That comment /* Table 4 ID mapping format */ really should be before the function and it should mention the spec it's from - specifically the earliest spec including the relevant table. > > -- > 2.43.0