On Thu, 2026-09-17 at 10:52 +0530, Subrahmanya Lingappa wrote:
> Add the virt machine rpmi=on/off option and wire the initial RPMI
> Base
> transport into machine realization.
>
> The option remains disabled by default and requires TCG plus librpmi.
> Invalid accelerator/build combinations fail during machine
> initialization. When enabled, virt allocates the RPMI shared-memory
> transport, creates the RPMI device, configures Base service metadata,
> and emits the corresponding device-tree nodes for firmware discovery.
>
> Signed-off-by: Subrahmanya Lingappa
> <[email protected]>
My review bot says (sorry about the line wrapping):
- [HIGH] In hw/riscv/virt.c (finalize_fdt), the new else branch calls
create_fdt_reset, a symbol with no declaration or definition anywhere
in
the tree at this commit. Since rpmi defaults to off, this is the
default
path, so every qemu-system-riscv64/riscv32 build of this revision
fails:
the implicit declaration is a hard error under -Werror, and the link
would fail regardless. A later series patch removes the call by
folding
reset-node control into riscv_create_fdt_syscon, fixing the final
state,
but every patch in a series must compile on its own or git-bisect
breaks.
Move the reset-node handling into this patch or drop the else branch.
- [LOW] Commit message for hw/riscv/virt.c rpmi option overclaims and
under-justifies. The RISC-V subsystem rules require justification for
new
board features, but no use case (e.g. RPMI client or firmware
testing) is
given. It claims the patch emits device-tree nodes (plural), yet only
a
single mailbox node is added here; service nodes arrive in later
patches.
It claims the option requires TCG plus librpmi, but the code only
rejects
KVM and non-librpmi builds, never positively requiring TCG. It also
omits
that the reset FDT node is made conditional on !have_rpmi.
- [LOW] In hw/riscv/virt.c (create_fdt_rpmi), the msi_phandle parameter
receives msi_pcie_phandle from finalize_fdt but is never referenced
in
the body, here or at the end of the series. Passing an MSI phandle
implies the RPMI mailbox is MSI-wired when the transport uses a
doorbell
MMIO region; drop the parameter. nit: virt_rpmi_config(s, NULL, 0) is
called only to fetch shmem/doorbell addresses; reading s->memmap
directly
would avoid coupling the FDT path to the device-config builder.
- [LOW] In hw/riscv/virt.c (virt_machine_class_init), the rpmi property
description says only Set on/off to enable/disable RISC-V RPMI
devices,
while the adjacent aclint property carries a (TCG only) qualifier.
Users discovering the option via -machine virt,help or QMP get no
hint
that enabling it aborts machine init under KVM or non-librpmi builds;
add the constraint to the description, still missing at series end.
Which seems to be true (I didn't test to make sure though).
I see the call to create_fdt_reset() in this patch is removed in patch
5. Please make sure you test all patches before sending them
Alistair
> ---
> hw/riscv/Kconfig | 1 +
> hw/riscv/virt.c | 96
> +++++++++++++++++++++++++++++++++++++++++
> include/hw/riscv/virt.h | 3 ++
> 3 files changed, 100 insertions(+)
>
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index d06ac26648..c3f9d7580b 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -49,6 +49,7 @@ config RISCV_VIRT
> imply VIRTIO_VGA
> imply TEST_DEVICES
> imply TPM_TIS_SYSBUS
> + imply RISCV_RPMI
> select DEVICE_TREE
> select RISCV_NUMA
> select GOLDFISH_RTC
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d58656f70d..c822cc15ca 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -44,6 +44,8 @@
> #include "hw/intc/riscv_aplic.h"
> #include "hw/intc/sifive_plic.h"
> #include "hw/misc/sifive_test.h"
> +#include "hw/misc/riscv_rpmi.h"
> +#include "hw/riscv/rpmi-fdt.h"
> #include "hw/core/platform-bus.h"
> #include "chardev/char.h"
> #include "system/device_tree.h"
> @@ -100,6 +102,8 @@ static const MemMapEntry virt_memmap[] = {
> /* UART1 supports page isolation from UART0 */
> [VIRT_UART1] = { 0x1000a000, 0x100 },
> [VIRT_FW_CFG] = { 0x10100000, 0x18 },
> + [VIRT_RPMI_SHMEM] = { 0x10200000, 0x20000 },
> + [VIRT_RPMI_DOORBELL] = { 0x10230000, 0x1000 },
> [VIRT_FLASH] = { 0x20000000, 0x4000000 },
> [VIRT_IMSIC_M] = { 0x24000000, VIRT_IMSIC_MAX_SIZE },
> [VIRT_IMSIC_S] = { 0x28000000, VIRT_IMSIC_MAX_SIZE },
> @@ -988,6 +992,40 @@ static void create_fdt_iommu(RISCVVirtState *s,
> uint16_t bdf)
> s->pci_iommu_bdf = bdf;
> }
>
> +
> +static RiscvRpmiConfig virt_rpmi_config(RISCVVirtState *s,
> + const uint32_t *hart_ids,
> + uint32_t hart_count)
> +{
> + return (RiscvRpmiConfig) {
> + .doorbell_base = s->memmap[VIRT_RPMI_DOORBELL].base,
> + .shmem_base = s->memmap[VIRT_RPMI_SHMEM].base,
> + .shmem_size = s->memmap[VIRT_RPMI_SHMEM].size,
> + .a2p_req_size = VIRT_RPMI_A2P_REQ_SIZE,
> + .p2a_req_size = VIRT_RPMI_P2A_REQ_SIZE,
> + .platform_info = "QEMU RISC-V virt RPMI",
> + .hart_ids = hart_ids,
> + .hart_count = hart_count,
> + };
> +}
> +
> +static void create_fdt_rpmi(RISCVVirtState *s, uint32_t *phandle,
> + uint32_t msi_phandle)
> +{
> + RiscvRpmiConfig rpmi_cfg = virt_rpmi_config(s, NULL, 0);
> + uint32_t rpmi_mbox_handle;
> + RiscvRpmiFdtMboxConfig cfg = {
> + .shmem_base = rpmi_cfg.shmem_base,
> + .doorbell_base = rpmi_cfg.doorbell_base,
> + .a2p_req_size = rpmi_cfg.a2p_req_size,
> + .p2a_req_size = rpmi_cfg.p2a_req_size,
> + .doorbell_size = s->memmap[VIRT_RPMI_DOORBELL].size,
> + };
> +
> + riscv_rpmi_fdt_add_mbox(MACHINE(s)->fdt, &cfg, phandle,
> + &rpmi_mbox_handle);
> +}
> +
> static void finalize_fdt(RISCVVirtState *s)
> {
> uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle =
> 1;
> @@ -1007,6 +1045,10 @@ static void finalize_fdt(RISCVVirtState *s)
> create_fdt_pcie(s, irq_pcie_phandle, msi_pcie_phandle,
> iommu_sys_phandle);
>
> + if (s->have_rpmi) {
> + create_fdt_rpmi(s, &phandle, msi_pcie_phandle);
> + }
> +
> create_fdt_reset(s, &phandle);
>
> create_fdt_uarts(s, irq_mmio_phandle);
> @@ -1334,6 +1376,18 @@ static void virt_machine_init(MachineState
> *machine)
> exit(1);
> }
>
> +
> + if (s->have_rpmi) {
> +#ifndef CONFIG_LIBRPMI
> + error_report("RISC-V RPMI support is not compiled in");
> + exit(1);
> +#else
> + if (kvm_enabled()) {
> + error_report("RISC-V RPMI support is not available with
> KVM");
> + exit(1);
> + }
> +#endif
> + }
> /* Initialize sockets */
> mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
> for (i = 0; i < socket_count; i++) {
> @@ -1484,6 +1538,25 @@ static void virt_machine_init(MachineState
> *machine)
> /* SiFive Test MMIO device */
> sifive_test_create(s->memmap[VIRT_TEST].base);
>
> + if (s->have_rpmi) {
> + MachineClass *mc = MACHINE_GET_CLASS(machine);
> + const CPUArchIdList *possible_cpus = mc-
> >possible_cpu_arch_ids(machine);
> + g_autofree uint32_t *rpmi_hart_ids =
> + g_new0(uint32_t, machine->smp.cpus);
> + RiscvRpmiConfig rpmi_cfg;
> + Error *local_err = NULL;
> +
> + for (i = 0; i < machine->smp.cpus; i++) {
> + rpmi_hart_ids[i] = possible_cpus->cpus[i].arch_id;
> + }
> +
> + rpmi_cfg = virt_rpmi_config(s, rpmi_hart_ids, machine-
> >smp.cpus);
> + if (!riscv_rpmi_create(&rpmi_cfg, &local_err)) {
> + error_report_err(local_err);
> + exit(1);
> + }
> + }
> +
> /* VirtIO MMIO devices */
> for (i = 0; i < VIRTIO_COUNT; i++) {
> sysbus_create_simple("virtio-mmio",
> @@ -1575,6 +1648,7 @@ static void virt_machine_instance_init(Object
> *obj)
>
> s->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
> s->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
> + s->have_rpmi = false;
> s->acpi = ON_OFF_AUTO_AUTO;
> s->iommu_sys = ON_OFF_AUTO_AUTO;
> s->num_sources = VIRT_IRQCHIP_NUM_SOURCES;
> @@ -1650,6 +1724,21 @@ static void virt_set_aclint(Object *obj, bool
> value, Error **errp)
> s->have_aclint = value;
> }
>
> +
> +static bool virt_get_rpmi(Object *obj, Error **errp)
> +{
> + RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> +
> + return s->have_rpmi;
> +}
> +
> +static void virt_set_rpmi(Object *obj, bool value, Error **errp)
> +{
> + RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> +
> + s->have_rpmi = value;
> +}
> +
> bool virt_is_iommu_sys_enabled(RISCVVirtState *s)
> {
> return s->iommu_sys == ON_OFF_AUTO_ON;
> @@ -1771,6 +1860,13 @@ static void
> virt_machine_class_init(ObjectClass *oc, const void *data)
> "enable/disable emulating
> "
> "ACLINT devices");
>
> +
> + object_class_property_add_bool(oc, "rpmi", virt_get_rpmi,
> + virt_set_rpmi);
> + object_class_property_set_description(oc, "rpmi",
> + "Set on/off to
> enable/disable "
> + "RISC-V RPMI devices");
> +
> object_class_property_add_str(oc, "aia", virt_get_aia,
> virt_set_aia);
> object_class_property_set_description(oc, "aia",
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 7c862b0da2..76b5a57001 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -55,6 +55,7 @@ struct RISCVVirtState {
>
> int fdt_size;
> bool have_aclint;
> + bool have_rpmi;
> RISCVVirtAIAType aia_type;
> int aia_guests;
> char *oem_id;
> @@ -82,6 +83,8 @@ enum {
> VIRT_VIRTIO,
> VIRT_UART1,
> VIRT_FW_CFG,
> + VIRT_RPMI_SHMEM,
> + VIRT_RPMI_DOORBELL,
> VIRT_IMSIC_M,
> VIRT_IMSIC_S,
> VIRT_FLASH,
