<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">Hi, all,</div>
<div class="moz-cite-prefix"> I wound like to thank you all for
reviewing the code so promptly. <br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"> As discussed earlier. The following
conclusions can be drawn :</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"> 1. This unfortunately cannot be
done unconditionally. You need to probe for the availability of
-accel.</div>
<div class="moz-cite-prefix">
-------------------------------------------------------------------</div>
<div class="moz-cite-prefix"> indeed, i haven't considered the
compatibility if '-accel' option can not be used. In the next
version</div>
<div class="moz-cite-prefix"> the 'accel cap' like { "accel",
NULL, QEMU_CAPS_ACCEL } will be introduced to help to choose the
right option between the two code paths</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"> 2. Either way, please split the
qemuBuildAccelCommandLine function
separation from the actual functional changes to make it easier to
see what's going on. <br>
</div>
<div class="moz-cite-prefix">
-------------------------------------------------------------------</div>
<div class="moz-cite-prefix"> i'll do the code clean like the
following:</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">firsh patch:<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">+static void<br>
+qemuBuildAccelCommandLineTcgOptions(void)<br>
+{<br>
+ /* TODO: build command line tcg accelerator<br>
+ * property like tb-size */<br>
+}<br>
+<br>
+<br>
+static void<br>
+qemuBuildAccelCommandLineKvmOptions(void)<br>
+{<br>
+ /* implemented in the next patch */<br>
+}<br>
+<br>
+<br>
+static int<br>
+qemuBuildAccelCommandLine(virCommandPtr cmd,<br>
+ const virDomainDef *def)<br>
+{<br>
+ /* the '-machine' options for accelerator are legacy,<br>
+ * using the '-accel' options by default */<br>
+ g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;<br>
+ virCommandAddArg(cmd, "-accel");<br>
+<br>
+ switch ((virDomainVirtType)def->virtType) {<br>
+ case VIR_DOMAIN_VIRT_QEMU:<br>
+ virBufferAddLit(&buf, "tcg");<br>
+ qemuBuildAccelCommandLineTcgOptions();<br>
+ break;<br>
+<br>
+ case VIR_DOMAIN_VIRT_KVM:<br>
+ virBufferAddLit(&buf, "kvm");<br>
+ qemuBuildAccelCommandLineKvmOptions();<br>
<br>
</div>
<div class="moz-cite-prefix">second patch:</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"> static void<br>
-qemuBuildAccelCommandLineKvmOptions(void)<br>
+qemuBuildAccelCommandLineKvmOptions(virBuffer *buf,<br>
+ const virDomainDef *def)<br>
{<br>
- /* implemented in the next patch */<br>
+ if (def->features[VIR_DOMAIN_FEATURE_KVM] ==
VIR_TRISTATE_SWITCH_ON &&<br>
+ def->kvm_features[VIR_DOMAIN_KVM_DIRTY_RING] ==
VIR_TRISTATE_SWITCH_ON) {<br>
+ virBufferAsprintf(buf, ",dirty-gfn-count=%d",
def->dirty_gfn_count);<br>
+ }<br>
}<br>
<br>
<br>
@@ -7217,7 +7224,7 @@ qemuBuildAccelCommandLine(virCommandPtr cmd,<br>
<br>
case VIR_DOMAIN_VIRT_KVM:<br>
virBufferAddLit(&buf, "kvm");<br>
- qemuBuildAccelCommandLineKvmOptions();<br>
+ qemuBuildAccelCommandLineKvmOptions(&buf, def);<br>
break;<br>
<br>
<span style="color: rgb(51, 133, 255); font-family: Arial,
'Microsoft YaHei', '\\5FAE软雅黑', '\\5B8B体', 'Malgun Gothic',
Meiryo, sans-serif; font-size: 14.000000953674316px; font-style:
normal; font-variant: normal; font-weight: normal;
letter-spacing: normal; line-height: 14.000000953674316px;
orphans: auto; text-align: left; text-indent: 0px;
text-transform: none; white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(238, 238, 238); display: inline
!important; float: none;"></span></div>
<div class="moz-cite-prefix"> 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</div>
<div class="moz-cite-prefix">
-----------------------------------------------------------------</div>
<div class="moz-cite-prefix"> It seems that the question is not
settled. The next version patch will switch the option as
previous.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">The patch will be posted tomorrow later
if things go smoothly.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">在 2021/1/11 17:51, Paolo Bonzini 写道:<br>
</div>
<blockquote type="cite"
cite="mid:1460a128-585d-89d2-022c-149ebd2431c5@redhat.com">On
11/01/21 10:38, Daniel P. Berrangé wrote:
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
The "-machine" options for accelerators are legacy, the
"-accel" options
<br>
is a better mechanism. The following are the details:
<br>
<a class="moz-txt-link-freetext" href="https://lore.kernel.org/qemu-devel/3aa73987-40e8-3619-0723-9f17f73850bd@redhat.com/">https://lore.kernel.org/qemu-devel/3aa73987-40e8-3619-0723-9f17f73850bd@redhat.com/</a>
<br>
<br>
This patch switch the option "-machine accel=xxx" to "-accel
xxx" when
<br>
specifying accelerator type once libvirt build QEMU command
line.
<br>
</blockquote>
This is going to break use of the /usr/bin/qemu-kvm wrapper on
<br>
Fedora because QEMU refuses to allow -accel combined with
-machine
<br>
<br>
$ /usr/bin/qemu-kvm -accel tc
<br>
qemu-system-x86_64: The -accel and "-machine accel=" options are
incompatible
<br>
</blockquote>
<br>
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.
<br>
<br>
Is there any other distro that does the same? Libvirt cannot use
newer KVM features with "-machine accel".
<br>
<br>
Paolo
<br>
<br>
</blockquote>
<p>Best Regards !</p>
<p>Hyman<br>
</p>
</body>
</html>