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

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

[Nicolin Chen]

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;
                 g_array_append_val(its_idmaps, next_range);
             }
 
-- 
2.43.0

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

[Michael S. Tsirkin]

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

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

[Nicolin Chen]

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