RIMT parser for acpiview command #11541

krzysztof-drobinski-plastelinet · 2025-09-22T12:55:37Z

Description

RISC-V IO Mapping Table (RIMT) is the ACPI table describing relations between IOMMU and other devices on RISC-V architecture.

See https://github.com/riscv-non-isa/riscv-acpi-rimt for details.

This PR extends acpiview command by adding RIMT parser

Breaking change?
- No
Impacts security?
- No
Includes tests?
- No

How This Was Tested

Tested on qemu-10.1 with RISC-V IOMMU enabled configured as SYS as well as PCIe device

qemu-system-riscv64 \
	-cpu rva23s64 \
	-smp 4 \
	-nographic \
	-M virt,acpi=on,aia=aplic-imsic,aia-guests=5,iommu-sys=on,pflash0=pflash0,pflash1=pflash1,  \
	-serial mon:stdio \
	-device virtio-gpu-pci \
	-device qemu-xhci \
	-device usb-kbd \
	-device virtio-rng-pci \
	-device ahci,id=ahci -device ide-hd,drive=disk,bus=ahci.0 -drive id=disk,file=./fat32.img,format=raw,if=none \
	-blockdev node-name=pflash0,driver=file,read-only=on,filename=./RISCV_VIRT_CODE.fd \
	-blockdev node-name=pflash1,driver=file,filename=.//RISCV_VIRT_VARS.fd \
	-bios ./build_opensbi/platform/generic/firmware/fw_dynamic.bin \
	-kernel ./build_linux/arch/riscv/boot/Image -append "root=/dev/sda" \
	--trace "riscv_iommu*"

Then in UEFI Shell

Shell> acpiview -s rimt


 --------------- RIMT Table --------------- 

Address  : 0x875D7000
Length   : 128
  
00000000 : 52 49 4D 54 80 00 00 00 - 01 E1 42 4F 43 48 53 20   RIMT......BOCHS 
00000010 : 42 58 50 43 20 20 20 20 - 01 00 00 00 42 58 50 43   BXPC    ....BXPC
00000020 : 01 00 00 00 02 00 00 00 - 30 00 00 00 00 00 00 00   ........0.......
00000030 : 00 01 28 00 00 00 00 00 - 52 53 43 56 30 30 30 34   ..(.....RSCV0004
00000040 : 00 00 01 03 00 00 00 00 - 00 00 00 00 00 00 00 00   ................
00000050 : 00 00 00 00 00 00 28 00 - 01 01 28 00 00 00 01 00   ......(...(.....
00000060 : 00 00 00 00 00 00 00 00 - 14 00 01 00 00 00 00 00   ................
00000070 : FF FF 00 00 00 00 00 00 - 30 00 00 00 00 00 00 00   ........0.......

Table Checksum : OK

RIMT                                 :
  Signature                          : RIMT
  Length                             : 128
  Revision                           : 1
  Checksum                           : 0xE1
  Oem ID                             : BOCHS 
  Oem Table ID                       : BXPC    
  Oem Revision                       : 0x1
  Creator ID                         : BXPC
  Creator Revision                   : 0x1
  Number of RIMT Nodes               : 2
  Offset to RIMT Node Array          : 0x30
  Reserved                           : 0x0
  RIMT Node [0]                      :
    Type                             : IOMMU
    Revision                         : 1
    Length                           : 40
    Reserved                         : 0x0
    Id                               : 0
    Hardware ID                      : RSCV0004 (0x3430303056435352)
    Base Address                     : 0x3010000
    Flags                            : 0x0
      Device Type                    : "Platform"
      Proximity Domain               : "Invalid"
      Reserved                       : 0x0

    Proximity Domain                 : 0
    PCIe Segment number              : 0x0
    PCIe B/D/F                       : 0
    Number of interrupt wires        : 0x0
    Interrupt wire array offset      : 0x28
  RIMT Node [1]                      :
    Type                             : PCIe Root Complex
    Revision                         : 1
    Length                           : 40
    Reserved                         : 0x0
    Id                               : 1
    Flags                            : 0x0
      ATS                            : "Not Supported"
      PRI                            : "Not Supported"
      Reserved                       : 0x0

    Reserved                         : 0x0
    PCIe Segment Number              : 0
    Id Mapping Array Offset          : 0x14
    Number Of Id Mappings            : 1
    ID Mapping [0]                   :
      Source ID Base                 : 0x0
      Number of IDs                  : 65535
      Destination Device ID Base     : 0
      Destination IOMMU Offset       : 0x30
      Flags                          : 0x0
        ATS                          : "Not Required"
        PRI                          : "Not Required"
        Reserved                     : 0x0


Table Statistics:
        0 Error(s)
        0 Warning(s)

Integration Instructions

N/A

MdePkg/Include/IndustryStandard/Acpi66.h

leiflindholm · 2025-09-23T16:37:35Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+  )
+{
+  PrintRimtNodeHeader ((EFI_ACPI_6_6_RIMT_NODE_HEADER_STRUCTURE *)Node);
+  HardwareIdToString (&Node->HardwareId, HardwareIdStr);


Could replace with call to AsciiStrnCpyS instead of custom function?

ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni

pierregondois · 2025-09-24T12:00:58Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+    RimtNodeIommuTypeName[GET_BIT (0, Node->Flags)],
+    RimtNodeIommuProximityDomainTypeName[GET_BIT (1, Node->Flags)]
+    );
+  Print (L"\tProximity Domain            : %d\n", Node->ProximityDomain);


Hello Krzysztof,
Would it be possible to follow the table-based approach as the other parsers do ?
The Mcfg Parser should provide a good example:
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mcfg/McfgParser.c
and there are also wrappers to print bitfields, cf., the ParseAcpiBitFields() function

Hi Pierre,
I could but the issue I see with table approach is that it respects ACPI structures from MdePkg/Include/IndustryStandard/ only partially (only generic ACPI header from Acpi.h in fact)
I looked at MdePkg/Include/IndustryStandard/IoRemappingTable.h and ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c for instance.
IortParser duplicates most of IORT structures in a way of offset definitions in parser tables.
There is no sync point between state of IndustryStandard headers and parser tables in IORT acpiview for example. Do you recommend rewriting to table-based approach?

There is effectively no synchronization between the MdePkg definitions and the AcpiView definitions, however using the table-based approach:

allows to have a uniform way to display the information. Modifying ParseAcpi() would allow to update the way to print all tables without applying changes to each parser individually

does some additional checking. In the RIMT parser, there doesn't seem to be any check regarding the size of the table: if the table contains an error, the parser will try to access data outside of the table.

allows to have cross-table validation. This is not present right now, but there was an idea of keeping track of some field values of some tables in order to validate the fields of some other tables. If the parser just prints the fields, this is not possible to do anymore.

@pierregondois Ok What about something like

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c index 5cdd34f442..5410355548 100644 --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c @@ -29,9 +29,9 @@ STATIC EFI_ACPI_6_6_RIMT_STRUCTURE RimtHdrInfo; **/ STATIC CONST ACPI_PARSER RimtParser[] = { PARSE_ACPI_HEADER (&RimtHdrInfo.Header), - { L"Number of RIMT Nodes", 4, 36, L"%d", NULL, NULL, NULL, NULL }, - { L"Offset to RIMT Node Array", 4, 40, L"0x%x", NULL, NULL, NULL, NULL }, - { L"Reserved", 4, 44, L"0x%x", NULL, NULL, NULL, NULL } + { L"Number of RIMT Nodes", sizeof(RimtHdrInfo.NumberOfRimtNodes), OFFSET_OF(EFI_ACPI_6_6_RIMT_STRUCTURE, NumberOfRimtNodes), L"%d", NULL, NULL, NULL, NULL },^M + { L"Offset to RIMT Node Array", sizeof(RimtHdrInfo.OffsetToRimtNodeArray), OFFSET_OF(EFI_ACPI_6_6_RIMT_STRUCTURE, OffsetToRimtNodeArray), L"0x%x", NULL, NULL, NULL, NULL },^M + { L"Reserved", sizeof(RimtHdrInfo.Reserved), OFFSET_OF(EFI_ACPI_6_6_RIMT_STRUCTURE, Reserved), L"0x%x", NULL, NULL, NULL, NULL }^M

This way acpiview could validate IndustrialStandard table header for real.

This way acpiview could validate IndustrialStandard table header for real.

Yeah, that looks like something we would want to do across the board. So would be good to do start doing it at least for new additions.

Yes nice, it is better to avoid hard coded value. (+ tables are used for parsing)

@leiflindholm the original intent behind the current approach in acpiview was that the first three fields of the ACPI_PARSER—Field, Byte Length, and Byte Offset—would be taken directly from the ACPI table specification. For example, these correspond to the first three columns in the MADT table definition: https://uefi.org/specs/ACPI/6.6/05_ACPI_Software_Programming_Model.html#multiple-apic-description-table-madt
This approach helps verify that the offsets in the specification are correct, since the parser would flag an error if there were any mismatches.

I’m not sure if this is still considered useful, but it has definitely helped in the past to catch issues.
Personally, I’d be inclined to keep this scheme if possible.

@samimujawar I'm not sure I understand.
Are you saing it's beneficial to hard-code the values instead of using the core definitions because people would be unlikely to get it wrong in the same way twice when manually transcribing?

Thinking about it again it is a maintenance overhead as one has to review that the first 3 fields have been populated correctly. If the field definitions from the structure are used, those definitions would need to reflect the spec, and if the size and offset mismatch it will get flagged by the parser.
So we can go with the proposed approach rather that hardcoding the values.

@pierregondois Switched to ACPI_PARSERs

Introduce RIMT (RISC-V IO Mapping Table) structures as specified in [1]. RIMT was introduced as part of ACPI in version 6.6 [2]. [1] RISC-V IO Mapping Table (RIMT) Specification, Version v1.0, 2025-03-31 https://github.com/riscv-non-isa/riscv-acpi-rimt/releases/download/v1.0/rimt-spec.pdf [2] Advanced Configuration and Power Interface specification, Version 6.6 https://uefi.org/specs/ACPI/6.6/ Signed-off-by: Krzysztof Drobiński <krzysztof@plasteli.net>

Implementation tested on Qemu-10.1 virt machine with IOMMU enabled. Signed-off-by: Krzysztof Drobiński <krzysztof@plasteli.net>

mergify · 2025-10-02T11:50:14Z

⚠️ The sha of the head commit of this PR conflicts with #11562. Mergify cannot evaluate rules on this PR. ⚠️

leiflindholm · 2025-10-02T12:07:54Z

MdePkg/Include/IndustryStandard/RiscVIoMappingTable.h

+    (https://uefi.org/specs/ACPI/6.6/)
+**/
+
+#ifndef __RISCV_IO_MAPPING_TABLE_H_


Symbol names with leading underscore are reserved for toolchain use. (Please drop leading __).

leiflindholm · 2025-10-02T12:09:44Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+  VOID    *Context
+  )
+{
+  UINT32  data; // "Number of RIMT Nodes" field is 4 byte long in spec


'Data', not 'data'.

Although if this is a variable to hold specifically the number of nodes, it probably ought to be called NumNodes or something more descriptive.

pierregondois · 2025-10-02T16:09:45Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+  PCIERC,
+  PLATFORM,
+  UNSUPPORTED
+} RimtNodeTypes;


Ideally this should go in the MdePkg as these values are spec-related

pierregondois · 2025-10-02T16:12:03Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+  UINT8  NodeType;
+
+  NodeType = *(UINT8 *)Ptr;
+  if (NodeType == IOMMU) {


I think it would be easier to use an array of strings, cf. the ErstActionTable for instance

@pierregondois Not sure about that. It will make reading this simple code harder (by need of jumping to array of strings to see what's behind the Str). Additionally, we will loose relation to RimtNodeTypes here in favor of array index. Those strings are not used anywhere except this routine. What's the benefit?

pierregondois · 2025-10-02T16:13:44Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+  Every RIMT Node (IOMMU, PCIe Root Complex, PLatform Device) has this header
+  common
+**/
+STATIC UINT8                                    *RimtNodeType;


In general, static variables should be prefixed by m: mRimtNodeType
And also ideally be placed at the beginning of the file.
Same comment at other places

@pierregondois

In general, static variables should be prefixed by m: mRimtNodeType

Ok

And also ideally be placed at the beginning of the file.

Is that approach required by EDK2? IMHO, It makes code harder to read. Declaring (if possible) variables in places where they are going to be used for the first time simplifies reading.

It is a general rule, yeah. Each variant has its own benefits, we picked this one.

https://tianocore-docs.github.io/edk2-CCodingStandardsSpecification/release-2.20/5_source_files/54_code_file_structure.html#54-code-file-structure

pierregondois · 2025-10-02T16:16:45Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+  RIMT_PARSE_NODE_HEADER
+};
+
+#define RIMT_MINIMAL_NUMBER_OF_NODES_ALLOWED  2


Similarly, might move the definition to the MdePkg.
Please move all the possible RIMT definitions to the MdePkg

pierregondois · 2025-10-02T16:40:28Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+  @param [in] Active    String to display if interpreted bit is active.
+  @param [in] Inactive  String to display if interpreted bit is inactive.
+**/
+#define RIMT_DECLARE_FLAGS_BIT_PARSER_FUNC(Parser, Active, Inactive) \


This seems quite complex to have a mapping:
0 -> Inactive
1 -> Active

Would it be ok to just print the bit value ?

If no, would it possible instead to define an generic abstraction in:
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c using a similar logic as in the MadtParser.c::DumpValue() so that other parsers can re-use it ?
Tagging @leiflindholm just in case as what I m suggesting is slightly questionable.

pierregondois · 2025-10-02T16:48:13Z

ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rimt/RimtParser.c

+
+  @param [in] Parser  An ACPI_PARSER for tracing the data.
+**/
+#define RIMT_DECLARE_NODE_FLAGS_PARSER_FUNC(Parser) \


This might be a bit over-engineering. I don't know what other people think

krzysztof-drobinski-plastelinet force-pushed the kdrobinski/rimt_support_for_acpiview branch 7 times, most recently from c68e3f3 to 3cbb7ee Compare September 23, 2025 14:40

krzysztof-drobinski-plastelinet marked this pull request as ready for review September 23, 2025 15:45

tianocore-assign-reviewers bot requested review from ajfish, ardbiesheuvel, leiflindholm, lgao4, makubacki, mdkinney and pierregondois September 23, 2025 15:45

leiflindholm reviewed Sep 23, 2025

View reviewed changes

MdePkg/Include/IndustryStandard/Acpi66.h Outdated Show resolved Hide resolved

leiflindholm reviewed Sep 23, 2025

View reviewed changes

ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni Show resolved Hide resolved

pierregondois reviewed Sep 24, 2025

View reviewed changes

krzysztof-drobinski-plastelinet added 2 commits October 2, 2025 12:15

ShellPkg,acpiview: Add RIMT (RISC-V IO Mapping Table) parser

8af2978

Implementation tested on Qemu-10.1 virt machine with IOMMU enabled. Signed-off-by: Krzysztof Drobiński <krzysztof@plasteli.net>

krzysztof-drobinski-plastelinet force-pushed the kdrobinski/rimt_support_for_acpiview branch from 3cbb7ee to 8af2978 Compare October 2, 2025 11:49

mergify bot mentioned this pull request Oct 2, 2025

Kdrobinski/rimt support for acpiview uncrucify #11562

Closed

3 tasks

leiflindholm reviewed Oct 2, 2025

View reviewed changes

pierregondois reviewed Oct 2, 2025

View reviewed changes

ardbiesheuvel removed their request for review October 3, 2025 13:40

RIMT parser for acpiview command #11541

Are you sure you want to change the base?

RIMT parser for acpiview command #11541

Conversation

krzysztof-drobinski-plastelinet commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How This Was Tested

Integration Instructions

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krzysztof-drobinski-plastelinet commented Sep 22, 2025 •

edited

Loading