diff mbox series

[v3] hw/arm/virt-acpi-build: Fix id_count in build_iort_id_mapping

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

Commit Message

Nicolin Chen June 18, 2024, 9:11 p.m. UTC
It's observed that Linux kernel booting with the VM reports a "conflicting
mapping for input ID" FW_BUG.

The IORT doc defines "Number of IDs" to be "the number of IDs in the range
minus one", while virt-acpi-build.c simply stores the number of IDs in the
id_count without the "minus one". Meanwhile, some of the callers pass in a
0xFFFF following the spec. So, this is a mismatch between the function and
its callers.

Fix build_iort_id_mapping() by internally subtracting one from the pass-in
@id_count. Accordingly make sure that all existing callers pass in a value
without the "minus one", i.e. change all 0xFFFFs to 0x10000s.

Also, add a few lines of comments to highlight this change along with the
referencing document for this build_iort_id_mapping().

Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
Changelog
v3:
 * Added "-1" internally in build_iort_id_mapping() instead
 * Added comments to highlight this and referencing doc
v2:
 https://lore.kernel.org/all/20240617223945.906996-1-nicolinc@nvidia.com/
 * 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 | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Michael S. Tsirkin June 18, 2024, 9:14 p.m. UTC | #1
On Tue, Jun 18, 2024 at 02:11:10PM -0700, Nicolin Chen wrote:
> It's observed that Linux kernel booting with the VM reports a "conflicting
> mapping for input ID" FW_BUG.
> 
> The IORT doc defines "Number of IDs" to be "the number of IDs in the range
> minus one", while virt-acpi-build.c simply stores the number of IDs in the
> id_count without the "minus one". Meanwhile, some of the callers pass in a
> 0xFFFF following the spec. So, this is a mismatch between the function and
> its callers.
> 
> Fix build_iort_id_mapping() by internally subtracting one from the pass-in
> @id_count. Accordingly make sure that all existing callers pass in a value
> without the "minus one", i.e. change all 0xFFFFs to 0x10000s.
> 
> Also, add a few lines of comments to highlight this change along with the
> referencing document for this build_iort_id_mapping().
> 
> Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> Changelog
> v3:
>  * Added "-1" internally in build_iort_id_mapping() instead
>  * Added comments to highlight this and referencing doc
> v2:
>  https://lore.kernel.org/all/20240617223945.906996-1-nicolinc@nvidia.com/
>  * 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 | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index c3ccfef026..ee6f56b410 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -209,12 +209,20 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>  #define ROOT_COMPLEX_ENTRY_SIZE 36
>  #define IORT_NODE_OFFSET 48
>  
> +/*
> + * Input Output Remapping Table (IORT) -- Table 4 ID mapping format
> + * Conforms to "IO Remapping Table System Software on ARM Platforms",
> + * Document number: ARM DEN 0049E.b, Feb 2021
> + *
> + * Note that @id_count will be internally subtracted by one, following
> + * the IORT spec.
> + */
>  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 */
> +    /* Number of IDs - The number of IDs in the range minus one */
> +    build_append_int_noprefix(table_data, id_count - 1, 4);
>      build_append_int_noprefix(table_data, input_base, 4); /* Output base */
>      build_append_int_noprefix(table_data, out_ref, 4); /* Output Reference */
>      /* Flags */
> @@ -306,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          }
>  
>          /* Append the last RC -> ITS ID mapping */
> -        if (next_range.input_base < 0xFFFF) {
> -            next_range.id_count = 0xFFFF - next_range.input_base;
> +        if (next_range.input_base < 0x10000) {
> +            next_range.id_count = 0x10000 - next_range.input_base;
>              g_array_append_val(its_idmaps, next_range);
>          }

A change of logic here - I think the new one is right and old
one was wrong, actually. Right?

>  
> @@ -366,7 +374,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          build_append_int_noprefix(table_data, 0, 4);
>  
>          /* output IORT node is the ITS group node (the first node) */
> -        build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
> +        build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>      }
>  
>      /* Table 17 Root Complex Node */
> @@ -419,7 +427,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          }
>      } else {
>          /* output IORT node is the ITS group node (the first node) */
> -        build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
> +        build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>      }
>  
>      acpi_table_end(linker, &table);
> -- 
> 2.43.0
Nicolin Chen June 18, 2024, 9:19 p.m. UTC | #2
On Tue, Jun 18, 2024 at 05:14:32PM -0400, Michael S. Tsirkin wrote:
> > @@ -306,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >          }
> >
> >          /* Append the last RC -> ITS ID mapping */
> > -        if (next_range.input_base < 0xFFFF) {
> > -            next_range.id_count = 0xFFFF - next_range.input_base;
> > +        if (next_range.input_base < 0x10000) {
> > +            next_range.id_count = 0x10000 - next_range.input_base;
> >              g_array_append_val(its_idmaps, next_range);
> >          }
> 
> A change of logic here - I think the new one is right and old
> one was wrong, actually. Right?

Sorry, I don't quite follow that question...

Doesn't a patch correct an old wrong one to a new right one?

Thanks
Nicolin
Michael S. Tsirkin June 18, 2024, 9:34 p.m. UTC | #3
On Tue, Jun 18, 2024 at 02:19:25PM -0700, Nicolin Chen wrote:
> On Tue, Jun 18, 2024 at 05:14:32PM -0400, Michael S. Tsirkin wrote:
> > > @@ -306,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > >          }
> > >
> > >          /* Append the last RC -> ITS ID mapping */
> > > -        if (next_range.input_base < 0xFFFF) {
> > > -            next_range.id_count = 0xFFFF - next_range.input_base;
> > > +        if (next_range.input_base < 0x10000) {
> > > +            next_range.id_count = 0x10000 - next_range.input_base;
> > >              g_array_append_val(its_idmaps, next_range);
> > >          }
> > 
> > A change of logic here - I think the new one is right and old
> > one was wrong, actually. Right?
> 
> Sorry, I don't quite follow that question...
> 
> Doesn't a patch correct an old wrong one to a new right one?
> 
> Thanks
> Nicolin


So if base is 0xFFFF what should happen? I think previously we
skipped an entry and that is wrong. So that's another latent
bug this patch fixes then?
Worth documenting in the commit log too.
Nicolin Chen June 18, 2024, 9:51 p.m. UTC | #4
On Tue, Jun 18, 2024 at 05:34:21PM -0400, Michael S. Tsirkin wrote:
> On Tue, Jun 18, 2024 at 02:19:25PM -0700, Nicolin Chen wrote:
> > On Tue, Jun 18, 2024 at 05:14:32PM -0400, Michael S. Tsirkin wrote:
> > > > @@ -306,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > >          }
> > > >
> > > >          /* Append the last RC -> ITS ID mapping */
> > > > -        if (next_range.input_base < 0xFFFF) {
> > > > -            next_range.id_count = 0xFFFF - next_range.input_base;
> > > > +        if (next_range.input_base < 0x10000) {
> > > > +            next_range.id_count = 0x10000 - next_range.input_base;
> > > >              g_array_append_val(its_idmaps, next_range);
> > > >          }
> > >
> > > A change of logic here - I think the new one is right and old
> > > one was wrong, actually. Right?
> >
> > Sorry, I don't quite follow that question...
> >
> > Doesn't a patch correct an old wrong one to a new right one?
> >
> > Thanks
> > Nicolin
> 
> 
> So if base is 0xFFFF what should happen? I think previously we
> skipped an entry and that is wrong. So that's another latent
> bug this patch fixes then?
> Worth documenting in the commit log too.

I had noticed that -- yes, ideally it should have been "<= 0xFFFF".
Yet, practically input_base can never be 0xFFFF as it's calculated:

hw/arm/virt-acpi-build.c:245:                .input_base = min_bus << 8,
hw/arm/virt-acpi-build.c:305:            next_range.input_base = idmap->input_base + idmap->id_count;

The first one always sets input_base to 0xXX00 (min_bus = 0xXX).
The second one, as we know for id_count, must be 0xZZ00 too since
input_base from the first place must be 0xXX00 and 0xYY00
hw/arm/virt-acpi-build.c:301:                next_range.id_count = idmap->input_base - next_range.input_base;

So, it's a case that could never be triggered? Probably not worth
highlighting IMOH...

Thanks
Nicolin
Michael S. Tsirkin June 18, 2024, 10:05 p.m. UTC | #5
On Tue, Jun 18, 2024 at 02:51:41PM -0700, Nicolin Chen wrote:
> On Tue, Jun 18, 2024 at 05:34:21PM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jun 18, 2024 at 02:19:25PM -0700, Nicolin Chen wrote:
> > > On Tue, Jun 18, 2024 at 05:14:32PM -0400, Michael S. Tsirkin wrote:
> > > > > @@ -306,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > > >          }
> > > > >
> > > > >          /* Append the last RC -> ITS ID mapping */
> > > > > -        if (next_range.input_base < 0xFFFF) {
> > > > > -            next_range.id_count = 0xFFFF - next_range.input_base;
> > > > > +        if (next_range.input_base < 0x10000) {
> > > > > +            next_range.id_count = 0x10000 - next_range.input_base;
> > > > >              g_array_append_val(its_idmaps, next_range);
> > > > >          }
> > > >
> > > > A change of logic here - I think the new one is right and old
> > > > one was wrong, actually. Right?
> > >
> > > Sorry, I don't quite follow that question...
> > >
> > > Doesn't a patch correct an old wrong one to a new right one?
> > >
> > > Thanks
> > > Nicolin
> > 
> > 
> > So if base is 0xFFFF what should happen? I think previously we
> > skipped an entry and that is wrong. So that's another latent
> > bug this patch fixes then?
> > Worth documenting in the commit log too.
> 
> I had noticed that -- yes, ideally it should have been "<= 0xFFFF".
> Yet, practically input_base can never be 0xFFFF as it's calculated:
> 
> hw/arm/virt-acpi-build.c:245:                .input_base = min_bus << 8,
> hw/arm/virt-acpi-build.c:305:            next_range.input_base = idmap->input_base + idmap->id_count;
> 
> The first one always sets input_base to 0xXX00 (min_bus = 0xXX).
> The second one, as we know for id_count, must be 0xZZ00 too since
> input_base from the first place must be 0xXX00 and 0xYY00
> hw/arm/virt-acpi-build.c:301:                next_range.id_count = idmap->input_base - next_range.input_base;
> 
> So, it's a case that could never be triggered? Probably not worth
> highlighting IMOH...
> 
> Thanks
> Nicolin

Makes sense.
Eric Auger June 19, 2024, 2:15 p.m. UTC | #6
Hi Nicolin,

On 6/18/24 23:11, Nicolin Chen wrote:
> It's observed that Linux kernel booting with the VM reports a "conflicting
> mapping for input ID" FW_BUG.
> 
> The IORT doc defines "Number of IDs" to be "the number of IDs in the range
> minus one", while virt-acpi-build.c simply stores the number of IDs in the
> id_count without the "minus one". Meanwhile, some of the callers pass in a
> 0xFFFF following the spec. So, this is a mismatch between the function and
> its callers.
> 
> Fix build_iort_id_mapping() by internally subtracting one from the pass-in
> @id_count. Accordingly make sure that all existing callers pass in a value
> without the "minus one", i.e. change all 0xFFFFs to 0x10000s.
> 
> Also, add a few lines of comments to highlight this change along with the
> referencing document for this build_iort_id_mapping().
> 
> Fixes: 42e0f050e3a5 ("hw/arm/virt-acpi-build: Add IORT support to bypass SMMUv3")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> Changelog
> v3:
>  * Added "-1" internally in build_iort_id_mapping() instead
>  * Added comments to highlight this and referencing doc
> v2:
>  https://lore.kernel.org/all/20240617223945.906996-1-nicolinc@nvidia.com/
>  * 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 | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index c3ccfef026..ee6f56b410 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -209,12 +209,20 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>  #define ROOT_COMPLEX_ENTRY_SIZE 36
>  #define IORT_NODE_OFFSET 48
>  
> +/*
> + * Input Output Remapping Table (IORT) -- Table 4 ID mapping format
> + * Conforms to "IO Remapping Table System Software on ARM Platforms",
> + * Document number: ARM DEN 0049E.b, Feb 2021
I would rather explain what the function does, ie append an ID mapping
entry as desribed in Tabble 4 ID Mapping format.

Also while at it you may use a more recent revision
There is DEN0049E_IO_Remapping_Table_E.f.pdf available

Besides:

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric



> + *
> + * Note that @id_count will be internally subtracted by one, following
> + * the IORT spec.
> + */
>  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 */
> +    /* Number of IDs - The number of IDs in the range minus one */
> +    build_append_int_noprefix(table_data, id_count - 1, 4);
>      build_append_int_noprefix(table_data, input_base, 4); /* Output base */
>      build_append_int_noprefix(table_data, out_ref, 4); /* Output Reference */
>      /* Flags */
> @@ -306,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          }
>  
>          /* Append the last RC -> ITS ID mapping */
> -        if (next_range.input_base < 0xFFFF) {
> -            next_range.id_count = 0xFFFF - next_range.input_base;
> +        if (next_range.input_base < 0x10000) {
> +            next_range.id_count = 0x10000 - next_range.input_base;
>              g_array_append_val(its_idmaps, next_range);
>          }
>  
> @@ -366,7 +374,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          build_append_int_noprefix(table_data, 0, 4);
>  
>          /* output IORT node is the ITS group node (the first node) */
> -        build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
> +        build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>      }
>  
>      /* Table 17 Root Complex Node */
> @@ -419,7 +427,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          }
>      } else {
>          /* output IORT node is the ITS group node (the first node) */
> -        build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
> +        build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>      }
>  
>      acpi_table_end(linker, &table);
Nicolin Chen June 19, 2024, 7:56 p.m. UTC | #7
On Wed, Jun 19, 2024 at 04:15:35PM +0200, Eric Auger wrote:
> > @@ -209,12 +209,20 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
> >  #define ROOT_COMPLEX_ENTRY_SIZE 36
> >  #define IORT_NODE_OFFSET 48
> >
> > +/*
> > + * Input Output Remapping Table (IORT) -- Table 4 ID mapping format
> > + * Conforms to "IO Remapping Table System Software on ARM Platforms",
> > + * Document number: ARM DEN 0049E.b, Feb 2021
> I would rather explain what the function does, ie append an ID mapping
> entry as desribed in Tabble 4 ID Mapping format.
> 
> Also while at it you may use a more recent revision
> There is DEN0049E_IO_Remapping_Table_E.f.pdf available

Sure. Will do a v4 with something like:

+/*
+ * Append an ID mapping entry as described in "Table 4 ID mapping format"
+ * from "IO Remapping Table System Software on ARM Platforms", Chapter 3.
+ * Document number: ARM DEN 0049E.f, Apr 2024

> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks for the review!

Nicolin
diff mbox series

Patch

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index c3ccfef026..ee6f56b410 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -209,12 +209,20 @@  static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
 #define ROOT_COMPLEX_ENTRY_SIZE 36
 #define IORT_NODE_OFFSET 48
 
+/*
+ * Input Output Remapping Table (IORT) -- Table 4 ID mapping format
+ * Conforms to "IO Remapping Table System Software on ARM Platforms",
+ * Document number: ARM DEN 0049E.b, Feb 2021
+ *
+ * Note that @id_count will be internally subtracted by one, following
+ * the IORT spec.
+ */
 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 */
+    /* Number of IDs - The number of IDs in the range minus one */
+    build_append_int_noprefix(table_data, id_count - 1, 4);
     build_append_int_noprefix(table_data, input_base, 4); /* Output base */
     build_append_int_noprefix(table_data, out_ref, 4); /* Output Reference */
     /* Flags */
@@ -306,8 +314,8 @@  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         }
 
         /* Append the last RC -> ITS ID mapping */
-        if (next_range.input_base < 0xFFFF) {
-            next_range.id_count = 0xFFFF - next_range.input_base;
+        if (next_range.input_base < 0x10000) {
+            next_range.id_count = 0x10000 - next_range.input_base;
             g_array_append_val(its_idmaps, next_range);
         }
 
@@ -366,7 +374,7 @@  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         build_append_int_noprefix(table_data, 0, 4);
 
         /* output IORT node is the ITS group node (the first node) */
-        build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
+        build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
     }
 
     /* Table 17 Root Complex Node */
@@ -419,7 +427,7 @@  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         }
     } else {
         /* output IORT node is the ITS group node (the first node) */
-        build_iort_id_mapping(table_data, 0, 0xFFFF, IORT_NODE_OFFSET);
+        build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
     }
 
     acpi_table_end(linker, &table);