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 <xiangwench...@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 > 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. > 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. > 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. > + > + 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. > @@ -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. > + } > } > 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 >
