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