Re: [PATCH RFC 1/2] qemu: use "-accel" option to specify accelerator instead of "-machine"

[Hyman <huangy81@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxx/
> > >           
> > >           
> > >           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