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

[Hyman]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210112/3ca9e289/attachment-0001.htm>