| Message ID | 20240613234802.828265-1-nicolinc@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt-acpi-build: Fix IORT id_count | expand |
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 <nicolinc@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
On Mon, Jun 17, 2024 at 06:41:55AM -0400, Michael S. Tsirkin wrote: > 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 <nicolinc@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. OK. Let's do this then: - 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); > > } > > > But the value is used later: > > next_range.input_base = idmap->input_base + idmap->id_count; > > Wouldn't that make next_range incorrect? Ah, missed that. Thanks! - next_range.input_base = idmap->input_base + idmap->id_count; + next_range.input_base = idmap->input_base + idmap->id_count + 1; > > 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. Well, it's an inline-like function called by build_iort() only, where there is a function header mentioning the doc. Should this function have to repeat? Anyway, that would be a different patch. I'll submit a v2 first. Thank you Nicolin
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; g_array_append_val(its_idmaps, next_range); }
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> --- hw/arm/virt-acpi-build.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)