> From: "Peter Krempa"<pkre...@redhat.com> > Date: Wed, Aug 20, 2025, 16:08 > Subject: Re: [PATCH] qemu: support riscv-aia feature for RISC-V KVM > To: "BillXiang"<xiangwench...@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 <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 >
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 > > >
