On Thu, Aug 21, 2025 at 18:36:44 +0800, Bill Xiang wrote:
> > 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.
Weird. The testsuite works okay for me, also it works okay in the
upstream CI.
Let us know if you encounter any problems, but don't forget to mention
the exact steps and setup needed to reproduce the issue (same way as
when reporting a bug).
>
> >
> > > 8 files changed, 67 insertions(+), 3 deletions(-)
> > >
[...]
> > > @@ -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.
It doesn't really matter that it's currently unsupported. Support can be
added later in qemu. If it is a guest visible feature it must be handled
in the ABI stability check. If the setting doesn't influence anything
guest visible then it is okay.
I noted this here because the feature has an additional setting and you
didn't handle it here. If it is not guest visible you'll need to node in
a comment stating that this particular config is not guest visible for
anyone coming after you wanting to understand why it works this way.
>
> > > @@ -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?
I was pointing out the discrepancy between the docs + RNG schema, which
declared it as optional and the code in the parser, which didn't handle
the possibilty that it's missing (passed it to strcmp, which would
crash) and the formatter, which didn't handle it being NULL and would
pass NULL to the formatter (this wouldn't crash, but would create
invalid config).
As noted I'm not really interested in the feature so I don't know if
mirroring existing code makes sense. I merely pointed out that the
implementation as documented was wrong.
