Hi, all,
    I wound like to thank you all for reviewing the code so promptly.

    As discussed earlier. The following conclusions can be drawn :

    1. This unfortunately cannot be done unconditionally.  You need to probe for the availability of -accel.
    -------------------------------------------------------------------
    indeed, i haven't considered the compatibility if '-accel' option can not be used. In the next version
    the 'accel cap' like { "accel", NULL, QEMU_CAPS_ACCEL } will be introduced to help to choose the right option between the two code paths

    2. Either way, please split the qemuBuildAccelCommandLine function separation from the actual functional changes to make it easier to see what's going on.
    -------------------------------------------------------------------
    i'll do the code clean like the following:

firsh patch:

+static void
+qemuBuildAccelCommandLineTcgOptions(void)
+{
+    /* TODO: build command line tcg accelerator
+     * property like tb-size */
+}
+
+
+static void
+qemuBuildAccelCommandLineKvmOptions(void)
+{
+    /* implemented in the next patch */
+}
+
+
+static int
+qemuBuildAccelCommandLine(virCommandPtr cmd,
+                          const virDomainDef *def)
+{
+    /* the '-machine' options for accelerator are legacy,
+     * using the '-accel' options by default */
+    g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+    virCommandAddArg(cmd, "-accel");
+
+    switch ((virDomainVirtType)def->virtType) {
+    case VIR_DOMAIN_VIRT_QEMU:
+        virBufferAddLit(&buf, "tcg");
+        qemuBuildAccelCommandLineTcgOptions();
+        break;
+
+    case VIR_DOMAIN_VIRT_KVM:
+        virBufferAddLit(&buf, "kvm");
+        qemuBuildAccelCommandLineKvmOptions();

second patch:

 static void
-qemuBuildAccelCommandLineKvmOptions(void)
+qemuBuildAccelCommandLineKvmOptions(virBuffer *buf,
+                                    const virDomainDef *def)
 {
-    /* implemented in the next patch */
+    if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON &&
+        def->kvm_features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON) {
+        virBufferAsprintf(buf, ",dirty-gfn-count=%d", def->dirty_gfn_count);
+    }
 }
 
 
@@ -7217,7 +7224,7 @@ qemuBuildAccelCommandLine(virCommandPtr cmd,
 
     case VIR_DOMAIN_VIRT_KVM:
         virBufferAddLit(&buf, "kvm");
-        qemuBuildAccelCommandLineKvmOptions();
+        qemuBuildAccelCommandLineKvmOptions(&buf, def);
         break;

    3. This is going to break use of the /usr/bin/qemu-kvm wrapper on Fedora because QEMU refuses to allow -accel combined with -machine
    -----------------------------------------------------------------
    It seems that the question is not settled. The next version patch will switch the option as previous.

The patch will be posted tomorrow later if things go smoothly.

在 2021/1/11 17:51, Paolo Bonzini 写道:
On 11/01/21 10:38, Daniel P. Berrangé wrote:

The "-machine" options for accelerators are legacy, the "-accel" options
is a better mechanism. The following are the details:
https://lore.kernel.org/qemu-devel/3aa73987-40e8-3619-0723-9f17f73850bd@redhat.com/

This patch switch the option "-machine accel=xxx" to "-accel xxx" when
specifying accelerator type once libvirt build QEMU command line.
This is going to break use of the /usr/bin/qemu-kvm wrapper on
Fedora because QEMU refuses to allow -accel combined with -machine

$ /usr/bin/qemu-kvm -accel tc
qemu-system-x86_64: The -accel and "-machine accel=" options are incompatible

That script is useless, a symlink will do ever since QEMU 4.0.  Fedora can and should get rid of it, I'll take a look.

Is there any other distro that does the same?  Libvirt cannot use newer KVM features with "-machine accel".

Paolo

Best Regards !

Hyman