From: "Peter Krempa"<pkrempa@redhat.com> Date: Wed, Aug 20, 2025, 16:08 Subject: Re: [PATCH] qemu: support riscv-aia feature for RISC-V KVM To: "BillXiang"<xiangwencheng@lanxincomputing.com> Cc: <devel@lists.libvirt.org> On Fri, Aug 15, 2025 at 19:44:15 +0800, BillXiang wrote:
riscv-aia feature was introduced in qemu-8.2 to specify the KVM AIA mode. The "riscv-aia" parameter is passed along with --accel in QEMU command-line. 1) "riscv-aia=emul": IMSIC is emulated by hypervisor 2) "riscv-aia=hwaccel": use hardware guest IMSIC 3) "riscv-aia=auto": use the hardware guest IMSICs whenever available otherwise we fallback to software emulation. This patch add the corresponding feature named 'riscv-aia'. Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com> --- Note that in this review I'll note some common mistakes from this patch. I don't know what this feature is about so I'll not comment on that
Hi Peter, Thank you for your review. I've a few points I'd like to clarify, could you please provide a bit more detail?
NEWS.rst | 10 ++++++++ We mandate that changes to NEWS.rst are always separated into a separate commit with no other changes. This is to avoid conflicts both when applying the code (which is the case with this patch which can't be applied due to a conflict in the news) and also for backports.
I'll separate it into a separate patch.
docs/formatdomain.rst | 2 ++ src/conf/domain_conf.c | 29 ++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/conf/schemas/domaincommon.rng | 12 +++++++++ src/qemu/qemu_command.c | 13 +++++++--- tests/qemuxmlconfdata/kvm-features-off.xml | 1 + tests/qemuxmlconfdata/kvm-features.xml | 1 + You've modified test input files but apparently didn't run the test suite, because it now fails: Summary of Failures: 302/312 libvirt:syntax-check / trailing_blank FAIL 0.50s exit status 2 311/312 libvirt:bin / qemuxmlconftest FAIL 4.11s exit status 1 See https://libvirt.org/advanced-tests.html namely VIR_TEST_REGENERATE_OUTPUT to avoid needing to do it manually.
I've run the test suite but get lots of TIMEOUT. I'll try again.
8 files changed, 67 insertions(+), 3 deletions(-) diff --git a/NEWS.rst b/NEWS.rst index 435760e797..7e91f81d64 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,16 @@ v11.7.0 (unreleased) * **New features** + * Support riscv-aia feature for RISC-V KVM + + riscv-aia feature was introduced in qemu-8.2 to specify the + KVM AIA mode. The "riscv-aia" parameter is passed along with + --accel in QEMU command-line. + 1) "riscv-aia=emul": IMSIC is emulated by hypervisor + 2) "riscv-aia=hwaccel": use hardware guest IMSIC + 3) "riscv-aia=auto": use the hardware guest IMSICs whenever available + otherwise we fallback to software emulation. + * **Improvements** * **Bug fixes** diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 9f7311b6d5..2ccbb1bfb9 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2085,6 +2085,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. <poll-control state='on'/> <pv-ipi state='off'/> <dirty-ring state='on' size='4096'/> + <riscv-aia state='on' mode='auto'/> </kvm> <xen> <e820_host state='on'/> @@ -2206,6 +2207,7 @@ are: poll-control Decrease IO completion latency by introducing a grace period of busy waiting on, off :since:`6.10.0 (QEMU 4.2)` pv-ipi Paravirtualized send IPIs on, off :since:`7.10.0 (QEMU 3.1)` dirty-ring Enable dirty ring feature on, off; size - must be power of 2, range [1024,65536] :since:`8.0.0 (QEMU 6.1)` + riscv-aia Set riscv KVM AIA mode. Defaults to 'auto'. on, off; mode - optional string emul, hwaccel or auto :since:`11.7.0 (QEMU 8.2)` ============== ============================================================================ ====================================================== ============================ ``xen`` diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7766e302ec..8ed65d1794 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -224,6 +224,7 @@ VIR_ENUM_IMPL(virDomainKVM, "poll-control", "pv-ipi", "dirty-ring", + "riscv-aia", ); VIR_ENUM_IMPL(virDomainXen, @@ -17183,6 +17184,20 @@ virDomainFeaturesKVMDefParse(virDomainDef *def, return -1; } } + + if (feature == VIR_DOMAIN_KVM_RISCV_AIA && + value == VIR_TRISTATE_SWITCH_ON) { + def->kvm_features->riscv_aia_mode = virXMLPropString(feat, "mode"); + + if (strcmp(def->kvm_features->riscv_aia_mode, "emul") && + strcmp(def->kvm_features->riscv_aia_mode, "hwaccel") && + strcmp(def->kvm_features->riscv_aia_mode, "auto")) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("riscv aia must be emul, hwaccel or auto")); For constructs like this I'd suggest to declare an enum and store it as an enum value rather than a string. In addition we don't allow use of strcmp and requre use of STREQ/STRNEQ Also the string copied into 'riscv_aia_mode' is not freed, thus leaked. This iwll be avoided by using the enum.
I'll change it to enum.
+ + return -1; + } + } } def->features[VIR_DOMAIN_FEATURE_KVM] = VIR_TRISTATE_SWITCH_ON; @@ -21697,6 +21712,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, case VIR_DOMAIN_KVM_POLLCONTROL: case VIR_DOMAIN_KVM_PVIPI: case VIR_DOMAIN_KVM_DIRTY_RING: + case VIR_DOMAIN_KVM_RISCV_AIA: if (src->kvm_features->features[i] != dst->kvm_features->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of KVM feature '%1$s' differs: source: '%2$s', destination: '%3$s'"), Is the 'mode' guest visible? I'd expect that it's checked here too if it is guest visible.
The check here is for VM migration. But AFAIK,the RISC-V 'riscv_aia' feature requires the 'aia' feature being set to 'aplic-imsic', which currently lacks support for migration or snapshotting.
@@ -28752,6 +28768,19 @@ virDomainDefFormatFeatures(virBuffer *buf, } } break; + case VIR_DOMAIN_KVM_RISCV_AIA: + if (def->kvm_features->features[j] != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&childBuf, "<%s state='%s'", + virDomainKVMTypeToString(j), + virTristateSwitchTypeToString(def->kvm_features->features[j])); + if (def->kvm_features->riscv_aia_mode != NULL) { + virBufferAsprintf(&childBuf, " mode='%s'/>\n", + def->kvm_features->riscv_aia_mode); + } else { + virBufferAddLit(&childBuf, "/>\n"); + } + } + break; case VIR_DOMAIN_KVM_LAST: break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index eca820892e..0d749c0f3c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2270,6 +2270,7 @@ typedef enum { VIR_DOMAIN_KVM_POLLCONTROL, VIR_DOMAIN_KVM_PVIPI, VIR_DOMAIN_KVM_DIRTY_RING, + VIR_DOMAIN_KVM_RISCV_AIA, VIR_DOMAIN_KVM_LAST } virDomainKVM; @@ -2476,6 +2477,7 @@ struct _virDomainFeatureKVM { int features[VIR_DOMAIN_KVM_LAST]; unsigned int dirty_ring_size; /* size of dirty ring for each vCPU, no units */ + char *riscv_aia_mode; }; typedef struct _virDomainFeatureTCG virDomainFeatureTCG; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index e369fb6e81..3de5807b1e 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -8192,6 +8192,18 @@ </optional> </element> </optional> + <optional> + <element name="riscv-aia"> + <ref name="featurestate"/> + <optional> + <attribute name="mode"> + <data type="string"> + <param name="pattern">(emul|hwaccel|auto)</param> + </data> + </attribute> + </optional> + </element> + </optional> </interleave> </element> </define> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e8de386f30..6db283ef29 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6679,6 +6679,9 @@ qemuBuildCpuCommandLine(virCommand *cmd, case VIR_DOMAIN_KVM_DIRTY_RING: break; + + case VIR_DOMAIN_KVM_RISCV_AIA: + break; case VIR_DOMAIN_KVM_LAST: break; @@ -7314,9 +7317,13 @@ qemuBuildAccelCommandLine(virCommand *cmd, * not that either kvm or tcg can be specified by libvirt * so do not worry about the conflict of specifying both * */ - if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON && - def->kvm_features->features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON) { - virBufferAsprintf(&buf, ",dirty-ring-size=%d", def->kvm_features->dirty_ring_size); + if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) { + if (def->kvm_features->features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON) { + virBufferAsprintf(&buf, ",dirty-ring-size=%d", def->kvm_features->dirty_ring_size); + } + if (def->kvm_features->features[VIR_DOMAIN_KVM_RISCV_AIA] == VIR_TRISTATE_SWITCH_ON) { + virBufferAsprintf(&buf, ",riscv-aia=%s", def->kvm_features->riscv_aia_mode); The mode is declared as optional, here you use it unconditionally.
I've mirrored the implementation of VIR_DOMAIN_KVM_DIRTY_RING and checked VIR_DOMAIN_KVM_RISCV_AIA with VIR_TRISTATE_SWITCH_ON. Could you let me know what additional condition I should introduce?
+ } } break; diff --git a/tests/qemuxmlconfdata/kvm-features-off.xml b/tests/qemuxmlconfdata/kvm-features-off.xml index 3cd4ff18f2..2340a93ce5 100644 --- a/tests/qemuxmlconfdata/kvm-features-off.xml +++ b/tests/qemuxmlconfdata/kvm-features-off.xml @@ -16,6 +16,7 @@ <poll-control state='off'/> <pv-ipi state='off'/> <dirty-ring state='off'/> + <riscv-aia state='off'/> </kvm> </features> <cpu mode='host-passthrough' check='none' migratable='off'/> diff --git a/tests/qemuxmlconfdata/kvm-features.xml b/tests/qemuxmlconfdata/kvm-features.xml index 78091064b1..c06b0aa58b 100644 --- a/tests/qemuxmlconfdata/kvm-features.xml +++ b/tests/qemuxmlconfdata/kvm-features.xml @@ -16,6 +16,7 @@ <poll-control state='on'/> <pv-ipi state='on'/> <dirty-ring state='on' size='4096'/> + <riscv-aia state='on' mode='auto'/> </kvm> </features> <cpu mode='host-passthrough' check='none' migratable='off'/> -- 2.46.2.windows.1