On 2025/8/25 15:58, Peter Krempa wrote:
> 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.
>
Perhaps I’ve misinterpreted 'guest visible'. The mode is visible to
QEMU itself as described in the commit, not to the guest kernel.
>>
>>>> @@ -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.
>
