On 2025/8/25 16:12, Peter Krempa wrote:
> On Fri, Aug 22, 2025 at 17:53:28 +0800, BillXiang wrote:
>> From: xiangwencheng <xiangwench...@lanxincomputing.com>
>>
>> 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>
>> ---
>> v1[1] -> v2:
>> - Separate changes to NEWS.rst into a separate commit.
>> - Remove changes to qemuxmlconfdata files that are used by x86.
>> - Store riscv-aia mode as enum.
>> - Remove VIR_DOMAIN_KVM_RISCV_AIA from
>> virDomainDefFeaturesCheckABIStability
>> beacuse it doesn't support migration currently.
>>
>> [1]
>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XBDTP6C5T2RIVHGEI3X7WTDMHSXZMGKL
>> ---
>> docs/formatdomain.rst | 2 ++
>> src/conf/domain_conf.c | 33 ++++++++++++++++++++++++++++++-
>> src/conf/domain_conf.h | 10 ++++++++++
>> src/conf/schemas/domaincommon.rng | 12 +++++++++++
>> src/libvirt_private.syms | 2 ++
>> src/qemu/qemu_command.c | 13 +++++++++---
>> 6 files changed, 68 insertions(+), 4 deletions(-)
>
> Fails to compile:
>
> In file included from ../../../libvirt/src/util/virxml.h:31,
> from ../../../libvirt/src/conf/cpu_conf.h:24,
> from ../../../libvirt/src/conf/capabilities.h:26,
> from ../../../libvirt/src/conf/domain_conf.h:31,
> from ../../../libvirt/src/conf/checkpoint_conf.h:26,
> from ../../../libvirt/src/conf/domain_conf.c:32:
> ../../../libvirt/src/conf/domain_conf.c:230:15: error: no previous prototype
> for ‘virDomainKVMRiscvAIAModeTypeToString’ [-Werror=missing-prototypes]
> 230 | VIR_ENUM_IMPL(virDomainKVMRiscvAIAMode,
> | ^~~~~~~~~~~~~~~~~~~~~~~~
> ../../../libvirt/src/util/virenum.h:35:17: note: in definition of macro
> ‘VIR_ENUM_IMPL’
> 35 | const char *name ## TypeToString(int type) { \
> | ^~~~
> ../../../libvirt/src/conf/domain_conf.c:230:15: error: no previous prototype
> for ‘virDomainKVMRiscvAIAModeTypeFromString’ [-Werror=missing-prototypes]
> 230 | VIR_ENUM_IMPL(virDomainKVMRiscvAIAMode,
> | ^~~~~~~~~~~~~~~~~~~~~~~~
> ../../../libvirt/src/util/virenum.h:40:9: note: in definition of macro
> ‘VIR_ENUM_IMPL’
> 40 | int name ## TypeFromString(const char *type) { \
> | ^~~~
> ../../../libvirt/src/conf/domain_conf.c: In function
> ‘virDomainDefFeaturesCheckABIStability’:
> ../../../libvirt/src/conf/domain_conf.c:21712:13: error: enumeration value
> ‘VIR_DOMAIN_KVM_RISCV_AIA’ not handled in switch [-Werror=switch]
> 21712 | switch ((virDomainKVM) i) {
> | ^~~~~~
> cc1: all warnings being treated as errors
> ninja: build stopped: subcommand failed.
>
> Looks like you're missing some VIR_ENUM_DECL()
Got it—please disregard my last reply of this.
>
> Couple more points inline. I'll re-arrange some hunks for clarity.
>
>
>>
>> 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``
>
> Stating that default is auto ...
>
>
>> @@ -17142,6 +17150,7 @@ virDomainFeaturesKVMDefParse(virDomainDef *def,
>> {
>> g_autoptr(GPtrArray) feats = virXMLNodeGetSubelementList(node, NULL);
>> size_t i;
>> + g_autofree char *ptval = NULL;
>
> Unused variable. Compiler will catch this.
>
>>
>> if (!def->kvm_features)
>> def->kvm_features = g_new0(virDomainFeatureKVM, 1);
>
> The enum is defined as:
>
>> +typedef enum {
>> + VIR_DOMAIN_KVM_TISCV_AIA_MODE_EMUL = 0,
>> + VIR_DOMAIN_KVM_TISCV_AIA_MODE_HWACCEL,
>> + VIR_DOMAIN_KVM_TISCV_AIA_MODE_AUTO,
>> +
>> + VIR_DOMAIN_KVM_TISCV_AIA_MODE_LAST
>> +} virDomainKVMRiscvAIAMode;
>> +
>
>
>
>> @@ -17183,6 +17192,15 @@ virDomainFeaturesKVMDefParse(virDomainDef *def,
>> return -1;
>> }
>> }
>> +
>> + if (feature == VIR_DOMAIN_KVM_RISCV_AIA &&
>> + value == VIR_TRISTATE_SWITCH_ON) {
>> + if (virXMLPropEnum(feat, "mode",
>> + virDomainKVMRiscvAIAModeTypeFromString,
>> + VIR_XML_PROP_NONZERO,
>
> Here you:
> 1) allow the "mode" property to be missing, in which case the value will
> be kept '0' thus VIR_DOMAIN_KVM_TISCV_AIA_MODE_EMUL
> 2) you explicitly refuse VIR_DOMAIN_KVM_TISCV_AIA_MODE_EMUL case when
> xml contains 'mode="emul"'
>
>> + &def->kvm_features->kvm_riscv_aia_mode) < 0)
>> + return -1;
>> + }
>> }
>>
>> def->features[VIR_DOMAIN_FEATURE_KVM] = VIR_TRISTATE_SWITCH_ON;
>> @@ -28752,7 +28770,20 @@ 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->kvm_riscv_aia_mode >= 0 &&
>
>
> Here you accept 0
>
>> + def->kvm_features->kvm_riscv_aia_mode <
>> VIR_DOMAIN_KVM_TISCV_AIA_MODE_LAST) {
>
> This shouldn't be needed.
>
>> + virBufferAsprintf(&childBuf, " mode='%s'/>\n",
>> +
>> virDomainKVMRiscvAIAModeTypeToString(def->kvm_features->kvm_riscv_aia_mode));
>> + } else {
>> + virBufferAddLit(&childBuf, "/>\n");
>> + }
>> + }
>> + break;
>> case VIR_DOMAIN_KVM_LAST:
>> break;
>> }
>> 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/libvirt_private.syms b/src/libvirt_private.syms
>> index b846011f0f..f540bdbd5b 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -501,6 +501,8 @@ virDomainIOThreadIDDel;
>> virDomainIOThreadIDFind;
>> virDomainKeyWrapCipherNameTypeFromString;
>> virDomainKeyWrapCipherNameTypeToString;
>> +virDomainKVMRiscvAIAModeTypeFromString;
>> +virDomainKVMRiscvAIAModeTypeToString;
>> virDomainLaunchSecurityTypeFromString;
>> virDomainLaunchSecurityTypeToString;
>> virDomainLeaseDefFree;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index e8de386f30..9f34b5a37d 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",
>> virDomainKVMRiscvAIAModeTypeToString(def->kvm_features->kvm_riscv_aia_mode));
>> + }
>
> This still doesn't handle the situation where the setting is missing
> from the XML correctly.
>
> If qemu requires a mode, make it mandatory, do not declare deafults in
> libvirt.
>
>
>> }
>> break;
>>
>> --
>> 2.34.1
>>
>
