Re: Module/Tool/SO to convert from KVM-xml => QEMU-native?

[Ajay Garg]

Came across this :
https://fedoraproject.org/wiki/Changes/LibvirtModularDaemons

Upon regular compilation, it looks virsh (and possibly others) are
compiled in the modular mode.

Is it possible to compile in the legacy mode, in the latest libvirt?

On Sat, Oct 9, 2021 at 12:20 AM Ajay Garg  wrote:
>
> Hi All.
>
> Following has been done :
>
> 1.
> "sudo apt-get install virt-manager"
>
> Above installed all packages required for libvirt/kvm/qemu via apt,
> and a vm was setup successfully.
> Also, the vm booting works fine, when the green start-button of
> virt-manager is clicked.
>
>
> 2.
> Next, qemu was compiled from sources, and only and only the following
> was replaced, as :
>
> "sudo cp build/qemu-system-x86_64 /usr/bin/qemu-system-x86_64"
>
>
> 3.
> Now, if the green-button on the virt-manager is clicked, following
> error is seen :
>
> ##
> Please use server=on instead
> qemu-system-x86_64: -chardev
> socket,id=charmonitor,fd=31,server,nowait: warning: short-form boolean
> option 'nowait' deprecated
> Please use wait=off instead
> qemu-system-x86_64: -spice
> port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on:
> warning: short-form boolean option 'disable-ticketing' deprecated
> Please use disable-ticketing=on instead
> ##
>
>
> So, what are the minimum libvirt modules that need to be replaced, so
> that upon booting up, the kvm-xml is converted to qemu-native in the
> latest qemu-compatible format?
>
>
> Thanks and Regards,
> Ajay

Re: Squelch 'eof from qemu monitor' error on normal VM shutdown

[Jim Fehlig]

On 10/7/21 20:45, Jim Fehlig wrote:

On 10/4/21 08:55, Michal Prívozník wrote:

On 9/30/21 7:15 PM, Jim Fehlig wrote:

On 9/29/21 21:29, Jim Fehlig wrote:

Hi All,

Likely Christian received a bug report that motivated commit
aeda1b8c56, which was later reverted by Michal with commit 72adaf2f10.
In the past, I recall being asked about "internal error: End of file
from qemu monitor" on normal VM shutdown and gave a hand wavy response
using some of Michal's words from the revert commit message.

I recently received a bug report (sorry, but no public link) from a
concerned user about this error and wondered if there is some way to
improve it? I went down some dead ends before circling back to
Christian's patch. When rebased to latest master, I cannot reproduce
the hangs reported by Michal [1]. Perhaps Nikolay's series to resolve
hangs/crashes of libvirtd [2] has now made Christian's patch viable?


Hmm, Nikolay's series improves thread management at daemon shutdown and
doesn't touch VM shutdown logic. But there has been some behavior change
from the time aeda1b8c56 was committed (3.4.0 dev cycle) to current git
master. At least I don't see libvirtd hanging when running Michal's test
on master + rebased aeda1b8c56.


After reworking the tests a bit, I still don't see libvirtd hanging, but I do 
see VM shutdown stuck in "in shutdown" state. Attaching gdb shows the following 
thread blocked waiting for a response from the monitor that will never come 
since EOF has already occurred on the socket


Thread 21 (Thread 0x7fdb557fa700 (LWP 9110) "rpc-worker"):
#0  0x7fdbc922a4dc in pthread_cond_wait@@GLIBC_2.3.2 () at 
/lib64/libpthread.so.0
#1  0x7fdbccee2310 in virCondWait (c=0x7fdba403cd40, m=0x7fdba403cd18) at 
../src/util/virthread.c:156
#2  0x7fdb87150a8b in qemuMonitorSend (mon=0x7fdba403cd00, 
msg=0x7fdb557f95b0) at ../src/qemu/qemu_monito

r.c:964
#3  0x7fdb8715fbf1 in qemuMonitorJSONCommandWithFd (mon=0x7fdba403cd00, 
cmd=0x7fdb4015bae0, scm_fd=-1, re

ply=0x7fdb557f9678) at ../src/qemu/qemu_monitor_json.c:327
#4  0x7fdb8715fda0 in qemuMonitorJSONCommand (mon=0x7fdba403cd00, 
cmd=0x7fdb4015bae0, reply=0x7fdb557f967

8) at ../src/qemu/qemu_monitor_json.c:352
#5  0x7fdb87174b71 in qemuMonitorJSONGetIOThreads (mon=0x7fdba403cd00, 
iothreads=0x7fdb557f9798, niothrea

ds=0x7fdb557f9790) at ../src/qemu/qemu_monitor_json.c:7838
#6  0x7fdb8715d059 in qemuMonitorGetIOThreads (mon=0x7fdba403cd00, 
iothreads=0x7fdb557f9798, niothreads=0

x7fdb557f9790) at ../src/qemu/qemu_monitor.c:4083
#7  0x7fdb870e8ae3 in qemuDomainGetIOThreadsMon (driver=0x7fdb6c06a4f0, 
vm=0x7fdb6c063940, iothreads=0x7f

db557f9798, niothreads=0x7fdb557f9790) at ../src/qemu/qemu_driver.c:4941
#8  0x7fdb871129bf in qemuDomainGetStatsIOThread (driver=0x7fdb6c06a4f0, 
dom=0x7fdb6c063940, params=0x7fd

b401c0cc0, privflags=1) at ../src/qemu/qemu_driver.c:18292
#9  0x7fdb871130ee in qemuDomainGetStats (conn=0x7fdb9c006760, 
dom=0x7fdb6c063940, stats=1023, record=0x7

fdb557f98d0, flags=1) at ../src/qemu/qemu_driver.c:18504
#10 0x7fdb87113526 in qemuConnectGetAllDomainStats (conn=0x7fdb9c006760, 
doms=0x0, ndoms=0, stats=1023, r

etStats=0x7fdb557f9990, flags=0) at ../src/qemu/qemu_driver.c:18598
#11 0x7fdbcd163e4e in virConnectGetAllDomainStats (conn=0x7fdb9c006760, 
stats=0, retStats=0x7fdb557f9990,

  flags=0) at ../src/libvirt-domain.c:11975
...

So indeed, reporting the error when processing monitor IO is needed to prevent 
other threads from subsequently writing to the socket. One idea I had was to 
postpone reporting the error until someone tries to write to the socket, 
although not reporting an error when it is encountered seems distasteful. I've 
been testing such a hack (attached) and along with squelching the error message, 
I no longer see VMs stuck in the "in shutdown" state after 32 iterations of the 
test. A simple rebase of aeda1b8c56 on current git master never survived more 
than a dozen iterations. I'll let the test continue to run.


The test has now survived 1134 iterations. I adjusted the patch to be more 
upstream friendly and submitted it as an RFC


https://listman.redhat.com/archives/libvir-list/2021-October/msg00484.html

I restarted the tests with this version of the patch and now at iteration 75.

Regards,
Jim

[RFC PATCH] qemu: Do not report eof when processing monitor IO

[Jim Fehlig]

There have been countless reports from users concerned about the following
error reported by libvirtd when qemu domains are shutdown

internal error: End of file from qemu monitor

While the error is harmless, users often mistaken it for real problem with
their deployments. EOF from the monitor can't be entirely ignored since
other threads may be using the monitor and must be able to detect the EOF
condition.

One potential fix is to delay reporting EOF until the monitor is used
after EOF is detected. This patch adds a 'goteof' member to the
qemuMonitor structure, which is set when EOF is detected on the monitor
socket. If another thread later tries to send data on the monitor, the
EOF error is reported.

Signed-off-by: Jim Fehlig 
---

An RFC patch to squelch qemu monitor EOF error messages on VM shutdown.
Previous discussions and information on testing of the patch can be
found in this thread

https://listman.redhat.com/archives/libvir-list/2021-September/msg00949.html

 src/qemu/qemu_monitor.c | 29 -
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 5fc23f13d3..751ec8ba6c 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -113,6 +113,7 @@ struct _qemuMonitor {
 
 /* true if qemu no longer wants 'props' sub-object of object-add */
 bool objectAddNoWrap;
+bool goteof;
 };
 
 /**
@@ -526,10 +527,10 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 {
 qemuMonitor *mon = opaque;
 bool error = false;
-bool eof = false;
 bool hangup = false;
 
 virObjectRef(mon);
+mon->goteof = false;
 
 /* lock access to the monitor and protect fd */
 virObjectLock(mon);
@@ -544,7 +545,7 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 
 if (mon->lastError.code != VIR_ERR_OK) {
 if (cond & (G_IO_HUP | G_IO_ERR))
-eof = true;
+mon->goteof = true;
 error = true;
 } else {
 if (cond & G_IO_OUT) {
@@ -562,7 +563,7 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 if (errno == ECONNRESET)
 hangup = true;
 } else if (got == 0) {
-eof = true;
+mon->goteof = true;
 } else {
 /* Ignore hangup/error cond if we read some data, to
  * give time for that data to be consumed */
@@ -575,22 +576,19 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 
 if (cond & G_IO_HUP) {
 hangup = true;
-if (!error) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("End of file from qemu monitor"));
-eof = true;
-}
+if (!error)
+mon->goteof = true;
 }
 
-if (!error && !eof &&
+if (!error && !mon->goteof &&
 cond & G_IO_ERR) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Invalid file descriptor while waiting for 
monitor"));
-eof = true;
+mon->goteof = true;
 }
 }
 
-if (error || eof) {
+if (error || mon->goteof) {
 if (hangup && mon->logFunc != NULL) {
 /* Check if an error message from qemu is available and if so, use
  * it to overwrite the actual message. It's done only in early
@@ -609,7 +607,7 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 /* Already have an error, so clear any new error */
 virResetLastError();
 } else {
-if (virGetLastErrorCode() == VIR_ERR_OK)
+if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Error while processing monitor IO"));
 virCopyLastError(>lastError);
@@ -630,7 +628,7 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 /* We have to unlock to avoid deadlock against command thread,
  * but is this safe ?  I think it is, because the callback
  * will try to acquire the virDomainObj *mutex next */
-if (eof) {
+if (mon->goteof) {
 qemuMonitorEofNotifyCallback eofNotify = mon->cb->eofNotify;
 virDomainObj *vm = mon->vm;
 
@@ -949,6 +947,11 @@ qemuMonitorSend(qemuMonitor *mon,
 virSetError(>lastError);
 return -1;
 }
+if (mon->goteof) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("End of file from qemu monitor"));
+return -1;
+}
 
 mon->msg = msg;
 qemuMonitorUpdateWatch(mon);
-- 
2.33.0

Module/Tool/SO to convert from KVM-xml => QEMU-native?

[Ajay Garg]

Hi All.

Following has been done :

1.
"sudo apt-get install virt-manager"

Above installed all packages required for libvirt/kvm/qemu via apt,
and a vm was setup successfully.
Also, the vm booting works fine, when the green start-button of
virt-manager is clicked.


2.
Next, qemu was compiled from sources, and only and only the following
was replaced, as :

"sudo cp build/qemu-system-x86_64 /usr/bin/qemu-system-x86_64"


3.
Now, if the green-button on the virt-manager is clicked, following
error is seen :

##
Please use server=on instead
qemu-system-x86_64: -chardev
socket,id=charmonitor,fd=31,server,nowait: warning: short-form boolean
option 'nowait' deprecated
Please use wait=off instead
qemu-system-x86_64: -spice
port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on:
warning: short-form boolean option 'disable-ticketing' deprecated
Please use disable-ticketing=on instead
##


So, what are the minimum libvirt modules that need to be replaced, so
that upon booting up, the kvm-xml is converted to qemu-native in the
latest qemu-compatible format?


Thanks and Regards,
Ajay

Re: [PATCH v3] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Marc-André Lureau]

Hi

On Fri, Oct 8, 2021 at 10:31 PM Stefan Berger  wrote:

>
> On 10/8/21 12:33 PM, Marc-André Lureau wrote:
>
> Hi
>
> On Fri, Oct 8, 2021 at 6:44 PM Stefan Berger 
> wrote:
>
>> Using swtpm v0.7.0 we can run swtpm_setup to create default config files
>> for swtpm_setup and swtpm-localca in session mode. Now a user can start
>> a VM with an attached TPM without having to run this program on the
>> command line before. This program needs to run once.
>>
>> This patch addresses the issue raised in
>> https://bugzilla.redhat.com/show_bug.cgi?id=2010649
>>
>> Signed-off-by: Stefan Berger 
>>
>> ---
>> v3:
>>   - Removed logfile parameter
>>
>
> I guess it's redirected to libvirt log then?
>
> So you are saying it should capture the stderr output ?
>

Something should, not sure what's the best practice today with libvirt.
Someone more familiar should say.


>
>> v2:
>>   - fixed return code if swtpm_setup doesn't support the option
>> ---
>>  src/qemu/qemu_tpm.c | 39 +++
>>  src/util/virtpm.c   |  1 +
>>  src/util/virtpm.h   |  1 +
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> index 100481503c..434c357c24 100644
>> --- a/src/qemu/qemu_tpm.c
>> +++ b/src/qemu/qemu_tpm.c
>> @@ -385,6 +385,42 @@ qemuTPMSetupEncryption(const unsigned char
>> *secretuuid,
>>  return virCommandSetSendBuffer(cmd, g_steal_pointer(),
>> secret_len);
>>  }
>>
>> +
>> +/*
>> + * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files
>> skip-if-exist
>> + */
>> +static int
>> +qemuTPMCreateConfigFiles(void)
>> +{
>> +g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
>> +g_autoptr(virCommand) cmd = NULL;
>> +int exitstatus;
>> +
>> +if (!swtpm_setup)
>> +return -1;
>>
>
> I think what Daniel was saying is that this shouldn't fail suddenly for
> users with older swtpm that already have the configuration setup.
>
>
> The above only fails if swtm_setup is not installed, if that's what you
> mean. That's when swtpm_setup is NULL.
>
> If you run `swptm_setup --create-config-files skip-if-exist` when any one
> of the 3 config files already exist, it will do nothing and exit with 0.
>
>
>
Oh I see, I misread, looks correct then


> +
>> +if (!virTPMSwtpmSetupCapsGet(
>> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
>> +return 0;
>>
>
> This is the case when swtpm_setup doesn't support --create-config-files.
> That's when the user has to use the old
> /usr/share/swtpm/swptm-create-user-config-files to create the config files,
> which will then be located at the same place that `swptm_setup
> --create-config-files skip-if-exist` would create them or check whether any
> one of them exist and skip.
>
> 3 config files will be created and if the user then removes 1 or 2 of them
> he created an invalid configuration and the start of the next VM will fail
> due to a bad configuration with missing config files. The config files
> would only be created again if all 3 were missing.
>
>
> +
>> +cmd = virCommandNew(swtpm_setup);
>> +if (!cmd)
>> +return -1;
>> +
>> +virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist",
>> NULL);
>> +virCommandClearCaps(cmd);
>> +
>> +if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("Could not run '%s' to create config files.
>> exitstatus: %d",
>> +  swtpm_setup, exitstatus);
>> +return -1;
>>
> In the error case the stderr output of swtpm_setup should probably show up
> here?
>
>
> +}
>> +
>> +return 0;
>> +}
>> +
>> +
>>  /*
>>   * qemuTPMEmulatorRunSetup
>>   *
>> @@ -432,6 +468,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
>>   "this requires privileged mode for a "
>>   "TPM 1.2\n"), 0600);
>>
>> +if (!privileged && qemuTPMCreateConfigFiles())
>> +return -1;
>> +
>>  cmd = virCommandNew(swtpm_setup);
>>  if (!cmd)
>>  return -1;
>> diff --git a/src/util/virtpm.c b/src/util/virtpm.c
>> index 1a567139b4..0f50de866c 100644
>> --- a/src/util/virtpm.c
>> +++ b/src/util/virtpm.c
>> @@ -45,6 +45,7 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature,
>>  VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
>>VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
>>"cmdarg-pwdfile-fd",
>> +  "cmdarg-create-config-files",
>>  );
>>
>>  /**
>> diff --git a/src/util/virtpm.h b/src/util/virtpm.h
>> index d021a083b4..3bb03b3b33 100644
>> --- a/src/util/virtpm.h
>> +++ b/src/util/virtpm.h
>> @@ -38,6 +38,7 @@ typedef enum {
>>
>>  typedef enum {
>>  VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD,
>> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES,
>>
>>  VIR_TPM_SWTPM_SETUP_FEATURE_LAST
>>  } virTPMSwtpmSetupFeature;
>> --
>> 2.31.1
>>
>>

Re: [PATCH v3] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

On 10/8/21 12:33 PM, Marc-André Lureau wrote:
Hi On Fri, Oct 8, 2021 at 6:44 PM Stefan Berger 
 wrote: Using swtpm v0.7.0 we can run 
swtpm_setup to create default config files for swtpm_setup and 
swtpm-localca in session mode. Now a user can start a VM with 
ZjQcmQRYFpfptBannerStart

This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
Hi

On Fri, Oct 8, 2021 at 6:44 PM Stefan Berger > wrote:


Using swtpm v0.7.0 we can run swtpm_setup to create default config
files
for swtpm_setup and swtpm-localca in session mode. Now a user can
start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.

This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649


Signed-off-by: Stefan Berger mailto:stef...@linux.ibm.com>>

---
v3:
  - Removed logfile parameter


I guess it's redirected to libvirt log then?


So you are saying it should capture the stderr output ?





v2:
  - fixed return code if swtpm_setup doesn't support the option
---
 src/qemu/qemu_tpm.c | 39 +++
 src/util/virtpm.c   |  1 +
 src/util/virtpm.h   |  1 +
 3 files changed, 41 insertions(+)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 100481503c..434c357c24 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -385,6 +385,42 @@ qemuTPMSetupEncryption(const unsigned char
*secretuuid,
     return virCommandSetSendBuffer(cmd, g_steal_pointer(),
secret_len);
 }

+
+/*
+ * qemuTPMCreateConfigFiles: run swtpm_setup
--create-config-files skip-if-exist
+ */
+static int
+qemuTPMCreateConfigFiles(void)
+{
+    g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
+    g_autoptr(virCommand) cmd = NULL;
+    int exitstatus;
+
+    if (!swtpm_setup)
+        return -1;


I think what Daniel was saying is that this shouldn't fail suddenly 
for users with older swtpm that already have the configuration setup.



The above only fails if swtm_setup is not installed, if that's what you 
mean. That's when swtpm_setup is NULL.


If you run `swptm_setup --create-config-files skip-if-exist` when any 
one of the 3 config files already exist, it will do nothing and exit with 0.





+
+    if (!virTPMSwtpmSetupCapsGet(
+ VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
+        return 0;



This is the case when swtpm_setup doesn't support --create-config-files. 
That's when the user has to use the old 
/usr/share/swtpm/swptm-create-user-config-files to create the config 
files, which will then be located at the same place that `swptm_setup 
--create-config-files skip-if-exist` would create them or check whether 
any one of them exist and skip.


3 config files will be created and if the user then removes 1 or 2 of 
them he created an invalid configuration and the start of the next VM 
will fail due to a bad configuration with missing config files. The 
config files would only be created again if all 3 were missing.




+
+    cmd = virCommandNew(swtpm_setup);
+    if (!cmd)
+        return -1;
+
+    virCommandAddArgList(cmd, "--create-config-files",
"skip-if-exist", NULL);
+    virCommandClearCaps(cmd);
+
+    if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Could not run '%s' to create config
files. exitstatus: %d",
+                          swtpm_setup, exitstatus);
+        return -1;

In the error case the stderr output of swtpm_setup should probably show 
up here?




+    }
+
+    return 0;
+}
+
+
 /*
  * qemuTPMEmulatorRunSetup
  *
@@ -432,6 +468,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
                                  "this requires privileged mode
for a "
                                  "TPM 1.2\n"), 0600);

+    if (!privileged && qemuTPMCreateConfigFiles())
+        return -1;
+
     cmd = virCommandNew(swtpm_setup);
     if (!cmd)
         return -1;
diff --git a/src/util/virtpm.c b/src/util/virtpm.c
index 1a567139b4..0f50de866c 100644
--- a/src/util/virtpm.c
+++ b/src/util/virtpm.c
@@ -45,6 +45,7 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature,
 VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
               VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
               "cmdarg-pwdfile-fd",
+              "cmdarg-create-config-files",
 );

 /**
diff --git a/src/util/virtpm.h b/src/util/virtpm.h
index d021a083b4..3bb03b3b33 100644
--- a/src/util/virtpm.h
+++ b/src/util/virtpm.h
@@ -38,6 +38,7 @@ typedef enum {

  

[libvirt PATCH 08/21] qemu: Introduce QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI

[Andrea Bolognani]

This capability detects the availability of the virtio-iommu-pci
device.

Note that, while this device is present even in somewhat old
versions of QEMU, it's only some recent changes that made it
actually usable for our purposes.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c  | 2 ++
 src/qemu/qemu_capabilities.h  | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 +
 tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml  | 1 +
 18 files changed, 19 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 82687dbf39..bfd02e866f 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -644,6 +644,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "virtio-mem-pci", /* QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI */
   "memory-backend-file.reserve", /* 
QEMU_CAPS_MEMORY_BACKEND_RESERVE */
   "piix4.acpi-root-pci-hotplug", /* 
QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG */
+  "virtio-iommu-pci", /* QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI */
 );
 
 
@@ -1359,6 +1360,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
 { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
 { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
+{ "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
 };
 
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 2bbfc15dc4..e2611a6193 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -624,6 +624,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI, /* -device virtio-mem-pci */
 QEMU_CAPS_MEMORY_BACKEND_RESERVE, /* -object memory-backend-*.reserve= */
 QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG, /* -M pc 
PIIX4_PM.acpi-root-pci-hotplug */
+QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI, /* -device virtio-iommu-pci */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
index 2c125044de..083ed23eb2 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
@@ -184,6 +184,7 @@
   
   
   
+  
   500
   0
   61700241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
index ce0f63c2b8..d0a1320400 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
@@ -192,6 +192,7 @@
   
   
   
+  
   500
   0
   42900241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
index 05c8e0e467..2ec45d95aa 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
@@ -177,6 +177,7 @@
   
   
   
+  
   500
   0
   0
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
index aae5fe018f..2c5f438d48 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
@@ -227,6 +227,7 @@
   
   
   
+  
   500
   0
   43100241
diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
index e9ae3c5abb..4480b80c54 100644
--- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
@@ -230,6 +230,7 @@
   
   
   
+  
   5001000
   0
   43100242
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml
index 59e1f746ca..6a341dc6d5 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml
@@ -188,6 +188,7 @@
   
   
   
+  
   5002000
   0
   61700243
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml
index 0ce8a5ba2f..b5af8cff63 100644
--- 

[libvirt PATCH 20/21] docs: Document virtio-iommu

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 docs/formatdomain.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index a02802a954..9cbc63f6c7 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -7948,8 +7948,9 @@ Example:
...
 
 ``model``
-   Supported values are ``intel`` (for Q35 guests) and, :since:`since 5.5.0` ,
-   ``smmuv3`` (for ARM virt guests).
+   Supported values are ``intel`` (for Q35 guests) ``smmuv3``
+   (:since:`since 5.5.0`, for ARM virt guests), and ``virtio``
+   (:since:`since 7.9.0`, for Q35 and ARM virt guests).
 
 ``driver``
The ``driver`` subelement can be used to configure additional options, some
-- 
2.31.1

[libvirt PATCH 11/21] tests: Add test cases for virtio-iommu

[Andrea Bolognani]

These represent valid uses of the device.

Signed-off-by: Andrea Bolognani 
---
 .../virtio-iommu-aarch64.aarch64-latest.args  | 34 +++
 .../qemuxml2argvdata/virtio-iommu-aarch64.xml | 20 +++
 .../virtio-iommu-x86_64.x86_64-latest.args| 30 
 .../qemuxml2argvdata/virtio-iommu-x86_64.xml  | 18 ++
 tests/qemuxml2argvtest.c  |  2 ++
 .../virtio-iommu-aarch64.aarch64-latest.xml   | 32 +
 .../virtio-iommu-x86_64.x86_64-latest.xml | 34 +++
 tests/qemuxml2xmltest.c   |  2 ++
 8 files changed, 172 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args
 create mode 100644 tests/qemuxml2argvdata/virtio-iommu-aarch64.xml
 create mode 100644 
tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/virtio-iommu-x86_64.xml
 create mode 100644 
tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml
 create mode 100644 
tests/qemuxml2xmloutdata/virtio-iommu-x86_64.x86_64-latest.xml

diff --git a/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args 
b/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args
new file mode 100644
index 00..0363b0370d
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-guest \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-guest/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-guest/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-guest/.config \
+/usr/bin/qemu-system-aarch64 \
+-name guest=guest,debug-threads=on \
+-S \
+-object 
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-guest/master-key.aes"}'
 \
+-blockdev 
'{"driver":"file","filename":"/usr/share/AAVMF/AAVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}'
 \
+-blockdev 
'{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}'
 \
+-machine 
virt,accel=tcg,usb=off,dump-guest-core=off,gic-version=2,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,memory-backend=mach-virt.ram
 \
+-cpu cortex-a15 \
+-m 1024 \
+-object 
'{"qom-type":"memory-backend-ram","id":"mach-virt.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev id=audio1,driver=none \
+-sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-iommu-aarch64.xml 
b/tests/qemuxml2argvdata/virtio-iommu-aarch64.xml
new file mode 100644
index 00..3e89cb2dac
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-aarch64.xml
@@ -0,0 +1,20 @@
+
+  guest
+  1ccfd97d-5eb4-478a-bbe6-88d254c16db7
+  1048576
+  1
+  
+hvm
+/usr/share/AAVMF/AAVMF_CODE.fd
+/var/lib/libvirt/qemu/nvram/guest_VARS.fd
+  
+  
+
+  
+  
+/usr/bin/qemu-system-aarch64
+
+
+
+  
+
diff --git a/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args 
b/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args
new file mode 100644
index 00..8a1413d33d
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args
@@ -0,0 +1,30 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object 
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
 \
+-machine q35,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-cpu qemu64 \
+-m 214 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev id=audio1,driver=none \
+-sandbox 

[libvirt PATCH 16/21] qemu: Assign PCI address to virtio-iommu

[Andrea Bolognani]

The device is configured to be an integrated endpoint, as is
necessary for it to function correctly.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain_address.c  | 6 +-
 .../virtio-iommu-aarch64.aarch64-latest.xml | 4 +++-
 .../virtio-iommu-x86_64.x86_64-latest.xml   | 4 +++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e23de3bb83..767e2c28f8 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1004,7 +1004,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
 case VIR_DOMAIN_DEVICE_IOMMU:
 switch ((virDomainIOMMUModel) dev->data.iommu->model) {
 case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
-return 0;
+return virtioFlags | VIR_PCI_CONNECT_INTEGRATED;
 
 case VIR_DOMAIN_IOMMU_MODEL_INTEL:
 case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
@@ -2386,6 +2386,10 @@ qemuDomainAssignDevicePCISlots(virDomainDef *def,
 
 switch ((virDomainIOMMUModel) iommu->model) {
 case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
+if (virDeviceInfoPCIAddressIsWanted(>info) &&
+qemuDomainPCIAddressReserveNextAddr(addrs, >info) < 0) {
+return -1;
+}
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_INTEL:
diff --git a/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml 
b/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml
index 336f99d539..c6560e9a91 100644
--- a/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml
+++ b/tests/qemuxml2xmloutdata/virtio-iommu-aarch64.aarch64-latest.xml
@@ -27,6 +27,8 @@
 
 
 
-
+
+  
+
   
 
diff --git a/tests/qemuxml2xmloutdata/virtio-iommu-x86_64.x86_64-latest.xml 
b/tests/qemuxml2xmloutdata/virtio-iommu-x86_64.x86_64-latest.xml
index 0b6c2d0eaf..ad3a702b0b 100644
--- a/tests/qemuxml2xmloutdata/virtio-iommu-x86_64.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/virtio-iommu-x86_64.x86_64-latest.xml
@@ -29,6 +29,8 @@
 
 
 
-
+
+  
+
   
 
-- 
2.31.1

[libvirt PATCH 18/21] tests: Add test for virtio-iommu address

[Andrea Bolognani]

virtio-iommu needs to be an integrated device, and our address
assignment code will make sure that is the case. If the user has
provided an explicit address, however, we should make sure any
addresses pointing to a different bus are rejected.

Signed-off-by: Andrea Bolognani 
---
 ...io-iommu-invalid-address.x86_64-latest.err |  1 +
 .../virtio-iommu-invalid-address.xml  | 20 +++
 tests/qemuxml2argvtest.c  |  1 +
 3 files changed, 22 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/virtio-iommu-invalid-address.x86_64-latest.err
 create mode 100644 tests/qemuxml2argvdata/virtio-iommu-invalid-address.xml

diff --git 
a/tests/qemuxml2argvdata/virtio-iommu-invalid-address.x86_64-latest.err 
b/tests/qemuxml2argvdata/virtio-iommu-invalid-address.x86_64-latest.err
new file mode 100644
index 00..997948e91f
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-invalid-address.x86_64-latest.err
@@ -0,0 +1 @@
+XML error: The device at PCI address :01:00.0 needs to be an integrated 
device (bus=0)
diff --git a/tests/qemuxml2argvdata/virtio-iommu-invalid-address.xml 
b/tests/qemuxml2argvdata/virtio-iommu-invalid-address.xml
new file mode 100644
index 00..0daa58e3e7
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-invalid-address.xml
@@ -0,0 +1,20 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  1
+  
+hvm
+  
+  
+
+  
+  
+/usr/bin/qemu-system-x86_64
+
+
+
+  
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 0d349567a5..8a5b986e1b 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3244,6 +3244,7 @@ mymain(void)
 DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-wrong-machine");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-no-acpi");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-invalid-address-type");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-invalid-address");
 
 DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS);
 DO_TEST_PARSE_ERROR("cpu-hotplug-granularity",
-- 
2.31.1

[libvirt PATCH 19/21] qemu: Generate command line for virtio-iommu

[Andrea Bolognani]

https://bugzilla.redhat.com/show_bug.cgi?id=1653327

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_command.c | 17 ++---
 .../virtio-iommu-aarch64.aarch64-latest.args|  1 +
 .../virtio-iommu-x86_64.x86_64-latest.args  |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ef5ebe6413..9941e2a10b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6204,10 +6204,11 @@ qemuBuildBootCommandLine(virCommand *cmd,
 
 static int
 qemuBuildIOMMUCommandLine(virCommand *cmd,
-  const virDomainDef *def)
+  const virDomainDef *def,
+  virQEMUCaps *qemuCaps)
 {
 g_auto(virBuffer) opts = VIR_BUFFER_INITIALIZER;
-const virDomainIOMMUDef *iommu = def->iommu;
+virDomainIOMMUDef *iommu = def->iommu;
 
 if (!iommu)
 return 0;
@@ -6239,6 +6240,16 @@ qemuBuildIOMMUCommandLine(virCommand *cmd,
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
+if (qemuBuildVirtioDevStr(, "virtio-iommu", qemuCaps,
+  VIR_DOMAIN_DEVICE_IOMMU, iommu) < 0) {
+return -1;
+}
+
+if (qemuBuildDeviceAddressStr(, def, >info) < 0)
+return -1;
+
+virCommandAddArg(cmd, "-device");
+virCommandAddArgBuffer(cmd, );
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
@@ -10493,7 +10504,7 @@ qemuBuildCommandLine(virQEMUDriver *driver,
 if (qemuBuildBootCommandLine(cmd, def) < 0)
 return NULL;
 
-if (qemuBuildIOMMUCommandLine(cmd, def) < 0)
+if (qemuBuildIOMMUCommandLine(cmd, def, qemuCaps) < 0)
 return NULL;
 
 if (qemuBuildGlobalControllerCommandLine(cmd, def) < 0)
diff --git a/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args 
b/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args
index 0363b0370d..3f33abd696 100644
--- a/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-iommu-aarch64.aarch64-latest.args
@@ -29,6 +29,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-guest/.config \
 -rtc base=utc \
 -no-shutdown \
 -boot strict=on \
+-device virtio-iommu-pci,bus=pcie.0,addr=0x1 \
 -audiodev id=audio1,driver=none \
 -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
 -msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args 
b/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args
index 8a1413d33d..7249d29b93 100644
--- a/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-latest.args
@@ -25,6 +25,7 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
 -rtc base=utc \
 -no-shutdown \
 -boot strict=on \
+-device virtio-iommu-pci,bus=pcie.0,addr=0x1 \
 -audiodev id=audio1,driver=none \
 -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
 -msg timestamp=on
-- 
2.31.1

[libvirt PATCH 17/21] qemu: Validate address type for virtio-iommu

[Andrea Bolognani]

virtio-iommu is a PCI device and attempts to use a different
address type should be rejected.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_validate.c  |  7 +++
 ...mmu-invalid-address-type.x86_64-latest.err |  1 +
 .../virtio-iommu-invalid-address-type.xml | 20 +++
 tests/qemuxml2argvtest.c  |  1 +
 4 files changed, 29 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.x86_64-latest.err
 create mode 100644 tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.xml

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 816320944a..7c18963a33 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4820,6 +4820,13 @@ qemuValidateDomainDeviceDefIOMMU(const virDomainIOMMUDef 
*iommu,
virDomainIOMMUModelTypeToString(iommu->model));
 return -1;
 }
+if (iommu->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+iommu->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("IOMMU device: '%s' needs a PCI address"),
+   virDomainIOMMUModelTypeToString(iommu->model));
+return -1;
+}
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
diff --git 
a/tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.x86_64-latest.err 
b/tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.x86_64-latest.err
new file mode 100644
index 00..216848eaf8
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: IOMMU device: 'virtio' needs a PCI address
diff --git a/tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.xml 
b/tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.xml
new file mode 100644
index 00..8c227c38a4
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-invalid-address-type.xml
@@ -0,0 +1,20 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  1
+  
+hvm
+  
+  
+
+  
+  
+/usr/bin/qemu-system-x86_64
+
+
+
+  
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index d14f017098..0d349567a5 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3243,6 +3243,7 @@ mymain(void)
 DO_TEST_CAPS_ARCH_LATEST("virtio-iommu-aarch64", "aarch64");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-wrong-machine");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-no-acpi");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-invalid-address-type");
 
 DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS);
 DO_TEST_PARSE_ERROR("cpu-hotplug-granularity",
-- 
2.31.1

[libvirt PATCH 21/21] news: Document virtio-iommu

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 NEWS.rst | 4 
 1 file changed, 4 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index caa61f0646..ab524a68fe 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -32,6 +32,10 @@ v7.9.0 (unreleased)
 controller. The default behavior is unchanged (hotplug is
 allowed).
 
+  * qemu: Introduce support for virtio-iommu
+
+This IOMMU device can be used with both Q35 and ARM virt guests.
+
 * **Improvements**
 
 * **Bug fixes**
-- 
2.31.1

[libvirt PATCH 15/21] conf: Add virDomainDeviceInfo to virDomainIOMMUDef

[Andrea Bolognani]

This is needed so that IOMMU devices can have addresses.

Existing IOMMU devices (intel-iommu and SMMUv3) are system
devices and as such don't have an address associated to them, but
virtio-iommu is a PCI device and needs one.

Signed-off-by: Andrea Bolognani 
---
 docs/schemas/domaincommon.rng | 63 +++
 src/conf/domain_conf.c| 37 +---
 src/conf/domain_conf.h|  1 +
 3 files changed, 60 insertions(+), 41 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 2f30b15c48..6dffe3afae 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5418,35 +5418,40 @@
   virtio
 
   
-  
-
-  
-
-  
-
-  
-  
-
-  
-
-  
-  
-
-  
-
-  
-  
-
-  
-
-  
-  
-
-  
-
-  
-
-  
+  
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+
+  
+
+  
+
+
+  
+
+  
+
+
+  
+
+  
+
+  
+
+
+  
+
+  
 
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a2d155069f..3b23c32a96 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2542,6 +2542,7 @@ virDomainIOMMUDefFree(virDomainIOMMUDef *iommu)
 if (!iommu)
 return;
 
+virDomainDeviceInfoClear(>info);
 g_free(iommu);
 }
 
@@ -4230,13 +4231,14 @@ virDomainDeviceGetInfo(virDomainDeviceDef *device)
 return >data.panic->info;
 case VIR_DOMAIN_DEVICE_MEMORY:
 return >data.memory->info;
+case VIR_DOMAIN_DEVICE_IOMMU:
+return >data.iommu->info;
 case VIR_DOMAIN_DEVICE_VSOCK:
 return >data.vsock->info;
 
 /* The following devices do not contain virDomainDeviceInfo */
 case VIR_DOMAIN_DEVICE_LEASE:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
-case VIR_DOMAIN_DEVICE_IOMMU:
 case VIR_DOMAIN_DEVICE_AUDIO:
 case VIR_DOMAIN_DEVICE_LAST:
 case VIR_DOMAIN_DEVICE_NONE:
@@ -4532,6 +4534,13 @@ virDomainDeviceInfoIterateFlags(virDomainDef *def,
 return rc;
 }
 
+device.type = VIR_DOMAIN_DEVICE_IOMMU;
+if (def->iommu) {
+device.data.iommu = def->iommu;
+if ((rc = cb(def, , >iommu->info, opaque)) != 0)
+return rc;
+}
+
 device.type = VIR_DOMAIN_DEVICE_VSOCK;
 if (def->vsock) {
 device.data.vsock = def->vsock;
@@ -4559,12 +4568,6 @@ virDomainDeviceInfoIterateFlags(virDomainDef *def,
 if ((rc = cb(def, , NULL, opaque)) != 0)
 return rc;
 }
-device.type = VIR_DOMAIN_DEVICE_IOMMU;
-if (def->iommu) {
-device.data.iommu = def->iommu;
-if ((rc = cb(def, , NULL, opaque)) != 0)
-return rc;
-}
 }
 
 /* Coverity is not very happy with this - all dead_error_condition */
@@ -14933,8 +14936,10 @@ virDomainMemoryDefParseXML(virDomainXMLOption *xmlopt,
 
 
 static virDomainIOMMUDef *
-virDomainIOMMUDefParseXML(xmlNodePtr node,
-  xmlXPathContextPtr ctxt)
+virDomainIOMMUDefParseXML(virDomainXMLOption *xmlopt,
+  xmlNodePtr node,
+  xmlXPathContextPtr ctxt,
+  unsigned int flags)
 {
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 xmlNodePtr driver;
@@ -14970,6 +14975,10 @@ virDomainIOMMUDefParseXML(xmlNodePtr node,
 return NULL;
 }
 
+if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt,
+>info, flags) < 0)
+return NULL;
+
 return g_steal_pointer();
 }
 
@@ -15167,7 +15176,8 @@ virDomainDeviceDefParse(const char *xmlStr,
 return NULL;
 break;
 case VIR_DOMAIN_DEVICE_IOMMU:
-if (!(dev->data.iommu = virDomainIOMMUDefParseXML(node, ctxt)))
+if (!(dev->data.iommu = virDomainIOMMUDefParseXML(xmlopt, node,
+  ctxt, flags)))
 return NULL;
 break;
 case VIR_DOMAIN_DEVICE_VSOCK:
@@ -20285,7 +20295,8 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
 }
 
 if (n > 0) {
-if (!(def->iommu = virDomainIOMMUDefParseXML(nodes[0], ctxt)))
+if (!(def->iommu = virDomainIOMMUDefParseXML(xmlopt, nodes[0],
+ ctxt, flags)))
 return NULL;
 }
 VIR_FREE(nodes);
@@ -22107,7 +22118,7 @@ 

[libvirt PATCH 14/21] qemu: Validate use of ACPI with virtio-iommu

[Andrea Bolognani]

virtio-iommu doesn't work without ACPI, so we need to make sure
the latter is enabled.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_validate.c  |  6 ++
 .../virtio-iommu-no-acpi.x86_64-latest.err|  1 +
 tests/qemuxml2argvdata/virtio-iommu-no-acpi.xml   | 15 +++
 tests/qemuxml2argvtest.c  |  1 +
 4 files changed, 23 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/virtio-iommu-no-acpi.x86_64-latest.err
 create mode 100644 tests/qemuxml2argvdata/virtio-iommu-no-acpi.xml

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index fd08a84fd6..816320944a 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4814,6 +4814,12 @@ qemuValidateDomainDeviceDefIOMMU(const virDomainIOMMUDef 
*iommu,
virDomainIOMMUModelTypeToString(iommu->model));
 return -1;
 }
+if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("IOMMU device: '%s' requires ACPI"),
+   virDomainIOMMUModelTypeToString(iommu->model));
+return -1;
+}
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
diff --git a/tests/qemuxml2argvdata/virtio-iommu-no-acpi.x86_64-latest.err 
b/tests/qemuxml2argvdata/virtio-iommu-no-acpi.x86_64-latest.err
new file mode 100644
index 00..6b598951bb
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-no-acpi.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: IOMMU device: 'virtio' requires ACPI
diff --git a/tests/qemuxml2argvdata/virtio-iommu-no-acpi.xml 
b/tests/qemuxml2argvdata/virtio-iommu-no-acpi.xml
new file mode 100644
index 00..36e5eb39b9
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-no-acpi.xml
@@ -0,0 +1,15 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  1
+  
+hvm
+  
+  
+/usr/bin/qemu-system-x86_64
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 75ea4d32f3..d14f017098 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3242,6 +3242,7 @@ mymain(void)
 DO_TEST_CAPS_VER_PARSE_ERROR("virtio-iommu-x86_64", "6.1.0");
 DO_TEST_CAPS_ARCH_LATEST("virtio-iommu-aarch64", "aarch64");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-wrong-machine");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-no-acpi");
 
 DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS);
 DO_TEST_PARSE_ERROR("cpu-hotplug-granularity",
-- 
2.31.1

[libvirt PATCH 13/21] qemu: Validate capabilities for virtio-iommu

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_validate.c  | 8 
 .../qemuxml2argvdata/virtio-iommu-x86_64.x86_64-6.1.0.err | 1 +
 tests/qemuxml2argvtest.c  | 1 +
 3 files changed, 10 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-6.1.0.err

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 073f7bce58..fd08a84fd6 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4806,6 +4806,14 @@ qemuValidateDomainDeviceDefIOMMU(const virDomainIOMMUDef 
*iommu,
virDomainIOMMUModelTypeToString(iommu->model));
 return -1;
 }
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI) ||
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("IOMMU device: '%s' is not supported with "
+ "this QEMU binary"),
+   virDomainIOMMUModelTypeToString(iommu->model));
+return -1;
+}
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
diff --git a/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-6.1.0.err 
b/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-6.1.0.err
new file mode 100644
index 00..e76e1540bc
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-x86_64.x86_64-6.1.0.err
@@ -0,0 +1 @@
+unsupported configuration: IOMMU device: 'virtio' is not supported with this 
QEMU binary
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 72ffac5de3..75ea4d32f3 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3239,6 +3239,7 @@ mymain(void)
 DO_TEST_CAPS_LATEST_PARSE_ERROR("intel-iommu-wrong-machine");
 DO_TEST_CAPS_ARCH_LATEST("iommu-smmuv3", "aarch64");
 DO_TEST_CAPS_LATEST("virtio-iommu-x86_64");
+DO_TEST_CAPS_VER_PARSE_ERROR("virtio-iommu-x86_64", "6.1.0");
 DO_TEST_CAPS_ARCH_LATEST("virtio-iommu-aarch64", "aarch64");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-wrong-machine");
 
-- 
2.31.1

[libvirt PATCH 12/21] qemu: Validate machine type used with virtio-iommu

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_validate.c   |  8 
 ...irtio-iommu-wrong-machine.x86_64-latest.err |  1 +
 .../virtio-iommu-wrong-machine.xml | 18 ++
 tests/qemuxml2argvtest.c   |  1 +
 4 files changed, 28 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/virtio-iommu-wrong-machine.x86_64-latest.err
 create mode 100644 tests/qemuxml2argvdata/virtio-iommu-wrong-machine.xml

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 4646cd2af3..073f7bce58 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4798,6 +4798,14 @@ qemuValidateDomainDeviceDefIOMMU(const virDomainIOMMUDef 
*iommu,
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
+if (!qemuDomainIsARMVirt(def) &&
+!qemuDomainIsQ35(def)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("IOMMU device: '%s' is only supported with "
+ "Q35 and ARM Virt machines"),
+   virDomainIOMMUModelTypeToString(iommu->model));
+return -1;
+}
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
diff --git 
a/tests/qemuxml2argvdata/virtio-iommu-wrong-machine.x86_64-latest.err 
b/tests/qemuxml2argvdata/virtio-iommu-wrong-machine.x86_64-latest.err
new file mode 100644
index 00..8d1cd19170
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-wrong-machine.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: IOMMU device: 'virtio' is only supported with Q35 
and ARM Virt machines
diff --git a/tests/qemuxml2argvdata/virtio-iommu-wrong-machine.xml 
b/tests/qemuxml2argvdata/virtio-iommu-wrong-machine.xml
new file mode 100644
index 00..ad2a516b3a
--- /dev/null
+++ b/tests/qemuxml2argvdata/virtio-iommu-wrong-machine.xml
@@ -0,0 +1,18 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  1
+  
+hvm
+  
+  
+
+  
+  
+/usr/bin/qemu-system-x86_64
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 5f07c09d57..72ffac5de3 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3240,6 +3240,7 @@ mymain(void)
 DO_TEST_CAPS_ARCH_LATEST("iommu-smmuv3", "aarch64");
 DO_TEST_CAPS_LATEST("virtio-iommu-x86_64");
 DO_TEST_CAPS_ARCH_LATEST("virtio-iommu-aarch64", "aarch64");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("virtio-iommu-wrong-machine");
 
 DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS);
 DO_TEST_PARSE_ERROR("cpu-hotplug-granularity",
-- 
2.31.1

[libvirt PATCH 03/21] conf: Make virDomainDeviceInfoFormat() const correct

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_addr.c | 4 ++--
 src/conf/domain_addr.h | 4 ++--
 src/conf/domain_conf.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 53b39923e8..fe6520cf3a 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1829,7 +1829,7 @@ virDomainVirtioSerialAddrIsComplete(virDomainDeviceInfo 
*info)
 
 
 bool
-virDomainUSBAddressPortIsValid(unsigned int *port)
+virDomainUSBAddressPortIsValid(const unsigned int *port)
 {
 return port[0] != 0;
 }
@@ -1837,7 +1837,7 @@ virDomainUSBAddressPortIsValid(unsigned int *port)
 
 void
 virDomainUSBAddressPortFormatBuf(virBuffer *buf,
- unsigned int *port)
+ const unsigned int *port)
 {
 size_t i;
 
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 3f8fcf8acb..814b556024 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -246,12 +246,12 @@ virDomainVirtioSerialAddrAutoAssign(virDomainDef *def,
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 bool
-virDomainUSBAddressPortIsValid(unsigned int *port)
+virDomainUSBAddressPortIsValid(const unsigned int *port)
 ATTRIBUTE_NONNULL(1);
 
 void
 virDomainUSBAddressPortFormatBuf(virBuffer *buf,
- unsigned int *port)
+ const unsigned int *port)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 #define VIR_DOMAIN_USB_HUB_PORTS 8
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b8370f6950..df51d59e0d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6319,7 +6319,7 @@ virDomainVirtioOptionsFormat(virBuffer *buf,
 
 static void ATTRIBUTE_NONNULL(2)
 virDomainDeviceInfoFormat(virBuffer *buf,
-  virDomainDeviceInfo *info,
+  const virDomainDeviceInfo *info,
   unsigned int flags)
 {
 g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
-- 
2.31.1

[libvirt PATCH 10/21] conf: Introduce virtio model for

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 docs/schemas/domaincommon.rng  | 1 +
 src/conf/domain_conf.c | 1 +
 src/conf/domain_conf.h | 1 +
 src/qemu/qemu_command.c| 4 
 src/qemu/qemu_domain_address.c | 6 ++
 src/qemu/qemu_validate.c   | 3 +++
 6 files changed, 16 insertions(+)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index ec5bd91740..2f30b15c48 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5415,6 +5415,7 @@
 
   intel
   smmuv3
+  virtio
 
   
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6df981ad47..a2d155069f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1261,6 +1261,7 @@ VIR_ENUM_IMPL(virDomainIOMMUModel,
   VIR_DOMAIN_IOMMU_MODEL_LAST,
   "intel",
   "smmuv3",
+  "virtio",
 );
 
 VIR_ENUM_IMPL(virDomainVsockModel,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 72db861105..21e069f42e 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2696,6 +2696,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSecDef, 
virDomainSecDefFree);
 typedef enum {
 VIR_DOMAIN_IOMMU_MODEL_INTEL,
 VIR_DOMAIN_IOMMU_MODEL_SMMUV3,
+VIR_DOMAIN_IOMMU_MODEL_VIRTIO,
 
 VIR_DOMAIN_IOMMU_MODEL_LAST
 } virDomainIOMMUModel;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb743e3d84..ef5ebe6413 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6238,6 +6238,9 @@ qemuBuildIOMMUCommandLine(virCommand *cmd,
 virCommandAddArgBuffer(cmd, );
 break;
 
+case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
+break;
+
 case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
 /* There is no -device for SMMUv3, so nothing to be done here */
 return 0;
@@ -6851,6 +6854,7 @@ qemuBuildMachineCommandLine(virCommand *cmd,
 break;
 
 case VIR_DOMAIN_IOMMU_MODEL_INTEL:
+case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
 /* These IOMMUs are formatted in qemuBuildIOMMUCommandLine */
 break;
 
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 733fa35444..e23de3bb83 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1003,6 +1003,9 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
 
 case VIR_DOMAIN_DEVICE_IOMMU:
 switch ((virDomainIOMMUModel) dev->data.iommu->model) {
+case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
+return 0;
+
 case VIR_DOMAIN_IOMMU_MODEL_INTEL:
 case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
@@ -2382,6 +2385,9 @@ qemuDomainAssignDevicePCISlots(virDomainDef *def,
 virDomainIOMMUDef *iommu = def->iommu;
 
 switch ((virDomainIOMMUModel) iommu->model) {
+case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
+break;
+
 case VIR_DOMAIN_IOMMU_MODEL_INTEL:
 case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index c84508cb64..4646cd2af3 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4797,6 +4797,9 @@ qemuValidateDomainDeviceDefIOMMU(const virDomainIOMMUDef 
*iommu,
 }
 break;
 
+case VIR_DOMAIN_IOMMU_MODEL_VIRTIO:
+break;
+
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
 default:
 virReportEnumRangeError(virDomainIOMMUModel, iommu->model);
-- 
2.31.1

[libvirt PATCH 09/21] qemu: Introduce QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS

[Andrea Bolognani]

This capability detects the availability of the boot-bypass
property of the virtio-iommu-pci device.

This property was only introduced in QEMU 6.2 but, since the
device has been around for much longer, we end up querying its
properties for several more releases. As I don't have convenient
access to the 10+ binaries necessary to regenerate the replies,
I just put some fake data in there.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c  |   8 +
 src/qemu/qemu_capabilities.h  |   1 +
 .../caps_5.0.0.aarch64.replies|  71 +++--
 .../caps_5.0.0.ppc64.replies  |  59 ++--
 .../caps_5.0.0.riscv64.replies|  55 ++--
 .../caps_5.0.0.x86_64.replies |  71 +++--
 .../caps_5.1.0.x86_64.replies |  71 +++--
 .../caps_5.2.0.aarch64.replies|  71 +++--
 .../caps_5.2.0.ppc64.replies  |  59 ++--
 .../caps_5.2.0.riscv64.replies|  55 ++--
 .../caps_5.2.0.s390x.replies  |  59 ++--
 .../caps_5.2.0.x86_64.replies |  71 +++--
 .../caps_6.0.0.aarch64.replies|  71 +++--
 .../caps_6.0.0.s390x.replies  |  59 ++--
 .../caps_6.0.0.x86_64.replies |  71 +++--
 .../caps_6.1.0.x86_64.replies |  71 +++--
 .../caps_6.2.0.aarch64.replies| 275 --
 .../caps_6.2.0.aarch64.xml|   1 +
 .../caps_6.2.0.x86_64.replies | 275 --
 .../caps_6.2.0.x86_64.xml |   1 +
 20 files changed, 1099 insertions(+), 376 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index bfd02e866f..4b46707fc0 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -645,6 +645,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "memory-backend-file.reserve", /* 
QEMU_CAPS_MEMORY_BACKEND_RESERVE */
   "piix4.acpi-root-pci-hotplug", /* 
QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG */
   "virtio-iommu-pci", /* QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI */
+  "virtio-iommu.boot-bypass", /* 
QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS */
 );
 
 
@@ -1556,6 +1557,10 @@ static struct virQEMUCapsDevicePropsFlags 
virQEMUCapsDevicePropsVhostUserFS[] =
 { "bootindex", QEMU_CAPS_VHOST_USER_FS_BOOTINDEX, NULL },
 };
 
+static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioIOMMU[] 
= {
+{ "boot-bypass", QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS, NULL },
+};
+
 /* see documentation for virQEMUQAPISchemaPathGet for the query format */
 static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
 { "block-commit/arg-type/*top",  QEMU_CAPS_ACTIVE_COMMIT },
@@ -1714,6 +1719,9 @@ static virQEMUCapsDeviceTypeProps 
virQEMUCapsDeviceProps[] = {
 { "vhost-user-fs-device", virQEMUCapsDevicePropsVhostUserFS,
   G_N_ELEMENTS(virQEMUCapsDevicePropsVhostUserFS),
   QEMU_CAPS_DEVICE_VHOST_USER_FS },
+{ "virtio-iommu-pci", virQEMUCapsDevicePropsVirtioIOMMU,
+  G_N_ELEMENTS(virQEMUCapsDevicePropsVirtioIOMMU),
+  QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsMemoryBackendFile[] 
= {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index e2611a6193..fc2e819688 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -625,6 +625,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_MEMORY_BACKEND_RESERVE, /* -object memory-backend-*.reserve= */
 QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG, /* -M pc 
PIIX4_PM.acpi-root-pci-hotplug */
 QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI, /* -device virtio-iommu-pci */
+QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS, /* virtio-iommu.boot-bypass */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies 
b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies
index 574c14d4ce..29bde0357f 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.replies
@@ -20244,12 +20244,31 @@
   "id": "libvirt-32"
 }
 
+{
+  "execute": "device-list-properties",
+  "arguments": {
+"typename": "virtio-iommu-pci"
+  },
+  "id": "libvirt-33"
+}
+
+{
+  "return": [
+{
+  "name": "fake-data",
+  "description": "pretend there's real data here",
+  "type": "str"
+}
+  ],
+  "id": "libvirt-33"
+}
[...]
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies 
b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies
index 59a0c4ff55..a46b27e11c 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies
@@ -24190,12 +24190,235 @@
   "id": "libvirt-34"
 }
 
+{
+  "execute": "device-list-properties",
+  "arguments": {
+

[libvirt PATCH 00/21] qemu: Implement virtio-iommu support

[Andrea Bolognani]

The first couple of patches add replies files and as such have been
aggressively snipped to deal with mailing list message size limits.
Grab the unabriged version with

  $ git fetch https://gitlab.com/abologna/libvirt.git virtio-iommu

As noted in those patches, some of the QEMU changes this series
depends on have not yet been accepted upstream: the relevant patches
are

  https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00161.html
  https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07819.html

and of course this series should only be merged once those have gone
in.

Andrea Bolognani (21):
  DONOTMERGEYET: Add replies for QEMU 6.2.0 on x86_64
  DONOTMERGEYET: Add replies for QEMU 6.2.0 on aarch64
  conf: Make virDomainDeviceInfoFormat() const correct
  conf: Introduce VIR_PCI_CONNECT_INTEGRATED
  conf: Add IOMMU support to virDomainDeviceDefCopy()
  conf: Add new/free functions for virDomainIOMMUDef
  qemu: Tweak some code
  qemu: Introduce QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI
  qemu: Introduce QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS
  conf: Introduce virtio model for 
  tests: Add test cases for virtio-iommu
  qemu: Validate machine type used with virtio-iommu
  qemu: Validate capabilities for virtio-iommu
  qemu: Validate use of ACPI with virtio-iommu
  conf: Add virDomainDeviceInfo to virDomainIOMMUDef
  qemu: Assign PCI address to virtio-iommu
  qemu: Validate address type for virtio-iommu
  tests: Add test for virtio-iommu address
  qemu: Generate command line for virtio-iommu
  docs: Document virtio-iommu
  news: Document virtio-iommu

 NEWS.rst  |4 +
 docs/formatdomain.rst |5 +-
 docs/schemas/domaincommon.rng |   64 +-
 src/conf/domain_addr.c|   21 +-
 src/conf/domain_addr.h|   30 +-
 src/conf/domain_conf.c|   74 +-
 src/conf/domain_conf.h|5 +
 src/qemu/qemu_capabilities.c  |   10 +
 src/qemu/qemu_capabilities.h  |2 +
 src/qemu/qemu_command.c   |   35 +-
 src/qemu/qemu_domain_address.c|   33 +-
 src/qemu/qemu_validate.c  |   32 +
 .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml  |  220 +
 .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml  |  226 +
 .../qemu_6.2.0-virt.aarch64.xml   |  184 +
 tests/domaincapsdata/qemu_6.2.0.aarch64.xml   |  178 +
 tests/domaincapsdata/qemu_6.2.0.x86_64.xml|  220 +
 .../caps_5.0.0.aarch64.replies|   71 +-
 .../caps_5.0.0.aarch64.xml|1 +
 .../caps_5.0.0.ppc64.replies  |   59 +-
 .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |1 +
 .../caps_5.0.0.riscv64.replies|   55 +-
 .../caps_5.0.0.riscv64.xml|1 +
 .../caps_5.0.0.x86_64.replies |   71 +-
 .../caps_5.0.0.x86_64.xml |1 +
 .../caps_5.1.0.x86_64.replies |   71 +-
 .../caps_5.1.0.x86_64.xml |1 +
 .../caps_5.2.0.aarch64.replies|   71 +-
 .../caps_5.2.0.aarch64.xml|1 +
 .../caps_5.2.0.ppc64.replies  |   59 +-
 .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml |1 +
 .../caps_5.2.0.riscv64.replies|   55 +-
 .../caps_5.2.0.riscv64.xml|1 +
 .../caps_5.2.0.s390x.replies  |   59 +-
 .../qemucapabilitiesdata/caps_5.2.0.s390x.xml |1 +
 .../caps_5.2.0.x86_64.replies |   71 +-
 .../caps_5.2.0.x86_64.xml |1 +
 .../caps_6.0.0.aarch64.replies|   71 +-
 .../caps_6.0.0.aarch64.xml|1 +
 .../caps_6.0.0.s390x.replies  |   59 +-
 .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |1 +
 .../caps_6.0.0.x86_64.replies |   71 +-
 .../caps_6.0.0.x86_64.xml |1 +
 .../caps_6.1.0.x86_64.replies |   71 +-
 .../caps_6.1.0.x86_64.xml |1 +
 ...h64.replies => caps_6.2.0.aarch64.replies} | 5621 ++---
 ...0.0.aarch64.xml => caps_6.2.0.aarch64.xml} |   57 +-
 ...6_64.replies => caps_6.2.0.x86_64.replies} | 4154 +++-
 ...6.1.0.x86_64.xml => caps_6.2.0.x86_64.xml} |   85 +-
 .../virtio-iommu-aarch64.aarch64-latest.args  |   35 +
 .../qemuxml2argvdata/virtio-iommu-aarch64.xml |   20 +
 ...mmu-invalid-address-type.x86_64-latest.err |1 +
 .../virtio-iommu-invalid-address-type.xml |   20 +
 ...io-iommu-invalid-address.x86_64-latest.err |1 +
 .../virtio-iommu-invalid-address.xml  |   20 +
 .../virtio-iommu-no-acpi.x86_64-latest.err|1 +
 .../qemuxml2argvdata/virtio-iommu-no-acpi.xml |   15 +
 ...rtio-iommu-wrong-machine.x86_64-latest.err |1 +
 .../virtio-iommu-wrong-machine.xml|   18 +
 .../virtio-iommu-x86_64.x86_64-6.1.0.err  |1 +
 

[libvirt PATCH 05/21] conf: Add IOMMU support to virDomainDeviceDefCopy()

[Andrea Bolognani]

There doesn't seem to be a reason for IOMMUs not to be handled
by this function.

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_conf.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index df51d59e0d..60f2b1129f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29588,6 +29588,10 @@ virDomainDeviceDefCopy(virDomainDeviceDef *src,
 virDomainShmemDefFormat(, src->data.shmem, flags);
 rc = 0;
 break;
+case VIR_DOMAIN_DEVICE_IOMMU:
+virDomainIOMMUDefFormat(, src->data.iommu);
+rc = 0;
+break;
 case VIR_DOMAIN_DEVICE_VSOCK:
 virDomainVsockDefFormat(, src->data.vsock);
 rc = 0;
@@ -29600,7 +29604,6 @@ virDomainDeviceDefCopy(virDomainDeviceDef *src,
 case VIR_DOMAIN_DEVICE_SMARTCARD:
 case VIR_DOMAIN_DEVICE_MEMBALLOON:
 case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_IOMMU:
 case VIR_DOMAIN_DEVICE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Copying definition of '%d' type "
-- 
2.31.1

[libvirt PATCH 02/21] DONOTMERGEYET: Add replies for QEMU 6.2.0 on aarch64

[Andrea Bolognani]

These were generated using a QEMU binary built from commit

  v6.1.0-1220-g5564f06816

with

  https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00161.html
  https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07819.html

applied on top.

Signed-off-by: Andrea Bolognani 
---
 .../qemu_6.2.0-virt.aarch64.xml   |   184 +
 tests/domaincapsdata/qemu_6.2.0.aarch64.xml   |   178 +
 .../caps_6.2.0.aarch64.replies| 28671 
 .../caps_6.2.0.aarch64.xml|   541 +
 4 files changed, 29574 insertions(+)
 create mode 100644 tests/domaincapsdata/qemu_6.2.0-virt.aarch64.xml
 create mode 100644 tests/domaincapsdata/qemu_6.2.0.aarch64.xml
 create mode 100644 tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies
 create mode 100644 tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml

diff --git a/tests/domaincapsdata/qemu_6.2.0-virt.aarch64.xml 
b/tests/domaincapsdata/qemu_6.2.0-virt.aarch64.xml
new file mode 100644
index 00..4eb8209b07
--- /dev/null
+++ b/tests/domaincapsdata/qemu_6.2.0-virt.aarch64.xml
@@ -0,0 +1,184 @@
+
+  /usr/bin/qemu-system-aarch64
+  kvm
+  virt-6.2
+  aarch64
[...]
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies 
b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies
new file mode 100644
index 00..59a0c4ff55
--- /dev/null
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.aarch64.replies
@@ -0,0 +1,28671 @@
+{
+  "execute": "qmp_capabilities",
+  "id": "libvirt-1"
+}
+
+{
+  "return": {
+  },
+  "id": "libvirt-1"
+}
+
+{
+  "execute": "query-version",
+  "id": "libvirt-2"
+}
+
+{
+  "return": {
+"qemu": {
+  "micro": 50,
+  "minor": 1,
+  "major": 6
+},
+"package": ""
+  },
+  "id": "libvirt-2"
+}
[...]
-- 
2.31.1

[libvirt PATCH 07/21] qemu: Tweak some code

[Andrea Bolognani]

The altered code is functionally equivalent to the previous one,
but it's already laid down in a way that will make further
changes easier and less messy.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_command.c| 14 ++
 src/qemu/qemu_domain_address.c | 23 ++-
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 28bca1519c..bb743e3d84 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6206,15 +6206,14 @@ static int
 qemuBuildIOMMUCommandLine(virCommand *cmd,
   const virDomainDef *def)
 {
+g_auto(virBuffer) opts = VIR_BUFFER_INITIALIZER;
 const virDomainIOMMUDef *iommu = def->iommu;
 
 if (!iommu)
 return 0;
 
 switch (iommu->model) {
-case VIR_DOMAIN_IOMMU_MODEL_INTEL: {
-g_auto(virBuffer) opts = VIR_BUFFER_INITIALIZER;
-
+case VIR_DOMAIN_IOMMU_MODEL_INTEL:
 virBufferAddLit(, "intel-iommu");
 if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT) {
 virBufferAsprintf(, ",intremap=%s",
@@ -6238,7 +6237,6 @@ qemuBuildIOMMUCommandLine(virCommand *cmd,
 virCommandAddArg(cmd, "-device");
 virCommandAddArgBuffer(cmd, );
 break;
-}
 
 case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
 /* There is no -device for SMMUv3, so nothing to be done here */
@@ -6848,14 +6846,14 @@ qemuBuildMachineCommandLine(virCommand *cmd,
 
 if (def->iommu) {
 switch (def->iommu->model) {
-case VIR_DOMAIN_IOMMU_MODEL_INTEL:
-/* The 'intel' IOMMu is formatted in qemuBuildIOMMUCommandLine */
-break;
-
 case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
 virBufferAddLit(, ",iommu=smmuv3");
 break;
 
+case VIR_DOMAIN_IOMMU_MODEL_INTEL:
+/* These IOMMUs are formatted in qemuBuildIOMMUCommandLine */
+break;
+
 case VIR_DOMAIN_IOMMU_MODEL_LAST:
 default:
 virReportEnumRangeError(virDomainIOMMUModel, def->iommu->model);
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index c43ad23cf5..733fa35444 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1001,6 +1001,16 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
 }
 break;
 
+case VIR_DOMAIN_DEVICE_IOMMU:
+switch ((virDomainIOMMUModel) dev->data.iommu->model) {
+case VIR_DOMAIN_IOMMU_MODEL_INTEL:
+case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
+case VIR_DOMAIN_IOMMU_MODEL_LAST:
+/* These are not PCI devices */
+return 0;
+}
+break;
+
 case VIR_DOMAIN_DEVICE_VSOCK:
 switch ((virDomainVsockModel) dev->data.vsock->model) {
 case VIR_DOMAIN_VSOCK_MODEL_VIRTIO_TRANSITIONAL:
@@ -1040,7 +1050,6 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
 /* These devices don't even have a DeviceInfo */
 case VIR_DOMAIN_DEVICE_LEASE:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
-case VIR_DOMAIN_DEVICE_IOMMU:
 case VIR_DOMAIN_DEVICE_AUDIO:
 case VIR_DOMAIN_DEVICE_LAST:
 case VIR_DOMAIN_DEVICE_NONE:
@@ -2369,6 +2378,18 @@ qemuDomainAssignDevicePCISlots(virDomainDef *def,
 /* Nada - none are PCI based (yet) */
 }
 
+if (def->iommu) {
+virDomainIOMMUDef *iommu = def->iommu;
+
+switch ((virDomainIOMMUModel) iommu->model) {
+case VIR_DOMAIN_IOMMU_MODEL_INTEL:
+case VIR_DOMAIN_IOMMU_MODEL_SMMUV3:
+case VIR_DOMAIN_IOMMU_MODEL_LAST:
+/* These are not PCI devices */
+break;
+}
+}
+
 if (def->vsock &&
 virDeviceInfoPCIAddressIsWanted(>vsock->info)) {
 
-- 
2.31.1

[libvirt PATCH 04/21] conf: Introduce VIR_PCI_CONNECT_INTEGRATED

[Andrea Bolognani]

This new flag can be used to convince the PCI address assignment
algorithm to place a device directly on the root bus. It will be
used to implement support for virtio-iommu, which needs to be an
integrated device in order to work correctly.

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_addr.c | 17 +
 src/conf/domain_addr.h | 26 +++---
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index fe6520cf3a..035d60460f 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -303,6 +303,23 @@ virDomainPCIAddressFlagsCompatible(virPCIDeviceAddress 
*addr,
 virErrorNumber errType = (fromConfig
   ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
 
+if (devFlags & VIR_PCI_CONNECT_INTEGRATED) {
+if (addr->bus == 0) {
+/* pcie-root doesn't usually allow endpoint devices to be
+ * plugged directly into it, but for integrated devices
+ * that's exactly what we want */
+busFlags |= VIR_PCI_CONNECT_AUTOASSIGN;
+} else {
+if (reportError) {
+virReportError(errType,
+   _("The device at PCI address %s needs to be "
+ "an integrated device (bus=0)"),
+   addrStr);
+}
+return false;
+}
+}
+
 if (fromConfig) {
 /* If the requested connection was manually specified in
  * config, allow a PCI device to connect to a PCIe slot, or
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 814b556024..1772ea7088 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -35,24 +35,28 @@ typedef enum {
 VIR_PCI_CONNECT_AUTOASSIGN = 1 << 0, /* okay to autoassign a device to 
this controller */
 VIR_PCI_CONNECT_HOTPLUGGABLE = 1 << 1, /* is hotplug needed/supported */
 
+/* Set for devices that can only work as integrated devices (directly
+ * connected to pci.0 or pcie.0, with no additional buses in between) */
+VIR_PCI_CONNECT_INTEGRATED = 1 << 2,
+
 /* set for devices that can share a single slot in auto-assignment
  * (by assigning one device to each of the 8 functions on the slot)
  */
-VIR_PCI_CONNECT_AGGREGATE_SLOT = 1 << 2,
+VIR_PCI_CONNECT_AGGREGATE_SLOT = 1 << 3,
 
 /* kinds of devices as a bitmap so they can be combined (some PCI
  * controllers permit connecting multiple types of devices)
  */
-VIR_PCI_CONNECT_TYPE_PCI_DEVICE = 1 << 3,
-VIR_PCI_CONNECT_TYPE_PCIE_DEVICE = 1 << 4,
-VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT = 1 << 5,
-VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT = 1 << 6,
-VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT = 1 << 7,
-VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE = 1 << 8,
-VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS = 1 << 9,
-VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS = 1 << 10,
-VIR_PCI_CONNECT_TYPE_PCI_BRIDGE = 1 << 11,
-VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE = 1 << 12,
+VIR_PCI_CONNECT_TYPE_PCI_DEVICE = 1 << 4,
+VIR_PCI_CONNECT_TYPE_PCIE_DEVICE = 1 << 5,
+VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT = 1 << 6,
+VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT = 1 << 7,
+VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT = 1 << 8,
+VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE = 1 << 9,
+VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS = 1 << 10,
+VIR_PCI_CONNECT_TYPE_PCIE_EXPANDER_BUS = 1 << 11,
+VIR_PCI_CONNECT_TYPE_PCI_BRIDGE = 1 << 12,
+VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE = 1 << 13,
 } virDomainPCIConnectFlags;
 
 /* a combination of all bits that describe the type of connections
-- 
2.31.1

[libvirt PATCH 06/21] conf: Add new/free functions for virDomainIOMMUDef

[Andrea Bolognani]

This will make it possible to limit changes to a single spot
later on, and is also just an overall nicer way to create and
destroy objects.

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_conf.c | 29 +
 src/conf/domain_conf.h |  3 +++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 60f2b1129f..6df981ad47 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2524,6 +2524,27 @@ virDomainVsockDefFree(virDomainVsockDef *vsock)
 }
 
 
+virDomainIOMMUDef *
+virDomainIOMMUDefNew(void)
+{
+g_autoptr(virDomainIOMMUDef) iommu = NULL;
+
+iommu = g_new0(virDomainIOMMUDef, 1);
+
+return g_steal_pointer();
+}
+
+
+void
+virDomainIOMMUDefFree(virDomainIOMMUDef *iommu)
+{
+if (!iommu)
+return;
+
+g_free(iommu);
+}
+
+
 void
 virDomainNetTeamingInfoFree(virDomainNetTeamingInfo *teaming)
 {
@@ -3325,7 +3346,7 @@ void virDomainDeviceDefFree(virDomainDeviceDef *def)
 virDomainMemoryDefFree(def->data.memory);
 break;
 case VIR_DOMAIN_DEVICE_IOMMU:
-g_free(def->data.iommu);
+virDomainIOMMUDefFree(def->data.iommu);
 break;
 case VIR_DOMAIN_DEVICE_VSOCK:
 virDomainVsockDefFree(def->data.vsock);
@@ -3663,7 +3684,7 @@ void virDomainDefFree(virDomainDef *def)
 virDomainPanicDefFree(def->panics[i]);
 g_free(def->panics);
 
-g_free(def->iommu);
+virDomainIOMMUDefFree(def->iommu);
 
 g_free(def->idmap.uidmap);
 g_free(def->idmap.gidmap);
@@ -14916,11 +14937,11 @@ virDomainIOMMUDefParseXML(xmlNodePtr node,
 {
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 xmlNodePtr driver;
-g_autofree virDomainIOMMUDef *iommu = NULL;
+g_autoptr(virDomainIOMMUDef) iommu = NULL;
 
 ctxt->node = node;
 
-iommu = g_new0(virDomainIOMMUDef, 1);
+iommu = virDomainIOMMUDefNew();
 
 if (virXMLPropEnum(node, "model", virDomainIOMMUModelTypeFromString,
VIR_XML_PROP_REQUIRED, >model) < 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c23c233184..72db861105 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3291,6 +3291,9 @@ bool virDomainControllerIsPSeriesPHB(const 
virDomainControllerDef *cont);
 virDomainFSDef *virDomainFSDefNew(virDomainXMLOption *xmlopt);
 void virDomainFSDefFree(virDomainFSDef *def);
 void virDomainActualNetDefFree(virDomainActualNetDef *def);
+virDomainIOMMUDef *virDomainIOMMUDefNew(void);
+void virDomainIOMMUDefFree(virDomainIOMMUDef *iommu);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainIOMMUDef, virDomainIOMMUDefFree);
 virDomainVsockDef *virDomainVsockDefNew(virDomainXMLOption *xmlopt);
 void virDomainVsockDefFree(virDomainVsockDef *vsock);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainVsockDef, virDomainVsockDefFree);
-- 
2.31.1

[libvirt PATCH 01/21] DONOTMERGEYET: Add replies for QEMU 6.2.0 on x86_64

[Andrea Bolognani]

These were generated using a QEMU binary built from commit

  v6.1.0-1220-g5564f06816

with

  https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00161.html
  https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07819.html

applied on top.

Signed-off-by: Andrea Bolognani 
---
 .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml  |   220 +
 .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml  |   226 +
 tests/domaincapsdata/qemu_6.2.0.x86_64.xml|   220 +
 .../caps_6.2.0.x86_64.replies | 34273 
 .../caps_6.2.0.x86_64.xml |  3722 ++
 5 files changed, 38661 insertions(+)
 create mode 100644 tests/domaincapsdata/qemu_6.2.0-q35.x86_64.xml
 create mode 100644 tests/domaincapsdata/qemu_6.2.0-tcg.x86_64.xml
 create mode 100644 tests/domaincapsdata/qemu_6.2.0.x86_64.xml
 create mode 100644 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
 create mode 100644 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml

diff --git a/tests/domaincapsdata/qemu_6.2.0-q35.x86_64.xml 
b/tests/domaincapsdata/qemu_6.2.0-q35.x86_64.xml
new file mode 100644
index 00..df8bdae102
--- /dev/null
+++ b/tests/domaincapsdata/qemu_6.2.0-q35.x86_64.xml
@@ -0,0 +1,220 @@
+
+  /usr/bin/qemu-system-x86_64
+  kvm
+  pc-q35-6.2
+  x86_64
[...]
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies 
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
new file mode 100644
index 00..53af711c9b
--- /dev/null
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.replies
@@ -0,0 +1,34273 @@
+{
+  "execute": "qmp_capabilities",
+  "id": "libvirt-1"
+}
+
+{
+  "return": {
+  },
+  "id": "libvirt-1"
+}
+
+{
+  "execute": "query-version",
+  "id": "libvirt-2"
+}
+
+{
+  "return": {
+"qemu": {
+  "micro": 50,
+  "minor": 1,
+  "major": 6
+},
+"package": "v6.1.0-1234-gd27a3d1b9e-dirty"
+  },
+  "id": "libvirt-2"
+}
[...]
-- 
2.31.1

Re: [PATCH v3] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Marc-André Lureau]

Hi

On Fri, Oct 8, 2021 at 6:44 PM Stefan Berger  wrote:

> Using swtpm v0.7.0 we can run swtpm_setup to create default config files
> for swtpm_setup and swtpm-localca in session mode. Now a user can start
> a VM with an attached TPM without having to run this program on the
> command line before. This program needs to run once.
>
> This patch addresses the issue raised in
> https://bugzilla.redhat.com/show_bug.cgi?id=2010649
>
> Signed-off-by: Stefan Berger 
>
> ---
> v3:
>   - Removed logfile parameter
>

I guess it's redirected to libvirt log then?


> v2:
>   - fixed return code if swtpm_setup doesn't support the option
> ---
>  src/qemu/qemu_tpm.c | 39 +++
>  src/util/virtpm.c   |  1 +
>  src/util/virtpm.h   |  1 +
>  3 files changed, 41 insertions(+)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 100481503c..434c357c24 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -385,6 +385,42 @@ qemuTPMSetupEncryption(const unsigned char
> *secretuuid,
>  return virCommandSetSendBuffer(cmd, g_steal_pointer(),
> secret_len);
>  }
>
> +
> +/*
> + * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files
> skip-if-exist
> + */
> +static int
> +qemuTPMCreateConfigFiles(void)
> +{
> +g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> +g_autoptr(virCommand) cmd = NULL;
> +int exitstatus;
> +
> +if (!swtpm_setup)
> +return -1;
>

I think what Daniel was saying is that this shouldn't fail suddenly for
users with older swtpm that already have the configuration setup.

+
> +if (!virTPMSwtpmSetupCapsGet(
> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
> +return 0;
> +
> +cmd = virCommandNew(swtpm_setup);
> +if (!cmd)
> +return -1;
> +
> +virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist",
> NULL);
> +virCommandClearCaps(cmd);
> +
> +if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Could not run '%s' to create config files.
> exitstatus: %d",
> +  swtpm_setup, exitstatus);
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
>  /*
>   * qemuTPMEmulatorRunSetup
>   *
> @@ -432,6 +468,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
>   "this requires privileged mode for a "
>   "TPM 1.2\n"), 0600);
>
> +if (!privileged && qemuTPMCreateConfigFiles())
> +return -1;
> +
>  cmd = virCommandNew(swtpm_setup);
>  if (!cmd)
>  return -1;
> diff --git a/src/util/virtpm.c b/src/util/virtpm.c
> index 1a567139b4..0f50de866c 100644
> --- a/src/util/virtpm.c
> +++ b/src/util/virtpm.c
> @@ -45,6 +45,7 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature,
>  VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
>VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
>"cmdarg-pwdfile-fd",
> +  "cmdarg-create-config-files",
>  );
>
>  /**
> diff --git a/src/util/virtpm.h b/src/util/virtpm.h
> index d021a083b4..3bb03b3b33 100644
> --- a/src/util/virtpm.h
> +++ b/src/util/virtpm.h
> @@ -38,6 +38,7 @@ typedef enum {
>
>  typedef enum {
>  VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD,
> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES,
>
>  VIR_TPM_SWTPM_SETUP_FEATURE_LAST
>  } virTPMSwtpmSetupFeature;
> --
> 2.31.1
>
>

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

On 10/8/21 10:56 AM, Stefan Berger wrote:


On 10/8/21 10:52 AM, Daniel P. Berrangé wrote:

On Fri, Oct 08, 2021 at 09:56:35AM -0400, Stefan Berger wrote:
Using swtpm v0.7.0 we can run swtpm_setup to create default config 
files

for swtpm_setup and swtpm-localca in session mode. Now a user can start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.

This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649

BTW, I notice the this tool creates certs under $HOME/.config/var
with an expiry date of +10 years.

Now that sounds like a long time, and indeed it is a long time,
but then I look at the support lifetime of RHEL... Hopefully
bare metal hardware won't last for the whole 10 years without
being replaced, but with nested virt the "hosts" could be VMs
that get moved to new hardware.

So what's the story if a host hits the 10 year mark for the
swtpm certs ? Presumably swtpm is validating these dates
and will refuse to launch the TPM for the VMs on the host ?

It doesn't.


I am switching to non-expiring certificates now which should help 
address this issue for future CAs.


These CAs 'created on the fly' were thought of merely as a convenience 
for the user and someone more serious about the TPM CAs would create 
them on their own and use appropriate dates for the expiration and 
manage these certificates before they expire. In a larger setting all 
hosts should share a fairly well-known TPM CA so that all vTPMs' 
certificates are signed with the same CA and certificate validators 
don't need to have n hosts' certs but just '1'. However, that requires 
setup by an admin rather than relying on CAs 'created on the fly'.


Thanks for the feedback

    Stefan





Regards,
Daniel

Re: [PATCH v7 0/4] introduce support for acpi-bridge-hotplug feature

[Laine Stump]

Reviewed-by: Laine Stump 

I'm going to push this later today after I put it through CI (and get 
back from a mandatory 4 hour drive).


On 10/8/21 2:42 AM, Ani Sinha wrote:

changelog:

v7: Laine's suggestions from v6 incorporated. rebased the patchset to latest
 master.
v6: rebased to latest. capabilities have been renamed as per suggestions that
 were made here:
 https://listman.redhat.com/archives/libvir-list/2021-October/msg00061.html
v5: rebased the patchset with the latest master.
v4: split the original series into two - pci-root controller specific one
 (https://www.mail-archive.com/libvir-list@redhat.com/msg221645.html)
 and this one specific to pci bridges.
 The conf xml has been introduced as per suggestion by Berrange here:
 https://patchew.org/Libvirt/20210912032631.2853520-1-...@anisinha.ca
 Changes has been introduced to parse and validate the xml accordingly
 as well as to add backend qemu commandline option.
v3: reorganized the patches as per Laine's suggestion. Added more
 details in commit messages. Added conf description in formatdomain.rst.
 Added changelog for next release.
v2: fixed bugs and added additional missing unit tests.
v1: initial implementation. Had some bugs and missed some unit tests

This change introduces a new libvirt sub-element  under  that
can be used to configure all pci related features.
Currently the only sub-sub element supported by this sub-element is
'acpi-bridge-hotplug' as shown below:


   
 
   


The above option is only available for qemu driver and that too for x86 guests
only. It is a global option.

'acpi-bridge-hotplug' option enables or disables ACPI hotplug support for
cold-plugged pci bridges. Examples of bridges include PCI-PCI bridge
(pci-bridge controller) or PCIe-PCI bridges for pc machines and
pcie-root-port controller for q35 machines. Being global option, no other
bridge specific option are required. For pc machine type in x86, this option
is available in qemu for a long time, from version 2.1.
Please see the following changes in qemu repo:

9e047b982452c6 ("piix4: add acpi pci hotplug support")
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug 
is disabled")

For q35 machine type, this was introduced in qemu 6.1 with the following
changes in qemu repo:

(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
(b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")

The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as
opposed to native hotplug) are outlined in (b). There are use cases where users
would still want to use native hotplug (see notes). Therefore, this config 
option
enables users to choose either ACPI based hotplug or native hotplug for bridges
(for example for pcie root port controller in q35 machines).

Notes:
One concrete example of why one might still want to use native hotplug with
pcie-root-port controller is the fact that we are still discovering issues with
acpi hotplug on PCIE. One such issue is:
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html
Another reason is that users have been using native hotplug on pcie root ports
up until now. They have built and tested their systems based on native hotplug.
They may not want to suddenly move to acpi based hotplug just because it is now
the default in qemu. Supporting the option to chose one or the other through
libvirt makes things simpler for end users.

Ani Sinha (4):
   qemu: capablities: detect presence of
 acpi-pci-hotplug-with-bridge-support
   conf: introduce support for acpi-bridge-hotplug feature
   qemu: command: add support for acpi-bridge-hotplug feature
   NEWS: add new acpi pci hotplug config option in the release note for
 next release

  NEWS.rst  |  8 ++
  docs/formatdomain.rst | 16 
  docs/schemas/domaincommon.rng | 15 
  src/conf/domain_conf.c| 89 ++-
  src/conf/domain_conf.h|  9 ++
  src/qemu/qemu_capabilities.c  |  4 +
  src/qemu/qemu_capabilities.h  |  2 +
  src/qemu/qemu_command.c   | 20 +
  src/qemu/qemu_validate.c  | 46 ++
  .../caps_2.11.0.x86_64.xml|  1 +
  .../caps_2.12.0.x86_64.xml|  1 +
  .../caps_3.0.0.x86_64.xml |  1 +
  .../caps_3.1.0.x86_64.xml |  1 +
  .../caps_4.0.0.x86_64.xml |  1 +
  .../caps_4.1.0.x86_64.xml |  1 +
  .../caps_4.2.0.x86_64.xml |  1 +
  .../caps_5.0.0.x86_64.xml |  1 +
  .../caps_5.1.0.x86_64.xml |  1 +
  .../caps_5.2.0.x86_64.xml |  1 +
  .../caps_6.0.0.x86_64.xml |  1 +
  .../caps_6.1.0.x86_64.xml |  

Re: Cancellation of VM core dumps

[Jiri Denemark]

On Tue, Oct 05, 2021 at 14:30:43 +, Simon Rowe wrote:
> I see that cancellation of core dump jobs that are memory-only are explicitly 
> rejected
> 
> 
> >>> d.abortJob()
> 
> libvirt: QEMU Driver error : Requested operation is not valid: cannot abort 
> memory-only dump
> 
> Traceback (most recent call last):
> 
>   File "", line 1, in 
> 
>   File "/usr/lib64/python2.7/site-packages/libvirt.py", line 544, in abortJob
> 
> if ret == -1: raise libvirtError ('virDomainAbortJob() failed', dom=self)
> 
> libvirt.libvirtError: Requested operation is not valid: cannot abort 
> memory-only dump
> 
> Is there a technical reason for this? Dumps of very large guests may take a 
> long time and it would be useful to be able to abort them.

Yes. Historically, dumps (not the memory-only ones) were implemented
using migration to a file and that process can easily be aborted.
However, memory-only dumps use a special QEMU command and thus can't be
aborted in the same way libvirt abort migrations. That said we would
need to modify libvirt and likely QEMU as well to be able to abort
memory-only dumps.

Jirka

Re: [PATCH v2 14/15] qdev: Base object creation on QDict rather than QemuOpts

[Laurent Vivier]

On 08/10/2021 15:34, Kevin Wolf wrote:

QDicts are both what QMP natively uses and what the keyval parser
produces. Going through QemuOpts isn't useful for either one, so switch
the main device creation function to QDicts. By sharing more code with
the -object/object-add code path, we can even reduce the code size a
bit.

This commit doesn't remove the detour through QemuOpts from any code
path yet, but it allows the following commits to do so.

Signed-off-by: Kevin Wolf 
---
  include/hw/qdev-core.h | 11 +++---
  include/hw/virtio/virtio-net.h |  3 +-
  include/monitor/qdev.h |  2 +
  hw/core/qdev.c |  7 ++--
  hw/net/virtio-net.c| 23 +++-
  hw/vfio/pci.c  |  4 +-
  softmmu/qdev-monitor.c | 69 +++---
  7 files changed, 60 insertions(+), 59 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 74d8b614a7..910042c650 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,7 +180,7 @@ struct DeviceState {
  char *canonical_path;
  bool realized;
  bool pending_deleted_event;
-QemuOpts *opts;
+QDict *opts;
  int hotplugged;
  bool allow_unplug_during_migration;
  BusState *parent_bus;
@@ -205,8 +205,8 @@ struct DeviceListener {
   * On errors, it returns false and errp is set. Device creation
   * should fail in this case.
   */
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
-Error **errp);
+bool (*hide_device)(DeviceListener *listener, const QDict *device_opts,
+bool from_json, Error **errp);
  QTAILQ_ENTRY(DeviceListener) link;
  };
  
@@ -835,13 +835,14 @@ void device_listener_unregister(DeviceListener *listener);
  
  /**

   * @qdev_should_hide_device:
- * @opts: QemuOpts as passed on cmdline.
+ * @opts: options QDict > + * @from_json: true if @opts entries are typed, 
false for all strings


Add the errp here too:

* @errp: pointer to error object


   *
   * Check if a device should be added.
   * When a device is added via qdev_device_add() this will be called,
   * and return if the device should be added now or not.
   */
-bool qdev_should_hide_device(QemuOpts *opts, Error **errp);
+bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
  
  typedef enum MachineInitPhase {

  /* current_machine is NULL.  */


Thank you to have added the errp pointer in the hide_device helpers, it helps 
in my series.

Laurent

Re: [libvirt PATCH v3 6/6] qemu: drop support for full CPU model expansion

[Jiri Denemark]

On Fri, Oct 08, 2021 at 10:01:45 +0100, Daniel P. Berrangé wrote:
> The "canonical CPU features" capability is a derivative of the
> "unavailable features" capability, which is exposed when seeing
> the "max" CPU models has the "unavailable-features" property.
> 
> This property was actually added back in QEMU version 2.8.0 per
> the QAPI schema
> 
>   @unavailable-features: List of properties that prevent
>  the CPU model from running in the current
>  host. (since 2.8)
> 
> so given our minimum QEMU version 2.11 there is no need to
> query this.
> 
> XXX strangely when we stop querying this, the domain
> capabilities data for CPUs changes significantly for QEMU
> versions less than 4.1.0. This suggests this code was masking
> a need for some other capability check that would trigger for
> QEMU < 4.1.0 ?

This is because QEMU_CAPS_CANONICAL_CPU_FEATURES was not enabled in the
capabilities data files for QEMU < 4.1.0. And since this feature is just
an alias for QEMU_CAPS_CPU_UNAVAILABLE_FEATURES, the situation is caused
by missing this UNAVAILABLE_FEATURES capability. And indeed
qom-list-properties on max-x86_64-cpu does not list unavailable-features
This property is only supported by query-cpu-definitions, which is what
you found in the QAPI schema. Similarly named property of a CPU class
was apparently added only in 4.1.0.

Fortunately QEMU_CAPS_CPU_UNAVAILABLE_FEATURES was only selected as a
conservative witness for QEMU which supports hyphens in all feature
names at the time I implemented this in libvirt.

That said, I believe it is safe to apply this patch. But please, rewrite
the misleading commit message.

And I think there's one more patch missing as we can now remove
QEMU_CAPS_CANONICAL_CPU_FEATURES completely (well, by adding the X_
prefix).

Jirka

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

On 10/8/21 10:52 AM, Daniel P. Berrangé wrote:

On Fri, Oct 08, 2021 at 09:56:35AM -0400, Stefan Berger wrote:

Using swtpm v0.7.0 we can run swtpm_setup to create default config files
for swtpm_setup and swtpm-localca in session mode. Now a user can start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.

This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649

BTW, I notice the this tool creates certs under $HOME/.config/var
with an expiry date of +10 years.

Now that sounds like a long time, and indeed it is a long time,
but then I look at the support lifetime of RHEL... Hopefully
bare metal hardware won't last for the whole 10 years without
being replaced, but with nested virt the "hosts" could be VMs
that get moved to new hardware.

So what's the story if a host hits the 10 year mark for the
swtpm certs ? Presumably swtpm is validating these dates
and will refuse to launch the TPM for the VMs on the host ?

It doesn't.



Regards,
Daniel

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Daniel P . Berrangé]

On Fri, Oct 08, 2021 at 10:51:24AM -0400, Stefan Berger wrote:
> 
> On 10/8/21 10:43 AM, Daniel P. Berrangé wrote:
> > On Fri, Oct 08, 2021 at 09:56:35AM -0400, Stefan Berger wrote:
> > > Using swtpm v0.7.0 we can run swtpm_setup to create default config files
> > > for swtpm_setup and swtpm-localca in session mode. Now a user can start
> > > a VM with an attached TPM without having to run this program on the
> > > command line before. This program needs to run once.
> > Fedora 34 only has v0.6.0 and so
> 
> This is a new feature that will come out with v0.7.0.
> 
> 
> > 
> > > This patch addresses the issue raised in
> > > https://bugzilla.redhat.com/show_bug.cgi?id=2010649
> > > 
> > > Signed-off-by: Stefan Berger 
> > > 
> > > v2:
> > >- fixed return code if swtpm_setup doesn't support the option
> > > ---
> > >   src/qemu/qemu_tpm.c | 43 +++
> > >   src/util/virtpm.c   |  1 +
> > >   src/util/virtpm.h   |  1 +
> > >   3 files changed, 45 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > > index 100481503c..bf6c8e5ad5 100644
> > > --- a/src/qemu/qemu_tpm.c
> > > +++ b/src/qemu/qemu_tpm.c
> > > @@ -385,6 +385,46 @@ qemuTPMSetupEncryption(const unsigned char 
> > > *secretuuid,
> > >   return virCommandSetSendBuffer(cmd, g_steal_pointer(), 
> > > secret_len);
> > >   }
> > > +
> > > +/*
> > > + * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files 
> > > skip-if-exist
> > > + *
> > > + * @logfile: The file to write the log into; it must be writable
> > > + *   for the user given by userid or 'tss'
> > > + */
> > > +static int
> > > +qemuTPMCreateConfigFiles(const char *logfile)
> > > +{
> > > +g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> > > +g_autoptr(virCommand) cmd = NULL;
> > > +int exitstatus;
> > > +
> > > +if (!swtpm_setup)
> > > +return -1;
> > > +
> > > +if (!virTPMSwtpmSetupCapsGet(
> > > +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
> > > +return 0;
> > > +
> > > +cmd = virCommandNew(swtpm_setup);
> > > +if (!cmd)
> > > +return -1;
> > > +
> > > +virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist", 
> > > NULL);
> > > +virCommandClearCaps(cmd);
> > > +
> > > +if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
> > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > +   _("Could not run '%s' to create config files. 
> > > exitstatus: %d; "
> > > + "Check error log '%s' for details."),
> > > +  swtpm_setup, exitstatus, logfile);
> > This error path will trigger preventing use of the TPM, even if
> > the user has manually setup the config themselves.
> 
> skip-if-exists results in exit code 0 if any one of the 3 expected files
> exist.

No, you mis-understand me.  I have creates the cofnig by running
/usr/share/swtpm/swtpm-create-user-config-files.

Now this libvirt tries to invoke swtpm_setup --create-config-files 
skip-if-exists which fails to parse the command line, as I don't have version 
0.7.0,
and thus prevents my VMs working

> > Why aren't you running /usr/share/swtpm/swtpm-create-user-config-files
> > instead which is what I see does exist on Fedora today.
> > 
> > RHEL-8 has even older swtpm than Fedora.
> 
> This patch will be backported then and not regarded as new feature?

We can't assume distros will backport new cli options like that. We need
to be able to work correctly with the version of swtpm that is shipped
in the various distros today.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

On 10/8/21 10:43 AM, Daniel P. Berrangé wrote:


This error path will trigger preventing use of the TPM, even if
the user has manually setup the config themselves.

Why aren't you running /usr/share/swtpm/swtpm-create-user-config-files
instead which is what I see does exist on Fedora today.


That one will exit with error code '1' if any one file exists:

# /usr/share/swtpm/swtpm-create-user-config-files
File /home/stefanb/.config/swtpm_setup.conf already exists. Refusing to 
overwrite.

# echo $?
1

It wasn't designed to be run by libvirt but by the user on the command line.


   Stefan




RHEL-8 has even older swtpm than Fedora.

Regards,
Daniel

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Daniel P . Berrangé]

On Fri, Oct 08, 2021 at 09:56:35AM -0400, Stefan Berger wrote:
> Using swtpm v0.7.0 we can run swtpm_setup to create default config files
> for swtpm_setup and swtpm-localca in session mode. Now a user can start
> a VM with an attached TPM without having to run this program on the
> command line before. This program needs to run once.
> 
> This patch addresses the issue raised in
> https://bugzilla.redhat.com/show_bug.cgi?id=2010649

BTW, I notice the this tool creates certs under $HOME/.config/var
with an expiry date of +10 years.

Now that sounds like a long time, and indeed it is a long time,
but then I look at the support lifetime of RHEL... Hopefully
bare metal hardware won't last for the whole 10 years without
being replaced, but with nested virt the "hosts" could be VMs
that get moved to new hardware.

So what's the story if a host hits the 10 year mark for the
swtpm certs ? Presumably swtpm is validating these dates
and will refuse to launch the TPM for the VMs on the host ?


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

On 10/8/21 10:43 AM, Daniel P. Berrangé wrote:

On Fri, Oct 08, 2021 at 09:56:35AM -0400, Stefan Berger wrote:

Using swtpm v0.7.0 we can run swtpm_setup to create default config files
for swtpm_setup and swtpm-localca in session mode. Now a user can start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.

Fedora 34 only has v0.6.0 and so


This is a new feature that will come out with v0.7.0.





This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649

Signed-off-by: Stefan Berger 

v2:
   - fixed return code if swtpm_setup doesn't support the option
---
  src/qemu/qemu_tpm.c | 43 +++
  src/util/virtpm.c   |  1 +
  src/util/virtpm.h   |  1 +
  3 files changed, 45 insertions(+)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 100481503c..bf6c8e5ad5 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -385,6 +385,46 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
  return virCommandSetSendBuffer(cmd, g_steal_pointer(), secret_len);
  }
  
+

+/*
+ * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files 
skip-if-exist
+ *
+ * @logfile: The file to write the log into; it must be writable
+ *   for the user given by userid or 'tss'
+ */
+static int
+qemuTPMCreateConfigFiles(const char *logfile)
+{
+g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
+g_autoptr(virCommand) cmd = NULL;
+int exitstatus;
+
+if (!swtpm_setup)
+return -1;
+
+if (!virTPMSwtpmSetupCapsGet(
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
+return 0;
+
+cmd = virCommandNew(swtpm_setup);
+if (!cmd)
+return -1;
+
+virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist", NULL);
+virCommandClearCaps(cmd);
+
+if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not run '%s' to create config files. exitstatus: 
%d; "
+ "Check error log '%s' for details."),
+  swtpm_setup, exitstatus, logfile);

This error path will trigger preventing use of the TPM, even if
the user has manually setup the config themselves.


skip-if-exists results in exit code 0 if any one of the 3 expected files 
exist.





Why aren't you running /usr/share/swtpm/swtpm-create-user-config-files
instead which is what I see does exist on Fedora today.

RHEL-8 has even older swtpm than Fedora.


This patch will be backported then and not regarded as new feature?




Regards,
Daniel

[PATCH v3] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

Using swtpm v0.7.0 we can run swtpm_setup to create default config files
for swtpm_setup and swtpm-localca in session mode. Now a user can start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.

This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649

Signed-off-by: Stefan Berger 

---
v3:
  - Removed logfile parameter

v2:
  - fixed return code if swtpm_setup doesn't support the option
---
 src/qemu/qemu_tpm.c | 39 +++
 src/util/virtpm.c   |  1 +
 src/util/virtpm.h   |  1 +
 3 files changed, 41 insertions(+)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 100481503c..434c357c24 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -385,6 +385,42 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
 return virCommandSetSendBuffer(cmd, g_steal_pointer(), secret_len);
 }
 
+
+/*
+ * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files 
skip-if-exist
+ */
+static int
+qemuTPMCreateConfigFiles(void)
+{
+g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
+g_autoptr(virCommand) cmd = NULL;
+int exitstatus;
+
+if (!swtpm_setup)
+return -1;
+
+if (!virTPMSwtpmSetupCapsGet(
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
+return 0;
+
+cmd = virCommandNew(swtpm_setup);
+if (!cmd)
+return -1;
+
+virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist", NULL);
+virCommandClearCaps(cmd);
+
+if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not run '%s' to create config files. 
exitstatus: %d",
+  swtpm_setup, exitstatus);
+return -1;
+}
+
+return 0;
+}
+
+
 /*
  * qemuTPMEmulatorRunSetup
  *
@@ -432,6 +468,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
  "this requires privileged mode for a "
  "TPM 1.2\n"), 0600);
 
+if (!privileged && qemuTPMCreateConfigFiles())
+return -1;
+
 cmd = virCommandNew(swtpm_setup);
 if (!cmd)
 return -1;
diff --git a/src/util/virtpm.c b/src/util/virtpm.c
index 1a567139b4..0f50de866c 100644
--- a/src/util/virtpm.c
+++ b/src/util/virtpm.c
@@ -45,6 +45,7 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature,
 VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
   VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
   "cmdarg-pwdfile-fd",
+  "cmdarg-create-config-files",
 );
 
 /**
diff --git a/src/util/virtpm.h b/src/util/virtpm.h
index d021a083b4..3bb03b3b33 100644
--- a/src/util/virtpm.h
+++ b/src/util/virtpm.h
@@ -38,6 +38,7 @@ typedef enum {
 
 typedef enum {
 VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD,
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES,
 
 VIR_TPM_SWTPM_SETUP_FEATURE_LAST
 } virTPMSwtpmSetupFeature;
-- 
2.31.1

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Daniel P . Berrangé]

On Fri, Oct 08, 2021 at 09:56:35AM -0400, Stefan Berger wrote:
> Using swtpm v0.7.0 we can run swtpm_setup to create default config files
> for swtpm_setup and swtpm-localca in session mode. Now a user can start
> a VM with an attached TPM without having to run this program on the
> command line before. This program needs to run once.

Fedora 34 only has v0.6.0 and so

> 
> This patch addresses the issue raised in
> https://bugzilla.redhat.com/show_bug.cgi?id=2010649
> 
> Signed-off-by: Stefan Berger 
> 
> v2:
>   - fixed return code if swtpm_setup doesn't support the option
> ---
>  src/qemu/qemu_tpm.c | 43 +++
>  src/util/virtpm.c   |  1 +
>  src/util/virtpm.h   |  1 +
>  3 files changed, 45 insertions(+)
> 
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 100481503c..bf6c8e5ad5 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -385,6 +385,46 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
>  return virCommandSetSendBuffer(cmd, g_steal_pointer(), 
> secret_len);
>  }
>  
> +
> +/*
> + * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files 
> skip-if-exist
> + *
> + * @logfile: The file to write the log into; it must be writable
> + *   for the user given by userid or 'tss'
> + */
> +static int
> +qemuTPMCreateConfigFiles(const char *logfile)
> +{
> +g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> +g_autoptr(virCommand) cmd = NULL;
> +int exitstatus;
> +
> +if (!swtpm_setup)
> +return -1;
> +
> +if (!virTPMSwtpmSetupCapsGet(
> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
> +return 0;
> +
> +cmd = virCommandNew(swtpm_setup);
> +if (!cmd)
> +return -1;
> +
> +virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist", 
> NULL);
> +virCommandClearCaps(cmd);
> +
> +if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Could not run '%s' to create config files. 
> exitstatus: %d; "
> + "Check error log '%s' for details."),
> +  swtpm_setup, exitstatus, logfile);

This error path will trigger preventing use of the TPM, even if
the user has manually setup the config themselves.

Why aren't you running /usr/share/swtpm/swtpm-create-user-config-files
instead which is what I see does exist on Fedora today.

RHEL-8 has even older swtpm than Fedora.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v7 1/4] qemu: capablities: detect presence of acpi-pci-hotplug-with-bridge-support

[Ani Sinha]

On Fri, Oct 8, 2021 at 12:12 PM Ani Sinha  wrote:

> qemu added support for i440fx specific global boolean flag
>
> PIIX4_PM.acpi-pci-hotplug-with-bridge-support
>
> around version 2.1. This flag is enabled by default. When disabled, it
> turns
> off acpi pci hotplug for cold plugged pci bridges in i440fx machine types.
>
> Very recently, in qemu version 6.1, the same global option was also added
> for
> q35 machine types as well.
>
> ICH9-LPC.acpi-pci-hotplug-with-bridge-support
>
> This option turns on or off acpi based hotplug for cold plugged pcie
> bridges
> like pcie root ports. This flag is also enabled by default. Please refer to
> the following qemu changes:
>
> c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
>
> This patch adds the corresponding qemu capabilities in libvirt. For i440fx,
> the capability is detected as QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE.


Oops forgot to update the commit message here. It should be
QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE




For q35,
> the capability is detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE.
>
> Please note that the test specific qemu capabilities .replies files has
> already
> been updated as a part of regular refreshing them when a new qemu version
> is
> released. Hence, no updates to those files are required.
>
> Signed-off-by: Ani Sinha 
> Reviewed-by: Laine Stump 
> ---
>  src/qemu/qemu_capabilities.c  | 4 
>  src/qemu/qemu_capabilities.h  | 2 ++
>  tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml  | 2 ++
>  14 files changed, 19 insertions(+)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 82687dbf39..c4d0e1858c 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -644,6 +644,8 @@ VIR_ENUM_IMPL(virQEMUCaps,
>"virtio-mem-pci", /* QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI */
>"memory-backend-file.reserve", /*
> QEMU_CAPS_MEMORY_BACKEND_RESERVE */
>"piix4.acpi-root-pci-hotplug", /*
> QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG */
> +  "piix4.acpi-hotplug-bridge", /*
> QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE */
> +  "ich9.acpi-hotplug-bridge", /*
> QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */
>  );
>
>
> @@ -1472,6 +1474,7 @@ static struct virQEMUCapsDevicePropsFlags
> virQEMUCapsDevicePropsPiix4PM[] = {
>  { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL },
>  { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL },
>  { "acpi-root-pci-hotplug", QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG,
> NULL },
> +{ "acpi-pci-hotplug-with-bridge-support",
> QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE, NULL },
>  };
>
>  static struct virQEMUCapsDevicePropsFlags
> virQEMUCapsDevicePropsUSBRedir[] = {
> @@ -1524,6 +1527,7 @@ static struct virQEMUCapsDevicePropsFlags
> virQEMUCapsDevicePropsVirtioGpu[] = {
>  static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = {
>  { "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL },
>  { "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL },
> +{ "acpi-pci-hotplug-with-bridge-support",
> QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL },
>  };
>
>  static struct virQEMUCapsDevicePropsFlags
> virQEMUCapsDevicePropsUSBNECXHCI[] = {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 2bbfc15dc4..e9bd6c8885 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -624,6 +624,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
> syntax-check */
>  QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI, /* -device virtio-mem-pci */
>  QEMU_CAPS_MEMORY_BACKEND_RESERVE, /* -object
> memory-backend-*.reserve= */
>  QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG, /* -M pc
> PIIX4_PM.acpi-root-pci-hotplug */
> +QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE, /* -M pc
> PIIX4_PM.acpi-pci-hotplug-with-bridge-support */
> +QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35
> ICH9-LPC.acpi-pci-hotplug-with-bridge-support */
>
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
> b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
> index d6549d6440..65bfe911dd 100644
> --- 

Re: [PATCH 070/103] qemuBuildPCIHostdevDevProps: Move 'failover_pair_id' property before address

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c| 6 +-
tests/qemuxml2argvdata/net-virtio-teaming-hostdev.args | 4 ++--
tests/qemuxml2argvdata/net-virtio-teaming.args | 4 ++--
3 files changed, 5 insertions(+), 9 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 069/103] qemuBuildHostdevCommandLine: Format PCI host devices via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

For properties we use these are the QEMU types:
 host= - Address (bus/device/function) of the host device, 
example: 04:10.0
 bootindex=
 failover_pair_id=

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 69 ++---
src/qemu/qemu_command.h |  6 ++--
src/qemu/qemu_hotplug.c |  6 ++--
3 files changed, 43 insertions(+), 38 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 068/103] qemuCommandAddExtDevice: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Generate the 'zpci' device via JSON.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 81 +++--
src/qemu/qemu_command.h |  3 +-
src/qemu/qemu_hotplug.c | 27 +-
3 files changed, 49 insertions(+), 62 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 067/103] qemuBuildInputCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

For 'usb-mouse'/'usb-tablet'/'usb-kbd' we don't use any special
property.

For 'virtio-input-pci' we only use the 'evdev' argument which is a
string so this conversion doesn't impact anything.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 107 +---
src/qemu/qemu_command.h |  14 +++---
src/qemu/qemu_hotplug.c |  46 ++---
3 files changed, 91 insertions(+), 76 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 066/103] qemuBuildRedirdevCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The 'usb-redir' device has the following types according to QEMU for
properties we control:

 chardev=  - ID of a chardev to use as a backend
 filter=
 bootindex=

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 49 +++--
src/qemu/qemu_command.h |  6 ++---
src/qemu/qemu_hotplug.c |  6 ++---
3 files changed, 33 insertions(+), 28 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Marc-André Lureau]

On Fri, Oct 8, 2021 at 5:56 PM Stefan Berger  wrote:

> Using swtpm v0.7.0 we can run swtpm_setup to create default config files
> for swtpm_setup and swtpm-localca in session mode. Now a user can start
> a VM with an attached TPM without having to run this program on the
> command line before. This program needs to run once.
>
> This patch addresses the issue raised in
> https://bugzilla.redhat.com/show_bug.cgi?id=2010649
>
> Signed-off-by: Stefan Berger 
>

> v2:
>   - fixed return code if swtpm_setup doesn't support the option
> ---
>  src/qemu/qemu_tpm.c | 43 +++
>  src/util/virtpm.c   |  1 +
>  src/util/virtpm.h   |  1 +
>  3 files changed, 45 insertions(+)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 100481503c..bf6c8e5ad5 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -385,6 +385,46 @@ qemuTPMSetupEncryption(const unsigned char
> *secretuuid,
>  return virCommandSetSendBuffer(cmd, g_steal_pointer(),
> secret_len);
>  }
>
> +
> +/*
> + * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files
> skip-if-exist
> + *
> + * @logfile: The file to write the log into; it must be writable
> + *   for the user given by userid or 'tss'
> + */
> +static int
> +qemuTPMCreateConfigFiles(const char *logfile)
> +{
> +g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> +g_autoptr(virCommand) cmd = NULL;
> +int exitstatus;
> +
> +if (!swtpm_setup)
> +return -1;
> +
> +if (!virTPMSwtpmSetupCapsGet(
> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
> +return 0;
> +
> +cmd = virCommandNew(swtpm_setup);
> +if (!cmd)
> +return -1;
> +
> +virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist",
> NULL);
> +virCommandClearCaps(cmd);
> +
> +if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Could not run '%s' to create config files.
> exitstatus: %d; "
> + "Check error log '%s' for details."),
> +  swtpm_setup, exitstatus, logfile);
>

logfile isn't used, it seems

+return -1;
> +}
> +
> +return 0;
> +}
> +
> +
>  /*
>   * qemuTPMEmulatorRunSetup
>   *
> @@ -432,6 +472,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
>   "this requires privileged mode for a "
>   "TPM 1.2\n"), 0600);
>
> +if (!privileged && qemuTPMCreateConfigFiles(logfile))
> +return -1;
> +
>  cmd = virCommandNew(swtpm_setup);
>  if (!cmd)
>  return -1;
> diff --git a/src/util/virtpm.c b/src/util/virtpm.c
> index 1a567139b4..0f50de866c 100644
> --- a/src/util/virtpm.c
> +++ b/src/util/virtpm.c
> @@ -45,6 +45,7 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature,
>  VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
>VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
>"cmdarg-pwdfile-fd",
> +  "cmdarg-create-config-files",
>  );
>
>  /**
> diff --git a/src/util/virtpm.h b/src/util/virtpm.h
> index d021a083b4..3bb03b3b33 100644
> --- a/src/util/virtpm.h
> +++ b/src/util/virtpm.h
> @@ -38,6 +38,7 @@ typedef enum {
>
>  typedef enum {
>  VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD,
> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES,
>
>  VIR_TPM_SWTPM_SETUP_FEATURE_LAST
>  } virTPMSwtpmSetupFeature;
> --
> 2.31.1
>
>
lgtm otherwise

[PATCH v2] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

Using swtpm v0.7.0 we can run swtpm_setup to create default config files
for swtpm_setup and swtpm-localca in session mode. Now a user can start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.

This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649

Signed-off-by: Stefan Berger 

v2:
  - fixed return code if swtpm_setup doesn't support the option
---
 src/qemu/qemu_tpm.c | 43 +++
 src/util/virtpm.c   |  1 +
 src/util/virtpm.h   |  1 +
 3 files changed, 45 insertions(+)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 100481503c..bf6c8e5ad5 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -385,6 +385,46 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
 return virCommandSetSendBuffer(cmd, g_steal_pointer(), secret_len);
 }
 
+
+/*
+ * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files 
skip-if-exist
+ *
+ * @logfile: The file to write the log into; it must be writable
+ *   for the user given by userid or 'tss'
+ */
+static int
+qemuTPMCreateConfigFiles(const char *logfile)
+{
+g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
+g_autoptr(virCommand) cmd = NULL;
+int exitstatus;
+
+if (!swtpm_setup)
+return -1;
+
+if (!virTPMSwtpmSetupCapsGet(
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
+return 0;
+
+cmd = virCommandNew(swtpm_setup);
+if (!cmd)
+return -1;
+
+virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist", NULL);
+virCommandClearCaps(cmd);
+
+if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not run '%s' to create config files. 
exitstatus: %d; "
+ "Check error log '%s' for details."),
+  swtpm_setup, exitstatus, logfile);
+return -1;
+}
+
+return 0;
+}
+
+
 /*
  * qemuTPMEmulatorRunSetup
  *
@@ -432,6 +472,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
  "this requires privileged mode for a "
  "TPM 1.2\n"), 0600);
 
+if (!privileged && qemuTPMCreateConfigFiles(logfile))
+return -1;
+
 cmd = virCommandNew(swtpm_setup);
 if (!cmd)
 return -1;
diff --git a/src/util/virtpm.c b/src/util/virtpm.c
index 1a567139b4..0f50de866c 100644
--- a/src/util/virtpm.c
+++ b/src/util/virtpm.c
@@ -45,6 +45,7 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature,
 VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
   VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
   "cmdarg-pwdfile-fd",
+  "cmdarg-create-config-files",
 );
 
 /**
diff --git a/src/util/virtpm.h b/src/util/virtpm.h
index d021a083b4..3bb03b3b33 100644
--- a/src/util/virtpm.h
+++ b/src/util/virtpm.h
@@ -38,6 +38,7 @@ typedef enum {
 
 typedef enum {
 VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD,
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES,
 
 VIR_TPM_SWTPM_SETUP_FEATURE_LAST
 } virTPMSwtpmSetupFeature;
-- 
2.31.1

Re: [PATCH 065/103] qemuBuildHostdevMediatedDevProps: Format USB host devices via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The 'usb-host' device has the following types according to QEMU for
properties we control:

 hostdevice=
 hostbus=   -  (default: 0)
 hostaddr=  -  (default: 0)
 bootindex=

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 46 -
src/qemu/qemu_command.h |  7 ---
src/qemu/qemu_hotplug.c |  6 +++---
3 files changed, 34 insertions(+), 25 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[PATCH v2 07/15] qdev: Avoid using string visitor for properties

[Kevin Wolf]

The only thing the string visitor adds compared to a keyval visitor is
list support. git grep for 'visit_start_list' and 'visit.*List' shows
that devices don't make use of this.

In a world with a QAPIfied command line interface, the keyval visitor is
used to parse the command line. In order to make sure that no devices
start using this feature that would make backwards compatibility harder,
just switch away from object_property_parse(), which internally uses the
string visitor, to a keyval visitor and object_property_set().

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 softmmu/qdev-monitor.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0705f00846..034b999401 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -28,6 +28,8 @@
 #include "qapi/qmp/dispatch.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
@@ -198,16 +200,28 @@ static int set_property(void *opaque, const char *name, 
const char *value,
 Error **errp)
 {
 Object *obj = opaque;
+QString *val;
+Visitor *v;
+int ret;
 
 if (strcmp(name, "driver") == 0)
 return 0;
 if (strcmp(name, "bus") == 0)
 return 0;
 
-if (!object_property_parse(obj, name, value, errp)) {
-return -1;
+val = qstring_from_str(value);
+v = qobject_input_visitor_new_keyval(QOBJECT(val));
+
+if (!object_property_set(obj, name, v, errp)) {
+ret = -1;
+goto out;
 }
-return 0;
+
+ret = 0;
+out:
+visit_free(v);
+qobject_unref(val);
+return ret;
 }
 
 static const char *find_typename_by_alias(const char *alias)
-- 
2.31.1

[PATCH v2 10/15] qemu-option: Allow deleting opts during qemu_opts_foreach()

[Kevin Wolf]

Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted
while iterating through the whole list.

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 util/qemu-option.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 61cb4a97bd..eedd08929b 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1126,11 +1126,11 @@ int qemu_opts_foreach(QemuOptsList *list, 
qemu_opts_loopfunc func,
   void *opaque, Error **errp)
 {
 Location loc;
-QemuOpts *opts;
+QemuOpts *opts, *next;
 int rc = 0;
 
 loc_push_none();
-QTAILQ_FOREACH(opts, >head, next) {
+QTAILQ_FOREACH_SAFE(opts, >head, next, next) {
 loc_restore(>loc);
 rc = func(opaque, opts, errp);
 if (rc) {
-- 
2.31.1

[PATCH v2 15/15] vl: Enable JSON syntax for -device

[Kevin Wolf]

Like we already do for -object, introduce support for JSON syntax in
-device, which can be kept stable in the long term and guarantees that a
single code path with identical behaviour is used for both QMP and the
command line. Compared to the QemuOpts based code, the parser contains
less surprises and has support for non-scalar options (lists and
structs). Switching management tools to JSON means that we can more
easily change the "human" CLI syntax from QemuOpts to the keyval parser
later.

In the QAPI schema, a feature flag is added to the device-add command to
allow management tools to detect support for this.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 qapi/qdev.json | 15 
 softmmu/vl.c   | 63 --
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index d75e68908b..69656b14df 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -32,17 +32,23 @@
 ##
 # @device_add:
 #
+# Add a device.
+#
 # @driver: the name of the new device's driver
 #
 # @bus: the device's parent bus (device tree path)
 #
 # @id: the device's ID, must be unique
 #
-# Additional arguments depend on the type.
-#
-# Add a device.
+# Features:
+# @json-cli: If present, the "-device" command line option supports JSON
+#syntax with a structure identical to the arguments of this
+#command.
 #
 # Notes:
+#
+# Additional arguments depend on the type.
+#
 # 1. For detailed information about this command, please refer to the
 #'docs/qdev-device-use.txt' file.
 #
@@ -67,7 +73,8 @@
 ##
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
-  'gen': false } # so we can get the additional arguments
+  'gen': false, # so we can get the additional arguments
+  'features': ['json-cli'] }
 
 ##
 # @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 55ab70eb97..af0c4cbd99 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -144,6 +144,12 @@ typedef struct ObjectOption {
 QTAILQ_ENTRY(ObjectOption) next;
 } ObjectOption;
 
+typedef struct DeviceOption {
+QDict *opts;
+Location loc;
+QTAILQ_ENTRY(DeviceOption) next;
+} DeviceOption;
+
 static const char *cpu_option;
 static const char *mem_path;
 static const char *incoming;
@@ -151,6 +157,7 @@ static const char *loadvm;
 static const char *accelerators;
 static QDict *machine_opts_dict;
 static QTAILQ_HEAD(, ObjectOption) object_opts = 
QTAILQ_HEAD_INITIALIZER(object_opts);
+static QTAILQ_HEAD(, DeviceOption) device_opts = 
QTAILQ_HEAD_INITIALIZER(device_opts);
 static ram_addr_t maxram_size;
 static uint64_t ram_slots;
 static int display_remote;
@@ -494,21 +501,39 @@ const char *qemu_get_vm_name(void)
 return qemu_name;
 }
 
-static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+static void default_driver_disable(const char *driver)
 {
-const char *driver = qemu_opt_get(opts, "driver");
 int i;
 
-if (!driver)
-return 0;
+if (!driver) {
+return;
+}
+
 for (i = 0; i < ARRAY_SIZE(default_list); i++) {
 if (strcmp(default_list[i].driver, driver) != 0)
 continue;
 *(default_list[i].flag) = 0;
 }
+}
+
+static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+{
+const char *driver = qemu_opt_get(opts, "driver");
+
+default_driver_disable(driver);
 return 0;
 }
 
+static void default_driver_check_json(void)
+{
+DeviceOption *opt;
+
+QTAILQ_FOREACH(opt, _opts, next) {
+const char *driver = qdict_get_try_str(opt->opts, "driver");
+default_driver_disable(driver);
+}
+}
+
 static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
 {
 const char *proc_name;
@@ -1311,6 +1336,7 @@ static void qemu_disable_default_devices(void)
 {
 MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
 
+default_driver_check_json();
 qemu_opts_foreach(qemu_find_opts("device"),
   default_driver_check, NULL, NULL);
 qemu_opts_foreach(qemu_find_opts("global"),
@@ -2637,6 +2663,8 @@ static void qemu_init_board(void)
 
 static void qemu_create_cli_devices(void)
 {
+DeviceOption *opt;
+
 soundhw_init();
 
 qemu_opts_foreach(qemu_find_opts("fw_cfg"),
@@ -2652,6 +2680,18 @@ static void qemu_create_cli_devices(void)
 rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
 qemu_opts_foreach(qemu_find_opts("device"),
   device_init_func, NULL, _fatal);
+QTAILQ_FOREACH(opt, _opts, next) {
+loc_push_restore(>loc);
+/*
+ * TODO Eventually we should call qmp_device_add() here to make sure it
+ * behaves the same, but QMP still has to accept incorrectly typed
+ * options until libvirt is fixed and we want to be strict on the CLI
+ * from the start, so call qdev_device_add_from_qdict() directly for
+ * now.
+ */
+

[PATCH v2 14/15] qdev: Base object creation on QDict rather than QemuOpts

[Kevin Wolf]

QDicts are both what QMP natively uses and what the keyval parser
produces. Going through QemuOpts isn't useful for either one, so switch
the main device creation function to QDicts. By sharing more code with
the -object/object-add code path, we can even reduce the code size a
bit.

This commit doesn't remove the detour through QemuOpts from any code
path yet, but it allows the following commits to do so.

Signed-off-by: Kevin Wolf 
---
 include/hw/qdev-core.h | 11 +++---
 include/hw/virtio/virtio-net.h |  3 +-
 include/monitor/qdev.h |  2 +
 hw/core/qdev.c |  7 ++--
 hw/net/virtio-net.c| 23 +++-
 hw/vfio/pci.c  |  4 +-
 softmmu/qdev-monitor.c | 69 +++---
 7 files changed, 60 insertions(+), 59 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 74d8b614a7..910042c650 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,7 +180,7 @@ struct DeviceState {
 char *canonical_path;
 bool realized;
 bool pending_deleted_event;
-QemuOpts *opts;
+QDict *opts;
 int hotplugged;
 bool allow_unplug_during_migration;
 BusState *parent_bus;
@@ -205,8 +205,8 @@ struct DeviceListener {
  * On errors, it returns false and errp is set. Device creation
  * should fail in this case.
  */
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
-Error **errp);
+bool (*hide_device)(DeviceListener *listener, const QDict *device_opts,
+bool from_json, Error **errp);
 QTAILQ_ENTRY(DeviceListener) link;
 };
 
@@ -835,13 +835,14 @@ void device_listener_unregister(DeviceListener *listener);
 
 /**
  * @qdev_should_hide_device:
- * @opts: QemuOpts as passed on cmdline.
+ * @opts: options QDict
+ * @from_json: true if @opts entries are typed, false for all strings
  *
  * Check if a device should be added.
  * When a device is added via qdev_device_add() this will be called,
  * and return if the device should be added now or not.
  */
-bool qdev_should_hide_device(QemuOpts *opts, Error **errp);
+bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
 
 typedef enum MachineInitPhase {
 /* current_machine is NULL.  */
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index d118c95f69..74a10ebe85 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,7 +209,8 @@ struct VirtIONet {
 bool failover_primary_hidden;
 bool failover;
 DeviceListener primary_listener;
-QemuOpts *primary_opts;
+QDict *primary_opts;
+bool primary_opts_from_json;
 Notifier migration_state;
 VirtioNetRssData rss_data;
 struct NetRxPkt *rx_pkt;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 74e6c55a2b..1d57bf6577 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
+DeviceState *qdev_device_add_from_qdict(const QDict *opts,
+bool from_json, Error **errp);
 
 /**
  * qdev_set_id: parent the device and set its id if provided.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c3a021c444..7f06403752 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -28,6 +28,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-qdev.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
@@ -211,14 +212,14 @@ void device_listener_unregister(DeviceListener *listener)
 QTAILQ_REMOVE(_listeners, listener, link);
 }
 
-bool qdev_should_hide_device(QemuOpts *opts, Error **errp)
+bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp)
 {
 ERRP_GUARD();
 DeviceListener *listener;
 
 QTAILQ_FOREACH(listener, _listeners, link) {
 if (listener->hide_device) {
-if (listener->hide_device(listener, opts, errp)) {
+if (listener->hide_device(listener, opts, from_json, errp)) {
 return true;
 } else if (*errp) {
 return false;
@@ -958,7 +959,7 @@ static void device_finalize(Object *obj)
 dev->canonical_path = NULL;
 }
 
-qemu_opts_del(dev->opts);
+qobject_unref(dev->opts);
 g_free(dev->id);
 }
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f503f28c00..09e173a558 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,9 +858,11 @@ static void failover_add_primary(VirtIONet *n, Error 
**errp)
 return;
 }
 
-dev = qdev_device_add(n->primary_opts, );
+dev = qdev_device_add_from_qdict(n->primary_opts,
+ n->primary_opts_from_json,
+  

[PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally

[Kevin Wolf]

Instead of accessing the global QemuOptsList, which really belong to the
command line parser and shouldn't be accessed from devices, store a
pointer to the QemuOpts in a new VirtIONet field.

This is not the final state, but just an intermediate step to get rid of
QemuOpts in devices. It will later be replaced with an options QDict.

Before this patch, two "primary" devices could be hidden for the same
standby device, but only one of them would actually be enabled and the
other one would be kept hidden forever, so this doesn't make sense.
After this patch, configuring a second primary device is an error.

Signed-off-by: Kevin Wolf 
---
 include/hw/virtio/virtio-net.h |  1 +
 hw/net/virtio-net.c| 26 ++
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 824a69c23f..d118c95f69 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,6 +209,7 @@ struct VirtIONet {
 bool failover_primary_hidden;
 bool failover;
 DeviceListener primary_listener;
+QemuOpts *primary_opts;
 Notifier migration_state;
 VirtioNetRssData rss_data;
 struct NetRxPkt *rx_pkt;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a17d5739fc..ed9a9012e9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,27 +858,24 @@ static DeviceState 
*failover_find_primary_device(VirtIONet *n)
 static void failover_add_primary(VirtIONet *n, Error **errp)
 {
 Error *err = NULL;
-QemuOpts *opts;
-char *id;
 DeviceState *dev = failover_find_primary_device(n);
 
 if (dev) {
 return;
 }
 
-id = failover_find_primary_device_id(n);
-if (!id) {
+if (!n->primary_opts) {
 error_setg(errp, "Primary device not found");
 error_append_hint(errp, "Virtio-net failover will not work. Make "
   "sure primary device has parameter"
   " failover_pair_id=%s\n", n->netclient_name);
 return;
 }
-opts = qemu_opts_find(qemu_find_opts("device"), id);
-g_assert(opts); /* cannot be NULL because id was found using opts list */
-dev = qdev_device_add(opts, );
+
+dev = qdev_device_add(n->primary_opts, );
 if (err) {
-qemu_opts_del(opts);
+qemu_opts_del(n->primary_opts);
+n->primary_opts = NULL;
 } else {
 object_unref(OBJECT(dev));
 }
@@ -3317,6 +3314,19 @@ static bool failover_hide_primary_device(DeviceListener 
*listener,
 return false;
 }
 
+if (n->primary_opts) {
+error_setg(errp, "Cannot attach more than one primary device to '%s'",
+   n->netclient_name);
+return false;
+}
+
+/*
+ * Having a weak reference here should be okay because a device can't be
+ * deleted while it's hidden. This will be replaced soon with a QDict that
+ * has a clearer ownership model.
+ */
+n->primary_opts = device_opts;
+
 /* failover_primary_hidden is set during feature negotiation */
 return qatomic_read(>failover_primary_hidden);
 }
-- 
2.31.1

[PATCH v2 13/15] virtio-net: Avoid QemuOpts in failover_find_primary_device()

[Kevin Wolf]

Don't go through the global QemuOptsList, it is state of the legacy
command line parser and we will create devices that are not contained
in it. It is also just the command line configuration and not
necessarily the current runtime state.

Instead, look at the qdev device tree which has the current state of all
existing devices.

Signed-off-by: Kevin Wolf 
---
 hw/net/virtio-net.c | 52 +
 1 file changed, 19 insertions(+), 33 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index ed9a9012e9..f503f28c00 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -796,48 +796,34 @@ static inline uint64_t 
virtio_net_supported_guest_offloads(VirtIONet *n)
 
 typedef struct {
 VirtIONet *n;
-char *id;
-} FailoverId;
+DeviceState *dev;
+} FailoverDevice;
 
 /**
- * Set the id of the failover primary device
+ * Set the failover primary device
  *
  * @opaque: FailoverId to setup
  * @opts: opts for device we are handling
  * @errp: returns an error if this function fails
  */
-static int failover_set_primary(void *opaque, QemuOpts *opts, Error **errp)
+static int failover_set_primary(DeviceState *dev, void *opaque)
 {
-FailoverId *fid = opaque;
-const char *standby_id = qemu_opt_get(opts, "failover_pair_id");
+FailoverDevice *fdev = opaque;
+PCIDevice *pci_dev = (PCIDevice *)
+object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE);
 
-if (g_strcmp0(standby_id, fid->n->netclient_name) == 0) {
-fid->id = g_strdup(opts->id);
+if (!pci_dev) {
+return 0;
+}
+
+if (!g_strcmp0(pci_dev->failover_pair_id, fdev->n->netclient_name)) {
+fdev->dev = dev;
 return 1;
 }
 
 return 0;
 }
 
-/**
- * Find the primary device id for this failover virtio-net
- *
- * @n: VirtIONet device
- * @errp: returns an error if this function fails
- */
-static char *failover_find_primary_device_id(VirtIONet *n)
-{
-Error *err = NULL;
-FailoverId fid;
-
-fid.n = n;
-if (!qemu_opts_foreach(qemu_find_opts("device"),
-   failover_set_primary, , )) {
-return NULL;
-}
-return fid.id;
-}
-
 /**
  * Find the primary device for this failover virtio-net
  *
@@ -846,13 +832,13 @@ static char *failover_find_primary_device_id(VirtIONet *n)
  */
 static DeviceState *failover_find_primary_device(VirtIONet *n)
 {
-char *id = failover_find_primary_device_id(n);
-
-if (!id) {
-return NULL;
-}
+FailoverDevice fdev = {
+.n = n,
+};
 
-return qdev_find_recursive(sysbus_get_default(), id);
+qbus_walk_children(sysbus_get_default(), failover_set_primary, NULL,
+   NULL, NULL, );
+return fdev.dev;
 }
 
 static void failover_add_primary(VirtIONet *n, Error **errp)
-- 
2.31.1

[PATCH v2 11/15] qdev: Add Error parameter to hide_device() callbacks

[Kevin Wolf]

hide_device() is used for virtio-net failover, where the standby virtio
device delays creation of the primary device. It only makes sense to
have a single primary device for each standby device. Adding a second
one should result in an error instead of hiding it and never using it
afterwards.

Prepare for this by adding an Error parameter to the hide_device()
callback where virtio-net is informed about adding a primary device.

Signed-off-by: Kevin Wolf 
---
 include/hw/qdev-core.h | 8 ++--
 hw/core/qdev.c | 7 +--
 hw/net/virtio-net.c| 2 +-
 softmmu/qdev-monitor.c | 5 -
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5a073fc368..74d8b614a7 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -201,8 +201,12 @@ struct DeviceListener {
  * informs qdev if a device should be visible or hidden.  We can
  * hide a failover device depending for example on the device
  * opts.
+ *
+ * On errors, it returns false and errp is set. Device creation
+ * should fail in this case.
  */
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts);
+bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
+Error **errp);
 QTAILQ_ENTRY(DeviceListener) link;
 };
 
@@ -837,7 +841,7 @@ void device_listener_unregister(DeviceListener *listener);
  * When a device is added via qdev_device_add() this will be called,
  * and return if the device should be added now or not.
  */
-bool qdev_should_hide_device(QemuOpts *opts);
+bool qdev_should_hide_device(QemuOpts *opts, Error **errp);
 
 typedef enum MachineInitPhase {
 /* current_machine is NULL.  */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d918b50a1d..c3a021c444 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -211,14 +211,17 @@ void device_listener_unregister(DeviceListener *listener)
 QTAILQ_REMOVE(_listeners, listener, link);
 }
 
-bool qdev_should_hide_device(QemuOpts *opts)
+bool qdev_should_hide_device(QemuOpts *opts, Error **errp)
 {
+ERRP_GUARD();
 DeviceListener *listener;
 
 QTAILQ_FOREACH(listener, _listeners, link) {
 if (listener->hide_device) {
-if (listener->hide_device(listener, opts)) {
+if (listener->hide_device(listener, opts, errp)) {
 return true;
+} else if (*errp) {
+return false;
 }
 }
 }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f205331dcf..a17d5739fc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3304,7 +3304,7 @@ static void virtio_net_migration_state_notifier(Notifier 
*notifier, void *data)
 }
 
 static bool failover_hide_primary_device(DeviceListener *listener,
- QemuOpts *device_opts)
+ QemuOpts *device_opts, Error **errp)
 {
 VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
 const char *standby_id;
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index feb15818e6..ccc3c11563 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -625,6 +625,7 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error 
**errp)
 
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 {
+ERRP_GUARD();
 DeviceClass *dc;
 const char *driver, *path;
 DeviceState *dev = NULL;
@@ -668,11 +669,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 error_setg(errp, "Device with failover_pair_id don't have id");
 return NULL;
 }
-if (qdev_should_hide_device(opts)) {
+if (qdev_should_hide_device(opts, errp)) {
 if (bus && !qbus_is_hotpluggable(bus)) {
 error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
 }
 return NULL;
+} else if (*errp) {
+return NULL;
 }
 }
 
-- 
2.31.1

[PATCH v2 04/15] qom: Reduce use of error_propagate()

[Kevin Wolf]

ERRP_GUARD() makes debugging easier by making sure that _abort
still fails at the real origin of the error instead of
error_propagate().

Signed-off-by: Kevin Wolf 
---
 qom/object.c|  7 +++
 qom/object_interfaces.c | 19 ++-
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index e86cb05b84..6be710bc40 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name, 
Visitor *v,
 bool object_property_set(Object *obj, const char *name, Visitor *v,
  Error **errp)
 {
-Error *err = NULL;
+ERRP_GUARD();
 ObjectProperty *prop = object_property_find_err(obj, name, errp);
 
 if (prop == NULL) {
@@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name, 
Visitor *v,
 error_setg(errp, QERR_PERMISSION_DENIED);
 return false;
 }
-prop->set(obj, v, name, prop->opaque, );
-error_propagate(errp, err);
-return !err;
+prop->set(obj, v, name, prop->opaque, errp);
+return !*errp;
 }
 
 bool object_property_set_str(Object *obj, const char *name,
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ad9b56b59a..3b61c195c5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -46,25 +46,18 @@ static void object_set_properties_from_qdict(Object *obj, 
const QDict *qdict,
  Visitor *v, Error **errp)
 {
 const QDictEntry *e;
-Error *local_err = NULL;
 
-if (!visit_start_struct(v, NULL, NULL, 0, _err)) {
-goto out;
+if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+return;
 }
 for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-if (!object_property_set(obj, e->key, v, _err)) {
-break;
+if (!object_property_set(obj, e->key, v, errp)) {
+goto out;
 }
 }
-if (!local_err) {
-visit_check_struct(v, _err);
-}
-visit_end_struct(v, NULL);
-
+visit_check_struct(v, errp);
 out:
-if (local_err) {
-error_propagate(errp, local_err);
-}
+visit_end_struct(v, NULL);
 }
 
 void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
-- 
2.31.1

[PATCH v2 05/15] iotests/245: Fix type for iothread property

[Kevin Wolf]

iothread is a string property, so None (= JSON null) is not a valid
value for it. Pass the empty string instead to get the default iothread.

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/245 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bf8261eec0..9b12b42eed 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1189,10 +1189,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.run_test_iothreads('iothread0', 'iothread0')
 
 def test_iothreads_switch_backing(self):
-self.run_test_iothreads('iothread0', None)
+self.run_test_iothreads('iothread0', '')
 
 def test_iothreads_switch_overlay(self):
-self.run_test_iothreads(None, 'iothread0')
+self.run_test_iothreads('', 'iothread0')
 
 if __name__ == '__main__':
 iotests.activate_logging()
-- 
2.31.1

[PATCH v2 08/15] qdev: Make DeviceState.id independent of QemuOpts

[Kevin Wolf]

DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 include/hw/qdev-core.h  | 2 +-
 include/monitor/qdev.h  | 2 +-
 hw/arm/virt.c   | 2 +-
 hw/core/qdev.c  | 1 +
 hw/pci-bridge/pci_expander_bridge.c | 2 +-
 hw/ppc/e500.c   | 2 +-
 softmmu/qdev-monitor.c  | 5 +++--
 7 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4ff19c714b..5a073fc368 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
 Object parent_obj;
 /*< public >*/
 
-const char *id;
+char *id;
 char *canonical_path;
 bool realized;
 bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
 
 #endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7170aaacd5..4160d49688 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
 MemoryRegion *sysmem = get_system_memory();
 
 dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
 qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
 qdev_prop_set_uint32(dev, "mmio_size", 
vms->memmap[VIRT_PLATFORM_BUS].size);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
 }
 
 qemu_opts_del(dev->opts);
+g_free(dev->id);
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c 
b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool 
pcie, Error **errp)
 } else {
 bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, 
TYPE_PXB_BUS);
 bds = qdev_new("pci-bridge");
-bds->id = dev_name;
+bds->id = g_strdup(dev_name);
 qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
 qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
 }
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
 /* Platform Bus Device */
 if (pmc->has_platform_bus) {
 dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
 qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
 qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
 return bus;
 }
 
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
 {
 if (id) {
 dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 }
 }
 
-qdev_set_id(dev, qemu_opts_id(opts));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
 
 /* set properties */
 if (qemu_opt_foreach(opts, set_property, dev, errp)) {
-- 
2.31.1

[PATCH v2 06/15] iotests/051: Fix typo

[Kevin Wolf]

The iothread isn't called 'iothread0', but 'thread0'. Depending on the
order that properties are parsed, the error message may change from the
expected one to another one saying that the iothread doesn't exist.

Signed-off-by: Kevin Wolf 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/051| 2 +-
 tests/qemu-iotests/051.pc.out | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 7bf29343d7..1d2fa93a11 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -199,7 +199,7 @@ case "$QEMU_DEFAULT_MACHINE" in
 # virtio-blk enables the iothread only when the driver initialises the
 # device, so a second virtio-blk device can't be added even with the
 # same iothread. virtio-scsi allows this.
-run_qemu $iothread -device 
virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on
+run_qemu $iothread -device 
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
 run_qemu $iothread -device 
virtio-scsi,id=virtio-scsi1,iothread=thread0 -device 
scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
 ;;
  *)
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index afe7632964..063e4fc584 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -183,9 +183,9 @@ Testing: -drive 
file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) QEMU_PROG: -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on: 
Cannot change iothread of active block backend
 
-Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object 
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device 
virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on
+Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object 
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device 
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device 
virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on: Cannot change 
iothread of active block backend
+(qemu) QEMU_PROG: -device 
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread 
of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object 
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device 
virtio-scsi,id=virtio-scsi1,iothread=thread0 -device 
scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-- 
2.31.1

[PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id

[Kevin Wolf]

From: Damien Hedde 

qdev_set_id() is mostly used when the user adds a device (using
-device cli option or device_add qmp command). This commit adds
an error parameter to handle the case where the given id is
already taken.

Also document the function and add a return value in order to
be able to capture success/failure: the function now returns the
id in case of success, or NULL in case of failure.

The commit modifies the 2 calling places (qdev-monitor and
xen-legacy-backend) to add the error object parameter.

Note that the id is, right now, guaranteed to be unique because
all ids came from the "device" QemuOptsList where the id is used
as key. This addition is a preparation for a future commit which
will relax the uniqueness.

Signed-off-by: Damien Hedde 
Signed-off-by: Kevin Wolf 
---
 include/monitor/qdev.h  | 25 +++-
 hw/xen/xen-legacy-backend.c |  3 ++-
 softmmu/qdev-monitor.c  | 38 +++--
 3 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..74e6c55a2b 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,29 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+
+/**
+ * qdev_set_id: parent the device and set its id if provided.
+ * @dev: device to handle
+ * @id: id to be given to the device, or NULL.
+ *
+ * Returns: the id of the device in case of success; otherwise NULL.
+ *
+ * @dev must be unrealized, unparented and must not have an id.
+ *
+ * If @id is non-NULL, this function tries to setup @dev qom path as
+ * "/peripheral/id". If @id is already taken, it fails. If it succeeds,
+ * the id field of @dev is set to @id (@dev now owns the given @id
+ * parameter).
+ *
+ * If @id is NULL, this function generates a unique name and setups @dev
+ * qom path as "/peripheral-anon/name". This name is not set as the id
+ * of @dev.
+ *
+ * Upon success, it returns the id/name (generated or provided). The
+ * returned string is owned by the corresponding child property and must
+ * not be freed by the caller.
+ */
+const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
 #endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index be3cf4a195..085fd31ef7 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char 
*type, int dom,
 xendev = g_malloc0(ops->size);
 object_initialize(>qdev, ops->size, TYPE_XENBACKEND);
 OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+_fatal);
 qdev_realize(DEVICE(xendev), xen_sysbus, _fatal);
 object_unref(OBJECT(xendev));
 
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..feb15818e6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,22 +593,34 @@ static BusState *qbus_find(const char *path, Error **errp)
 }
 
 /* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
 {
-if (id) {
-dev->id = id;
-}
+ObjectProperty *prop;
 
-if (dev->id) {
-object_property_add_child(qdev_get_peripheral(), dev->id,
-  OBJECT(dev));
+assert(!dev->id && !dev->realized);
+
+/*
+ * object_property_[try_]add_child() below will assert the device
+ * has no parent
+ */
+if (id) {
+prop = object_property_try_add_child(qdev_get_peripheral(), id,
+ OBJECT(dev), NULL);
+if (prop) {
+dev->id = id;
+} else {
+error_setg(errp, "Duplicate device ID '%s'", id);
+return NULL;
+}
 } else {
 static int anon_count;
 gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
-  OBJECT(dev));
+prop = object_property_add_child(qdev_get_peripheral_anon(), name,
+ OBJECT(dev));
 g_free(name);
 }
+
+return prop->name;
 }
 
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
@@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 }
 }
 
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+/*
+ * set dev's parent and register its id.
+ * If it fails it means the id is already taken.
+ */
+if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
+goto err_del_dev;
+}
 
   

[PATCH v2 01/15] net: Introduce NetClientInfo.check_peer_type()

[Kevin Wolf]

Some network backends (vhost-user and vhost-vdpa) work only with
specific devices. At startup, they second guess what the command line
option handling will do and error out if they think a non-virtio device
will attach to them.

This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.

Add a callback where backends can check compatibility with a device when
it actually tries to attach, even on hotplug.

Signed-off-by: Kevin Wolf 
---
 include/net/net.h| 2 ++
 hw/core/qdev-properties-system.c | 6 ++
 2 files changed, 8 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 5d1508081f..986288eb07 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -62,6 +62,7 @@ typedef struct SocketReadState SocketReadState;
 typedef void (SocketReadStateFinalize)(SocketReadState *rs);
 typedef void (NetAnnounce)(NetClientState *);
 typedef bool (SetSteeringEBPF)(NetClientState *, int);
+typedef bool (NetCheckPeerType)(NetClientState *, ObjectClass *, Error **);
 
 typedef struct NetClientInfo {
 NetClientDriver type;
@@ -84,6 +85,7 @@ typedef struct NetClientInfo {
 SetVnetBE *set_vnet_be;
 NetAnnounce *announce;
 SetSteeringEBPF *set_steering_ebpf;
+NetCheckPeerType *check_peer_type;
 } NetClientInfo;
 
 struct NetClientState {
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index e71f5d64d1..a91f60567a 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -431,6 +431,12 @@ static void set_netdev(Object *obj, Visitor *v, const char 
*name,
 goto out;
 }
 
+if (peers[i]->info->check_peer_type) {
+if (!peers[i]->info->check_peer_type(peers[i], obj->class, errp)) {
+goto out;
+}
+}
+
 ncs[i] = peers[i];
 ncs[i]->queue_index = i;
 }
-- 
2.31.1

[PATCH v2 02/15] net/vhost-user: Fix device compatibility check

[Kevin Wolf]

vhost-user works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.

This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.

Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.

Signed-off-by: Kevin Wolf 
---
 net/vhost-user.c | 41 ++---
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 4a939124d2..b1a0247b59 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -198,6 +198,19 @@ static bool vhost_user_has_ufo(NetClientState *nc)
 return true;
 }
 
+static bool vhost_user_check_peer_type(NetClientState *nc, ObjectClass *oc,
+   Error **errp)
+{
+const char *driver = object_class_get_name(oc);
+
+if (!g_str_has_prefix(driver, "virtio-net-")) {
+error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
+return false;
+}
+
+return true;
+}
+
 static NetClientInfo net_vhost_user_info = {
 .type = NET_CLIENT_DRIVER_VHOST_USER,
 .size = sizeof(NetVhostUserState),
@@ -207,6 +220,7 @@ static NetClientInfo net_vhost_user_info = {
 .has_ufo = vhost_user_has_ufo,
 .set_vnet_be = vhost_user_set_vnet_endianness,
 .set_vnet_le = vhost_user_set_vnet_endianness,
+.check_peer_type = vhost_user_check_peer_type,
 };
 
 static gboolean net_vhost_user_watch(void *do_not_use, GIOCondition cond,
@@ -397,27 +411,6 @@ static Chardev *net_vhost_claim_chardev(
 return chr;
 }
 
-static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
-{
-const char *name = opaque;
-const char *driver, *netdev;
-
-driver = qemu_opt_get(opts, "driver");
-netdev = qemu_opt_get(opts, "netdev");
-
-if (!driver || !netdev) {
-return 0;
-}
-
-if (strcmp(netdev, name) == 0 &&
-!g_str_has_prefix(driver, "virtio-net-")) {
-error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
-return -1;
-}
-
-return 0;
-}
-
 int net_init_vhost_user(const Netdev *netdev, const char *name,
 NetClientState *peer, Error **errp)
 {
@@ -433,12 +426,6 @@ int net_init_vhost_user(const Netdev *netdev, const char 
*name,
 return -1;
 }
 
-/* verify net frontend */
-if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
-  (char *)name, errp)) {
-return -1;
-}
-
 queues = vhost_user_opts->has_queues ? vhost_user_opts->queues : 1;
 if (queues < 1 || queues > MAX_QUEUE_NUM) {
 error_setg(errp,
-- 
2.31.1

[PATCH v2 03/15] net/vhost-vdpa: Fix device compatibility check

[Kevin Wolf]

vhost-vdpa works only with specific devices. At startup, it second
guesses what the command line option handling will do and error out if
it thinks a non-virtio device will attach to them.

This second guessing is not only ugly, it can lead to wrong error
messages ('-device floppy,netdev=foo' should complain about an unknown
property, not about the wrong kind of network device being attached) and
completely ignores hotplugging.

Drop the old checks and implement .check_peer_type() instead to fix
this. As a nice side effect, it also removes one more dependency on the
legacy QemuOpts infrastructure and even reduces the code size.

Signed-off-by: Kevin Wolf 
---
 net/vhost-vdpa.c | 37 ++---
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 912686457c..6dc68d8677 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -147,12 +147,26 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
 
 }
 
+static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc,
+   Error **errp)
+{
+const char *driver = object_class_get_name(oc);
+
+if (!g_str_has_prefix(driver, "virtio-net-")) {
+error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
+return false;
+}
+
+return true;
+}
+
 static NetClientInfo net_vhost_vdpa_info = {
 .type = NET_CLIENT_DRIVER_VHOST_VDPA,
 .size = sizeof(VhostVDPAState),
 .cleanup = vhost_vdpa_cleanup,
 .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
 .has_ufo = vhost_vdpa_has_ufo,
+.check_peer_type = vhost_vdpa_check_peer_type,
 };
 
 static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
@@ -179,24 +193,6 @@ static int net_vhost_vdpa_init(NetClientState *peer, const 
char *device,
 return ret;
 }
 
-static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
-{
-const char *name = opaque;
-const char *driver, *netdev;
-
-driver = qemu_opt_get(opts, "driver");
-netdev = qemu_opt_get(opts, "netdev");
-if (!driver || !netdev) {
-return 0;
-}
-if (strcmp(netdev, name) == 0 &&
-!g_str_has_prefix(driver, "virtio-net-")) {
-error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
-return -1;
-}
-return 0;
-}
-
 int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
 NetClientState *peer, Error **errp)
 {
@@ -204,10 +200,5 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char 
*name,
 
 assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 opts = >u.vhost_vdpa;
-/* verify net frontend */
-if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
-  (char *)name, errp)) {
-return -1;
-}
 return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
 }
-- 
2.31.1

[PATCH v2 00/15] qdev: Add JSON -device

[Kevin Wolf]

It's still a long way until we'll have QAPIfied devices, but there are
some improvements that we can already make now to make the future switch
easier.

One important part of this is having code paths without QemuOpts, which
we want to get rid of and replace with the keyval parser in the long
run. This series adds support for JSON syntax to -device, which bypasses
QemuOpts.

While we're not using QAPI yet, devices are based on QOM, so we already
do have type checks and an implied schema. JSON syntax supported now can
be supported by QAPI later and regarding command line compatibility,
actually switching to it becomes an implementation detail this way (of
course, it will still add valuable user-visible features like
introspection and documentation).

Apart from making things more future proof, this also immediately adds
a way to do non-scalar properties on the command line. nvme could have
used list support recently, and the lack of it in -device led to some
rather unnatural solution in the first version (doing the relationship
between a device and objects backwards) and loss of features in the
following. With this series, using a list as a device property should be
possible without any weird tricks.

Unfortunately, even QMP device_add goes through QemuOpts before this
series, which destroys any type safety QOM provides and also can't
support non-scalar properties. This is a bug, but it turns out that
libvirt actually relies on it and passes only strings for everything.
So this series still leaves device_add alone until libvirt is fixed.

v2:
- Drop type safe QMP device_add because libvirt gets it wrong, too
- More network patches to eliminate dependencies on the legacy QemuOpts
  data structures which would not contain all devices any more after
  this series. Fix some bugs there as a side effect.
- Remove an unnecessary use of ERRP_GUARD()
- Replaced error handling patch for qdev_set_id() with Damien's
- Drop the deprecation patch until libvirt uses the new JSON syntax

Damien Hedde (1):
  softmmu/qdev-monitor: add error handling in qdev_set_id

Kevin Wolf (14):
  net: Introduce NetClientInfo.check_peer_type()
  net/vhost-user: Fix device compatibility check
  net/vhost-vdpa: Fix device compatibility check
  qom: Reduce use of error_propagate()
  iotests/245: Fix type for iothread property
  iotests/051: Fix typo
  qdev: Avoid using string visitor for properties
  qdev: Make DeviceState.id independent of QemuOpts
  qemu-option: Allow deleting opts during qemu_opts_foreach()
  qdev: Add Error parameter to hide_device() callbacks
  virtio-net: Store failover primary opts pointer locally
  virtio-net: Avoid QemuOpts in failover_find_primary_device()
  qdev: Base object creation on QDict rather than QemuOpts
  vl: Enable JSON syntax for -device

 qapi/qdev.json  | 15 +++--
 include/hw/qdev-core.h  | 15 +++--
 include/hw/virtio/virtio-net.h  |  2 +
 include/monitor/qdev.h  | 27 +++-
 include/net/net.h   |  2 +
 hw/arm/virt.c   |  2 +-
 hw/core/qdev-properties-system.c|  6 ++
 hw/core/qdev.c  | 11 +++-
 hw/net/virtio-net.c | 85 -
 hw/pci-bridge/pci_expander_bridge.c |  2 +-
 hw/ppc/e500.c   |  2 +-
 hw/vfio/pci.c   |  4 +-
 hw/xen/xen-legacy-backend.c |  3 +-
 net/vhost-user.c| 41 
 net/vhost-vdpa.c| 37 ---
 qom/object.c|  7 +-
 qom/object_interfaces.c | 19 ++
 softmmu/qdev-monitor.c  | 99 +++--
 softmmu/vl.c| 63 --
 util/qemu-option.c  |  4 +-
 tests/qemu-iotests/051  |  2 +-
 tests/qemu-iotests/051.pc.out   |  4 +-
 tests/qemu-iotests/245  |  4 +-
 23 files changed, 278 insertions(+), 178 deletions(-)

-- 
2.31.1

Re: [PATCH 064/103] qemuBuildHostdevMediatedDevProps: Move 'ramfb' and 'bootindex' before the address

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Simplify the generator by moving few properties earlier.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c   | 8 ++--
.../hostdev-mdev-display-ramfb.x86_64-latest.args | 2 +-
.../hostdev-subsys-mdev-vfio-ccw-boot.s390x-latest.args   | 2 +-
3 files changed, 4 insertions(+), 8 deletions(-)


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 063/103] qemuBuildHostdevCommandLine: Build mediated device commandline via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The 'vfio-pci-nohotplug' device has the following property types
according to QEMU:

 display=- on/off/auto (default: "off")
 sysfsdev=
 ramfb=
 bootindex=

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 48 ++---
src/qemu/qemu_command.h |  7 +++---
src/qemu/qemu_hotplug.c |  7 +++---
3 files changed, 27 insertions(+), 35 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH v3 5/6] qemu: remove use of implicit boolean syntax for guest features

[Jiri Denemark]

On Fri, Oct 08, 2021 at 10:01:44 +0100, Daniel P. Berrangé wrote:
> Some guest features that map to the -cpu arg are still added using
> implicit syntax "feature" which is a deprecated shorthand for
> "feature=on".
> 
> Reviewed-by: Peter Krempa 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_command.c | 6 +++---
>  tests/qemuxml2argvdata/clock-timer-hyperv-rtc.args  | 2 +-
>  .../hyperv-stimer-direct.x86_64-latest.args | 2 +-
>  tests/qemuxml2argvdata/hyperv.x86_64-4.0.0.args | 2 +-
>  tests/qemuxml2argvdata/hyperv.x86_64-latest.args| 2 +-
>  5 files changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Jiri Denemark 

[PATCH] qemu: tpm: Run swtpm_setup --create-config-files in session mode

[Stefan Berger]

Using swtpm v0.7.0 we can run swtpm_setup to create default config files
for swtpm_setup and swtpm-localca in session mode. Now a user can start
a VM with an attached TPM without having to run this program on the
command line before. This program needs to run once.

This patch addresses the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2010649

Sigbed-off-by: Stefan Berger 
---
 src/qemu/qemu_tpm.c | 43 +++
 src/util/virtpm.c   |  1 +
 src/util/virtpm.h   |  1 +
 3 files changed, 45 insertions(+)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 100481503c..1b504bce67 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -385,6 +385,46 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
 return virCommandSetSendBuffer(cmd, g_steal_pointer(), secret_len);
 }
 
+
+/*
+ * qemuTPMCreateConfigFiles: run swtpm_setup --create-config-files 
skip-if-exist
+ *
+ * @logfile: The file to write the log into; it must be writable
+ *   for the user given by userid or 'tss'
+ */
+static int
+qemuTPMCreateConfigFiles(const char *logfile)
+{
+g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
+g_autoptr(virCommand) cmd = NULL;
+int exitstatus;
+
+if (!swtpm_setup)
+return -1;
+
+if (!virTPMSwtpmSetupCapsGet(
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES))
+return -1;
+
+cmd = virCommandNew(swtpm_setup);
+if (!cmd)
+return -1;
+
+virCommandAddArgList(cmd, "--create-config-files", "skip-if-exist", NULL);
+virCommandClearCaps(cmd);
+
+if (virCommandRun(cmd, ) < 0 || exitstatus != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not run '%s' to create config files. 
exitstatus: %d; "
+ "Check error log '%s' for details."),
+  swtpm_setup, exitstatus, logfile);
+return -1;
+}
+
+return 0;
+}
+
+
 /*
  * qemuTPMEmulatorRunSetup
  *
@@ -432,6 +472,9 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
  "this requires privileged mode for a "
  "TPM 1.2\n"), 0600);
 
+if (!privileged && qemuTPMCreateConfigFiles(logfile))
+return -1;
+
 cmd = virCommandNew(swtpm_setup);
 if (!cmd)
 return -1;
diff --git a/src/util/virtpm.c b/src/util/virtpm.c
index 1a567139b4..0f50de866c 100644
--- a/src/util/virtpm.c
+++ b/src/util/virtpm.c
@@ -45,6 +45,7 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature,
 VIR_ENUM_IMPL(virTPMSwtpmSetupFeature,
   VIR_TPM_SWTPM_SETUP_FEATURE_LAST,
   "cmdarg-pwdfile-fd",
+  "cmdarg-create-config-files",
 );
 
 /**
diff --git a/src/util/virtpm.h b/src/util/virtpm.h
index d021a083b4..3bb03b3b33 100644
--- a/src/util/virtpm.h
+++ b/src/util/virtpm.h
@@ -38,6 +38,7 @@ typedef enum {
 
 typedef enum {
 VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD,
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES,
 
 VIR_TPM_SWTPM_SETUP_FEATURE_LAST
 } virTPMSwtpmSetupFeature;
-- 
2.31.1

Re: [PATCH 062/103] qemuBuildRNGCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The 'virtio-rng' has the following property types according to QEMU:
 rng=>
 max-bytes= -  (default: 9223372036854775807)
 period=-  (default: 65536)

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 46 +++--
src/qemu/qemu_command.h |  7 ---
src/qemu/qemu_hotplug.c |  6 +++---
3 files changed, 33 insertions(+), 26 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH v3 4/6] qemu: inline the qemuBuildCpuFeature code

[Jiri Denemark]

On Fri, Oct 08, 2021 at 10:01:43 +0100, Daniel P. Berrangé wrote:
> With the previous refactorings, there's no real benefit from the
> qemuBuildCpuFeature helper method. Only one of the callers really
> needs the CPU feature name re-writing logic, the others can just
> use the right name directly.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_command.c | 31 +++
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 92125dbc85..f24c8842aa 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6292,18 +6292,6 @@ qemuBuildGlobalControllerCommandLine(virCommand *cmd,
>  }
>  
>  
> -static void
> -qemuBuildCpuFeature(virQEMUCaps *qemuCaps,
> -virBuffer *buf,
> -const char *name,
> -bool state)
> -{
> -name = virQEMUCapsCPUFeatureToQEMU(qemuCaps, name);
> -
> -virBufferAsprintf(buf, ",%s=%s", name, state ? "on" : "off");
> -}
> -
> -
>  static int
>  qemuBuildCpuModelArgStr(virQEMUDriver *driver,
>  const virDomainDef *def,
> @@ -6376,15 +6364,17 @@ qemuBuildCpuModelArgStr(virQEMUDriver *driver,
>  virBufferAsprintf(buf, ",vendor=%s", cpu->vendor_id);
>  
>  for (i = 0; i < cpu->nfeatures; i++) {
> +const char *featname = virQEMUCapsCPUFeatureToQEMU(
> +qemuCaps, cpu->features[i].name);

That's not really the kind of formatting we use in libvirt. This looks
more like go :-)

Either
   const char *featname = virQEMUCapsCPUFeatureToQEMU(qemuCaps,
  
cpu->features[i].name);

or just put the all on a single line.

Anyway, nicely separated patch which makes it obvious removing the
function did not have unexpected side effects.

Reviewed-by: Jiri Denemark 

Re: [PATCH 061/103] qemuBuildMemballoonCommandLine: Reorder properties

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Move the 'deflate-on-oom' and 'free-page-reporting' before the address
to simplify the genrator code.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c   | 8 ++--
tests/qemuxml2argvdata/balloon-ccw-deflate.args   | 2 +-
tests/qemuxml2argvdata/balloon-device-deflate-off.args| 2 +-
tests/qemuxml2argvdata/balloon-device-deflate.args| 2 +-
...tions-memballoon-freepage-reporting.x86_64-latest.args | 2 +-
5 files changed, 6 insertions(+), 10 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] vireventglib: Remove handles with the highest priority

[Daniel P . Berrangé]

On Fri, Oct 08, 2021 at 03:13:27PM +0200, Michal Prívozník wrote:
> On 10/8/21 2:23 PM, Daniel P. Berrangé wrote:
> > On Fri, Oct 08, 2021 at 01:56:24PM +0200, Michal Prívozník wrote:
> >> On 10/8/21 1:36 PM, Daniel P. Berrangé wrote:
> > So, ACK to this patch as a quick fix, but it feels like we
> > ultimately have a more serious problem.
> > 
> > I wonder if we should temporarily stop watching events on
> > the incoming server if we get EMFILE first time on accept,
> > and set a timer to re-enable events 60 seconds later. Rinse,
> > repeat.
> 
> Yes, good idea. But I worry that 60 seconds may be too long. Or
> needlessly short - depending on usage. Nevertheless, let us start with
> 60 seconds and we can fine tune later.

Once we're in this out of FDs situation, we're in a bad mess no
matter what, so the length of timeout isn't a big issue IMHO.

Essentially we just need to stop burning CPU gratuitously.
Even 5 seconds is long enough to achieve that goal, so just
pick a number you're happy with - I don't feel strongly about
it being 60 seconds, it was just an arbitrary number i picked.

> So are you okay with me pushing this patch and sending this ^^ as a
> follow up?

Yes, consider is Reviewed-by: Daniel P. Berrangé 


> 
> > 
> > 
>   * Each event source is assigned a priority. The default priority,
>   * %G_PRIORITY_DEFAULT, is 0. Values less than 0 denote higher 
>  priorities.
>   * Values greater than 0 denote lower priorities. Events from high 
>  priority
>   * sources are always processed before events from lower priority 
>  sources.
> 
>  and per g_idle_add() documentation:
> 
>   * Adds a function to be called whenever there are no higher priority
>   * events pending to the default main loop. The function is given the
>   * default idle priority, %G_PRIORITY_DEFAULT_IDLE.
> 
>  Now, because we did not accept() the client we are constantly
>  seeing POLLIN on the main socket and thus the removal of the
>  client socket won't ever happen.
> 
>  The fix is to set at least the same priority as other sources,
>  but since we want to just close an FD, let's give it the highest
>  priority and call it before handling other events.
> 
>  This issue can be easily reproduced, for instance:
> 
>   # ulimit -S -n 40 (tweak this number if needed)
>   # ./src/libvirtd
> 
>  from another terminal:
> 
>   # for ((i=0; i<100; i++)); do virsh list & done; virsh list
> 
>  The last `virsh list` must not get stuck.
> >>>
> >>> The bug below describes libvirt core dumps rather than
> >>> hangs. Do you know why it dumped ?
> >>
> >> Because of glib. We called some glib function which internally wanted to
> >> open a pipe which failed. Thus glib aborted. I don't remember the exact
> >> function, but I can look it up if you want.
> > 
> > Oh, so this patch is not actually fixing the core dump scenario
> > then.  Even after we fix the bug that prevents clients disconnecting,
> > it will still dump if this API is called at a point where we are in
> > a EMFILE state ?
> 
> Yes. But I am afraid there's no way around it. Found the stack trace:
> 
> Stack trace of thread 52231:
> #0  0x03ffa90be9e4 raise (libc.so.6)
> #1  0x03ffa96590ee g_log_default_handler (libglib-2.0.so.0)
> #2  0x03ffa965935e g_logv (libglib-2.0.so.0)
> #3  0x03ffa965955a g_log (libglib-2.0.so.0)
> #4  0x03ffa969dd70 g_wakeup_new (libglib-2.0.so.0)
> #5  0x03ffa964eb38 g_main_context_new (libglib-2.0.so.0)
> #6  0x03ffa9946886 vir_event_thread_init (libvirt.so.0)
> #7  0x03ffa95b5ff8 g_type_create_instance (libgobject-2.0.so.0)
> #8  0x03ffa95978cc g_object_new_internal (libgobject-2.0.so.0)
> #9  0x03ffa95990de g_object_new_with_properties (libgobject-2.0.so.0)
> #10 0x03ffa9599ab6 g_object_new (libgobject-2.0.so.0)
> #11 0x03ffa9946b78 virEventThreadNew (libvirt.so.0)
> #12 0x03ff6c2c8bd8 qemuProcessQMPNew (libvirt_driver_qemu.so)
> #13 0x03ff6c1f57b8 virQEMUCapsInitQMPSingle (libvirt_driver_qemu.so)
> #14 0x03ff6c1f59ec virQEMUCapsNewForBinaryInternal
> (libvirt_driver_qemu.so)
> #15 0x03ff6c1f5c92 virQEMUCapsNewData (libvirt_driver_qemu.so)
> #16 0x03ffa99510f4 virFileCacheValidate (libvirt.so.0)
> #17 0x03ffa995133c virFileCacheLookup (libvirt.so.0)
> #18 0x03ff6c1f5e4a virQEMUCapsCacheLookup (libvirt_driver_qemu.so)
> #19 0x03ff6c1f621e virQEMUCapsCacheLookupCopy (libvirt_driver_qemu.so)
> #20 0x03ff6c2c4a44 qemuProcessInit (libvirt_driver_qemu.so)
> #21 0x03ff6c2c67f2 qemuProcessStart (libvirt_driver_qemu.so)
> #22 0x03ff6c26358c qemuDomainObjStart.constprop.55
> (libvirt_driver_qemu.so)
> #23 0x03ff6c2639c8 qemuDomainCreateWithFlags (libvirt_driver_qemu.so)
> #24 0x03ffa9b77902 virDomainCreate (libvirt.so.0)
> #25 0x02aa34ad2d52 remoteDispatchDomainCreateHelper (libvirtd)
> #26 0x03ffa9a6094a 

Re: [PATCH 060/103] qemuBuildMemballoonCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The generated properties have the following types according to QEMU:
 deflate-on-oom=  - on/off (default: false)
 free-page-reporting= - on/off (default: false)

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 33 -
1 file changed, 16 insertions(+), 17 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] vireventglib: Remove handles with the highest priority

[Michal Prívozník]

On 10/8/21 2:23 PM, Daniel P. Berrangé wrote:
> On Fri, Oct 08, 2021 at 01:56:24PM +0200, Michal Prívozník wrote:
>> On 10/8/21 1:36 PM, Daniel P. Berrangé wrote:
>>> On Fri, Oct 08, 2021 at 01:12:49PM +0200, Michal Privoznik wrote:
 When a server decides to close a client, the
 virNetServerClientCloseLocked() is called. In here various
 cleanup steps are taken, but the most important part (from this
 commit's POV at least) is the way that the socket is closed.
 Firstly, removal of the socket associated with the client from
 the event loop is signalized and then the socket is unrefed. The
 socket is not closed just yet though, because the event loop
 holds a reference to it. This reference will be freed as soon as
 the event loop wakes up and starts issuing callbacks (in this
 case virNetSocketEventFree()).

 So far, this is how things usually work. But if the daemon
 reaches the number of opened files limit, things start to work
 differently.

 If the RLIMIT_NOFILE limit is reached and there's a client that
 wants to connect then the event loop wakes up, sees POLLIN on the
 socket and calls virNetServerServiceAccept() which in turn calls
 virNetSocketAccept(). But because of the limit, accept() fails
 with EMFILE leaving the POLLIN event unhandled. The dispatch then
 continues to next FDs with events on them. BUT, it will NOT call
 the socket removal callback (virNetSocketEventFree()) because it
 has low priority (G_PRIORITY_DEFAULT_IDLE). Per glib's
 documentation:
>>>
>>> if virNetSocketAccept fails, there's no client socket to
>>> remove/close.  AFAIK, we are not removing the server
>>> socket either.
>>>
>>> So I'm not following which socket is supposedly being
>>> removed in this scenario ?
>>
>> Right. I didn't phrase it properly. Let's call the
>> /var/run/libvirt/libvirt-sock SockA and accepted socket (i.e. per
>> client) SockB.
>> Let the NOFILE limit be just the right value so that both SockA and
>> SockB can exist, but no more. Now, let's have another client trying to
>> connect(). The event loop wakes up, because there's an event on SockA.
>> But the connection can't be accept()-ed. Alright, so let the already
>> connected client disconnect (that is, SockB). This will result in:
>>
>>   g_idle_add(virEventGLibHandleRemoveIdle, SockB); /* I've made a
>> shortcut here, in reality there's a data variable that points to SockB,
>> but you get the idea */
>>
>> Now, because we still haven't handled the connection attempt at SockA
>> the idle source won't ever be called because it has lower priority.
>> Therefore, the event loop is stuck in endless loop trying to accept()
>> connection on SockA.
> 
> Ah i see, "idle" callbacks aren't called because the event loop
> isn't idle.
> 
> So with this change, we'll let the idle callback run, which will
> close the FD which will let libvird accept the client which gets
> us out of the 100% CPU burn loop. unless there were two
> pending clients
> 
> 
> So, ACK to this patch as a quick fix, but it feels like we
> ultimately have a more serious problem.
> 
> I wonder if we should temporarily stop watching events on
> the incoming server if we get EMFILE first time on accept,
> and set a timer to re-enable events 60 seconds later. Rinse,
> repeat.

Yes, good idea. But I worry that 60 seconds may be too long. Or
needlessly short - depending on usage. Nevertheless, let us start with
60 seconds and we can fine tune later.

So are you okay with me pushing this patch and sending this ^^ as a
follow up?

> 
> 
  * Each event source is assigned a priority. The default priority,
  * %G_PRIORITY_DEFAULT, is 0. Values less than 0 denote higher priorities.
  * Values greater than 0 denote lower priorities. Events from high priority
  * sources are always processed before events from lower priority sources.

 and per g_idle_add() documentation:

  * Adds a function to be called whenever there are no higher priority
  * events pending to the default main loop. The function is given the
  * default idle priority, %G_PRIORITY_DEFAULT_IDLE.

 Now, because we did not accept() the client we are constantly
 seeing POLLIN on the main socket and thus the removal of the
 client socket won't ever happen.

 The fix is to set at least the same priority as other sources,
 but since we want to just close an FD, let's give it the highest
 priority and call it before handling other events.

 This issue can be easily reproduced, for instance:

  # ulimit -S -n 40 (tweak this number if needed)
  # ./src/libvirtd

 from another terminal:

  # for ((i=0; i<100; i++)); do virsh list & done; virsh list

 The last `virsh list` must not get stuck.
>>>
>>> The bug below describes libvirt core dumps rather than
>>> hangs. Do you know why it dumped ?
>>
>> Because of glib. We called 

Re: [PATCH 059/103] qemuBuildShmemCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Note that the legacy 'ivshmem' device was already removed upstream, but
it's converted so that the code is identical.

For the two modern devices QEMU considers the properties being of
following types:

'ivshmem-doorbell'
 chardev=  - ID of a chardev to use as a backend
 ioeventfd=   - on/off (default: true)
 master= - on/off/auto (default: "off")
 vectors=   -  (default: 1)

'ivshmem-plain'
 master= - on/off/auto (default: "off")
 memdev=>

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 107 ++--
src/qemu/qemu_command.h |   7 ++-
src/qemu/qemu_hotplug.c |   6 +--
3 files changed, 64 insertions(+), 56 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 058/103] qemuBuildMemoryDeviceCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

This includes the 'pc-dimm', 'nvdimm', 'virtio-pmem-pci' and
'virtio-mem-pci' devices.

The value types according to QEMU are:

'pc-dimm'
 node=  -  (default: 0)
 memdev=>

'nvdimm'
 label-size=
 memdev=>
 node=  -  (default: 0)
 unarmed= -  (default: false)
 uuid=

'virtio-pmem-pci'
 memdev=>

'virtio-mem-pci'
 block-size=
 memdev=>
 node=  -  (default: 0)
 requested-size=

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 60 +++--
src/qemu/qemu_command.h |  7 +++--
src/qemu/qemu_hotplug.c |  6 ++---
3 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6fa804f7d8..794119a58c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3723,13 +3723,14 @@ qemuBuildMemoryDimmBackendStr(virCommand *cmd,
}


-char *
-qemuBuildMemoryDeviceStr(const virDomainDef *def,
- virDomainMemoryDef *mem,
- virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+virJSONValue *
+qemuBuildMemoryDeviceProps(const virDomainDef *def,
+   virDomainMemoryDef *mem)
{
-g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+g_autoptr(virJSONValue) props = NULL;
const char *device = NULL;
+g_autofree char *uuidstr = NULL;
+virTristateBool unarmed = VIR_TRISTATE_BOOL_ABSENT;

if (!mem->info.alias) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -3761,37 +3762,31 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
break;
}

-virBufferAsprintf(, "%s,", device);
-
-if (mem->targetNode >= 0)
-virBufferAsprintf(, "node=%d,", mem->targetNode);
-
-if (mem->labelsize)
-virBufferAsprintf(, "label-size=%llu,", mem->labelsize * 1024);
-
-if (mem->blocksize) {
-virBufferAsprintf(, "block-size=%llu,", mem->blocksize * 1024);
-virBufferAsprintf(, "requested-size=%llu,", mem->requestedsize * 
1024);
-}
+if (mem->readonly)
+unarmed = VIR_TRISTATE_BOOL_YES;

if (mem->uuid) {
-char uuidstr[VIR_UUID_STRING_BUFLEN];
-
+uuidstr = g_new0(char, VIR_UUID_STRING_BUFLEN);
virUUIDFormat(mem->uuid, uuidstr);
-virBufferAsprintf(, "uuid=%s,", uuidstr);
}

-if (mem->readonly) {
-virBufferAddLit(, "unarmed=on,");
-}
-
-virBufferAsprintf(, "memdev=mem%s,id=%s",
-  mem->info.alias, mem->info.alias);
+if (virJSONValueObjectCreate(,
+ "s:driver", device,
+ "k:node", mem->targetNode,
+ "P:label-size", mem->labelsize * 1024,
+ "P:block-size", mem->blocksize * 1024,
+ "P:requested-size", mem->requestedsize * 1024,
+ "S:uuid", uuidstr,
+ "T:unarmed", unarmed,
+ "f:memdev", g_strdup_printf("mem%s", 
mem->info.alias),


One more usage of 'f'.

Also, I'm really glad these keys use letters, instead of, let's say,
Morse code.


+ "s:id", mem->info.alias,
+ NULL) < 0)
+return NULL;



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH v3 3/6] qemu: remove use of (+|-)name syntax for -cpu featres

[Jiri Denemark]

On Fri, Oct 08, 2021 at 10:01:42 +0100, Daniel P. Berrangé wrote:
> The -cpu arg gained support for feature=on|off syntax for the x86
> emulator in 2.4.0
> 
>   commit 38e5c119c2925812bd441450ab9e5e00fc79e662
>   Author: Eduardo Habkost 
>   Date:   Mon Mar 23 17:29:32 2015 -0300
> 
> target-i386: Register QOM properties for feature flags
> 
> Most other targets gained this syntax even earlier in 1.4.1
> 
>   commit 1590bbcb02921dfe8e3cf66e3a3aafd31193babf
>   Author: Andreas Färber 
>   Date:   Mon Mar 3 23:33:51 2014 +0100
> 
> cpu: Implement CPUClass::parse_features() for the rest of CPUs
> 
> CPUs who do not provide their own implementation of feature parsing
> will treat each option as a QOM property and set it to the supplied
> value.
> 
> There appears no reason to keep supporting "+|-feature" syntax,
> given the current minimum QEMU version.
> 
> Signed-off-by: Daniel P. Berrangé 

Reviewed-by: Jiri Denemark 

Re: [libvirt PATCH v3 2/6] qemu: always use hyphens in hyperv feature names

[Jiri Denemark]

On Fri, Oct 08, 2021 at 10:01:41 +0100, Daniel P. Berrangé wrote:
> QEMU switched from using underscores in x86 CPU features to hyphens
> in the 2.8.0 series with two commits
> 
>   commit fc7dfd205f3287893c436d932a167bffa30579c8 (HEAD, refs/bisect/bad)
>   Author: Eduardo Habkost 
>   Date:   Fri Sep 30 15:49:40 2016 -0300
> 
> target-i386: Remove underscores from feat_names arrays
> 
>   commit 54b8dc7c19cd781e96f1e9b001ca6001d804eb19
>   Author: Eduardo Habkost 
>   Date:   Fri Sep 30 15:49:38 2016 -0300
> 
> target-i386: Register aliases for feature names with underscores
> 
> Libvirt names use underscores so we conditionally tranlate the
> names when talking to new QEMU. Since the min QEMU was raised to
> version 2.11.0, all QEMU versions we talk to expect hypens, so
> the translation can be done unconditionally.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_command.c | 8 +---
>  tests/qemuxml2argvdata/hyperv.x86_64-4.0.0.args | 2 +-
>  2 files changed, 2 insertions(+), 8 deletions(-)

Reviewed-by: Jiri Denemark 

Re: [libvirt PATCH v3 1/6] qemu: always translate underscores to hyphens in CPU features

[Jiri Denemark]

On Fri, Oct 08, 2021 at 10:01:40 +0100, Daniel P. Berrangé wrote:
> QEMU switched from using underscores in x86 CPU features to hyphens
> in the 2.8.0 series with two commits
> 
>   commit fc7dfd205f3287893c436d932a167bffa30579c8 (HEAD, refs/bisect/bad)
>   Author: Eduardo Habkost 
>   Date:   Fri Sep 30 15:49:40 2016 -0300
> 
> target-i386: Remove underscores from feat_names arrays
> 
>   commit 54b8dc7c19cd781e96f1e9b001ca6001d804eb19
>   Author: Eduardo Habkost 
>   Date:   Fri Sep 30 15:49:38 2016 -0300
> 
> target-i386: Register aliases for feature names with underscores
> 
> Libvirt names use underscores so we conditionally tranlate the
> names when talking to new QEMU. Since the min QEMU was raised to
> version 2.11.0, all QEMU versions we talk to expect hypens, so
> the translation can be done unconditionally.

Reviewed-by: Jiri Denemark 

Re: [PATCH 057/103] qemuBuildWatchdogCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The watchdog doesn't have any special properties.

Convert the command line generator and hotplug code.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 31 +++
src/qemu/qemu_command.h |  6 +++---
src/qemu/qemu_hotplug.c |  6 +++---
3 files changed, 21 insertions(+), 22 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 056/103] qemuBuildPanicCommandLine: Generate via JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Format a JSON object with the device properties and then use
qemuBuildDeviceCommandlineFromJSON to convert it to the standard
commandline for now.

The 'ioport' property of 'pvpanic' is a number in QEMU:
 ioport=-  (default: 1285)

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 35 +++
1 file changed, 23 insertions(+), 12 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 055/103] qemu: command: Introduce JSON variant of qemuBuildRomProps

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Add a JSON variant of the generator 'rom' properties. For convenience
both the old and new are for now marked as unused, which will be removed
once the conversion is complete.

The formatted properties have following types according to QEMU.
'virtio-blk-pci' was used as an example:
 rombar=-  (default: 1)
 romfile=

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 42 -
1 file changed, 41 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 054/103] qemu: command: Introduce helper for building JSON props of -device into commandline

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The helper converts the JSON object to a string and adds it to the
current command as arguments of '-device'. The helper also prepares for
'-device' taking JSON directly.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 27 +++
1 file changed, 27 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 053/103] qemu: command: Introduce JSON variant of qemuBuildVirtioDevStr

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Add a JSON variant of the generator of properties for virtio devices.
For convenience both the old and new are for now marked as unused, which
will be removed once the conversion is complete.

The formatted properties have following types according to QEMU.
'virtio-blk-pci' was used as an example:

 disable-legacy= - on/off/auto (default: "auto")
 disable-modern=  -  (default: false)
 iommu_platform=  - on/off (default: false)
 ats= - on/off (default: false)
 packed=  - on/off (default: false)

Note that  is an enum type without alternates in QMP so it
must be represented as a string in JSON.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 58 -
1 file changed, 57 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 033/103] qemu: command: Introduce JSON equivalent of qemuBuildDeviceAddressStr

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Upcoming patches will start converting the formatting of arguments for
-device from a string to JSON so that we can keep proper types around
when using it via QMP.

This means we will need an equivalet for the device address builder


*equivalent


function. 'qemuBuildDeviceAddressProps' provides equal functionality,
but the output differs for fields where a number is expected, where
we've previously formatted a hex value but now end up with a decimal
value per JSON standard.

For given address types I've selected an example device and used
'-device $DEV,help' to obtain the current types recognized by qemu:

Note that 'bus' is not shown below, but it's already a string so we can
keep using it as a string.

VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI (virtio-balloon-pci)
 acpi-index=-  (default: 0)
 addr=   - Slot and optional function number, example: 06.0 or 
06 (default: -1)
 multifunction=   - on/off (default: false)

Note that 'addr' is here defined as 'int32' but in fact internally in
qemu is an alternate type between a number and a string so we can keep
using strings here.

VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB (usb-tablet)
 port=

VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO (spapr-vty)
 reg=   -  (default: 4294967295)

VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW (virtio-blk-cww)
 devno=- Identifier of an I/O device in the channel subsystem, 
example: fe.1.23ab

VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA (isa-serial)
 iobase=-  (default: 4294967295)
 irq=   -  (default: 4294967295)

VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM (pc-dimm)
 slot=   -  (default: -1)
 addr=  -  (default: 0)

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 101 +++-
1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 56acd9a109..565b1970dc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -416,7 +416,7 @@ qemuBuildDeviceAddressPCIStr(virBuffer *buf,
}


-static int
+static int G_GNUC_UNUSED
qemuBuildDeviceAddressStr(virBuffer *buf,
  const virDomainDef *domainDef,
  virDomainDeviceInfo *info)
@@ -484,6 +484,105 @@ qemuBuildDeviceAddressStr(virBuffer *buf,
}


+static int G_GNUC_UNUSED
+qemuBuildDeviceAddressProps(virJSONValue *props,
+const virDomainDef *domainDef,
+virDomainDeviceInfo *info)
+{
+switch ((virDomainDeviceAddressType) info->type) {
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: {
+g_autofree char *pciaddr = NULL;
+
+if (info->addr.pci.function != 0)
+pciaddr = g_strdup_printf("0x%x.0x%x", info->addr.pci.slot, 
info->addr.pci.function);
+else
+pciaddr = g_strdup_printf("0x%x", info->addr.pci.slot);
+
+if (virJSONValueObjectAdd(props,
+  "F:bus", 
qemuBuildDeviceAddressPCIGetBus(domainDef, info),


Without the '[fF]' patch, this only needs one autofree'd variable.


+  "T:multifunction", info->addr.pci.multi,
+  "s:addr", pciaddr,
+  "p:acpi-index", info->acpiIndex,
+  NULL) < 0)
+return -1;
+
+return 0;
+}
+break;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: {
+const char *contAlias = NULL;
+g_auto(virBuffer) port = VIR_BUFFER_INITIALIZER;
+
+if (!(contAlias = virDomainControllerAliasFind(domainDef,
+   
VIR_DOMAIN_CONTROLLER_TYPE_USB,
+   info->addr.usb.bus)))
+return -1;
+
+virDomainUSBAddressPortFormatBuf(, info->addr.usb.port);
+
+if (virJSONValueObjectAdd(props,
+  "f:bus", g_strdup_printf("%s.0", contAlias),


Here too.


+  "S:port", virBufferCurrentContent(),
+  NULL) < 0)
+return -1;


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] vireventglib: Remove handles with the highest priority

[Daniel P . Berrangé]

On Fri, Oct 08, 2021 at 01:56:24PM +0200, Michal Prívozník wrote:
> On 10/8/21 1:36 PM, Daniel P. Berrangé wrote:
> > On Fri, Oct 08, 2021 at 01:12:49PM +0200, Michal Privoznik wrote:
> >> When a server decides to close a client, the
> >> virNetServerClientCloseLocked() is called. In here various
> >> cleanup steps are taken, but the most important part (from this
> >> commit's POV at least) is the way that the socket is closed.
> >> Firstly, removal of the socket associated with the client from
> >> the event loop is signalized and then the socket is unrefed. The
> >> socket is not closed just yet though, because the event loop
> >> holds a reference to it. This reference will be freed as soon as
> >> the event loop wakes up and starts issuing callbacks (in this
> >> case virNetSocketEventFree()).
> >>
> >> So far, this is how things usually work. But if the daemon
> >> reaches the number of opened files limit, things start to work
> >> differently.
> >>
> >> If the RLIMIT_NOFILE limit is reached and there's a client that
> >> wants to connect then the event loop wakes up, sees POLLIN on the
> >> socket and calls virNetServerServiceAccept() which in turn calls
> >> virNetSocketAccept(). But because of the limit, accept() fails
> >> with EMFILE leaving the POLLIN event unhandled. The dispatch then
> >> continues to next FDs with events on them. BUT, it will NOT call
> >> the socket removal callback (virNetSocketEventFree()) because it
> >> has low priority (G_PRIORITY_DEFAULT_IDLE). Per glib's
> >> documentation:
> > 
> > if virNetSocketAccept fails, there's no client socket to
> > remove/close.  AFAIK, we are not removing the server
> > socket either.
> > 
> > So I'm not following which socket is supposedly being
> > removed in this scenario ?
> 
> Right. I didn't phrase it properly. Let's call the
> /var/run/libvirt/libvirt-sock SockA and accepted socket (i.e. per
> client) SockB.
> Let the NOFILE limit be just the right value so that both SockA and
> SockB can exist, but no more. Now, let's have another client trying to
> connect(). The event loop wakes up, because there's an event on SockA.
> But the connection can't be accept()-ed. Alright, so let the already
> connected client disconnect (that is, SockB). This will result in:
> 
>   g_idle_add(virEventGLibHandleRemoveIdle, SockB); /* I've made a
> shortcut here, in reality there's a data variable that points to SockB,
> but you get the idea */
> 
> Now, because we still haven't handled the connection attempt at SockA
> the idle source won't ever be called because it has lower priority.
> Therefore, the event loop is stuck in endless loop trying to accept()
> connection on SockA.

Ah i see, "idle" callbacks aren't called because the event loop
isn't idle.

So with this change, we'll let the idle callback run, which will
close the FD which will let libvird accept the client which gets
us out of the 100% CPU burn loop. unless there were two
pending clients


So, ACK to this patch as a quick fix, but it feels like we
ultimately have a more serious problem.

I wonder if we should temporarily stop watching events on
the incoming server if we get EMFILE first time on accept,
and set a timer to re-enable events 60 seconds later. Rinse,
repeat.


> >>  * Each event source is assigned a priority. The default priority,
> >>  * %G_PRIORITY_DEFAULT, is 0. Values less than 0 denote higher priorities.
> >>  * Values greater than 0 denote lower priorities. Events from high priority
> >>  * sources are always processed before events from lower priority sources.
> >>
> >> and per g_idle_add() documentation:
> >>
> >>  * Adds a function to be called whenever there are no higher priority
> >>  * events pending to the default main loop. The function is given the
> >>  * default idle priority, %G_PRIORITY_DEFAULT_IDLE.
> >>
> >> Now, because we did not accept() the client we are constantly
> >> seeing POLLIN on the main socket and thus the removal of the
> >> client socket won't ever happen.
> >>
> >> The fix is to set at least the same priority as other sources,
> >> but since we want to just close an FD, let's give it the highest
> >> priority and call it before handling other events.
> >>
> >> This issue can be easily reproduced, for instance:
> >>
> >>  # ulimit -S -n 40 (tweak this number if needed)
> >>  # ./src/libvirtd
> >>
> >> from another terminal:
> >>
> >>  # for ((i=0; i<100; i++)); do virsh list & done; virsh list
> >>
> >> The last `virsh list` must not get stuck.
> > 
> > The bug below describes libvirt core dumps rather than
> > hangs. Do you know why it dumped ?
> 
> Because of glib. We called some glib function which internally wanted to
> open a pipe which failed. Thus glib aborted. I don't remember the exact
> function, but I can look it up if you want.

Oh, so this patch is not actally fixing the core dump scenario
then.  Even after we fix the bug that prevents clients disconnecting,
it will still dump if this API is called at a point where 

Re: [PATCH 018/103] qemu: capabilities: Introduce QEMU_CAPS_DEVICE_JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The flag will be used to switch use of JSON arguments for -device once
qemu will support it.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_capabilities.c | 3 +++
src/qemu/qemu_capabilities.h | 3 +++
2 files changed, 6 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 017/103] qemu: capabilities: Introduce QEMU_CAPS_CHARDEV_JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The flag will be used to switch use of JSON arguments for -chardev once
qemu will support it.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_capabilities.c | 1 +
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 2 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 1/2] qemu_migration: set bandwidth in priv during migration

[Jiri Denemark]

On Fri, Oct 08, 2021 at 12:56:24 +0200, Michal Prívozník wrote:
> On 10/8/21 12:44 PM, Jiri Denemark wrote:
> > On Fri, Oct 08, 2021 at 10:19:04 +0200, Kristina Hanicova wrote:
> >> We did not set priv->migMaxBandwidth if '--bandwidth' was
> >> specified as an option in the 'migrate' virsh command. This
> >> caused in printing the wrong value if virsh command
> >> 'migrate-getspeed' was called during the migration. This patch
> >> first sets the value to the given bandwidth (if one was
> >> specified) and restores the previous value after the migration.
> >>
> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1806856
> > 
> > I was wondering whether we should just read the bandwidth back from QEMU
> > instead when miration is running. But I guess this is good enough.
> 
> I was looking into this, but I believe that 'query-migrate' returns the
> actual bandwidth and not the limit. Or do you mean other command?

Yes, query-migrate-parameters, which is internally mapped to
qemuMigrationParamsFetch.

Jirka

Re: [PATCH 015/103] qemuxml2argvtest: Refactor QAPI schema validation code

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Prevent duplication of code when extending the validator for new
commands. Add a struct describing a command to validate and make the
validation loop a bit more robust to corner cases.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvtest.c | 120 +--
1 file changed, 66 insertions(+), 54 deletions(-)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 7c6e3d3dd7..b5fab1178c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c


[..]


+
+if (!(jsonargs = virJSONValueFromString(curargs)))
+return -1;
+
+if (testQEMUSchemaValidateCommand(command->schema, jsonargs,
+  schema, false, false, ) < 
0) {
+VIR_TEST_VERBOSE("failed to validate '%s %s' against QAPI schema: 
%s",
+ command->name, curargs, 
virBufferCurrentContent());
+return -1;
+}
+
+/* don't check the argument twice */


// this is bridge


+arg++;
+}
+}
+
+return 0;


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 015/103] qemuxml2argvtest: Refactor QAPI schema validation code

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Prevent duplication of code when extending the validator for new
commands. Add a struct describing a command to validate and make the
validation loop a bit more robust to corner cases.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvtest.c | 120 +--
1 file changed, 66 insertions(+), 54 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 013/103] qemu: Remove 'qemuBuildCommandLineFlags' and associated code

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

The -netdev formatter code switched to a real virQEMUCaps flag so we can
remove the old flags which used to enable JSON for -netdev for
validation purposes.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.h  |  4 
src/qemu/qemu_driver.c   |  2 +-
src/qemu/qemu_process.c  | 10 ++
src/qemu/qemu_process.h  |  3 +--
tests/qemuxml2argvtest.c | 11 ---
5 files changed, 8 insertions(+), 22 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 012/103] virQEMUBuildNetdevCommandlineFromJSON: Remove unused formatter

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Now that everything was replaced by the new code we can remove this
function.

Signed-off-by: Peter Krempa 
---
src/libvirt_private.syms |  1 -
src/util/virqemu.c   | 30 --
src/util/virqemu.h   |  4 
3 files changed, 35 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 011/103] qemu: command: Format netdev as JSON when QEMU_CAPS_NETDEV_JSON is present

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Base the JSON output on a regular capability flag rather than purely
internal flag. This will prepare for the time when QEMU will accept JSON
argumets for -netdev.

For now the capability is not set (thus we for now don't have QMP
schema validation) but that will be addressed later.

To achieve this 'qemuBuildNetdevCommandlineFromJSON' is introduced
and all callers of 'virQEMUBuildNetdevCommandlineFromJSON' are
refactored to use the new helper.

Signed-off-by: Peter Krempa 
---
src/libvirt_private.syms |  1 +
src/qemu/qemu_command.c  | 55 ++--
src/util/virqemu.c   |  2 +-
src/util/virqemu.h   |  4 +++
4 files changed, 42 insertions(+), 20 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 010/103] testCompareXMLToArgvValidateSchema: Base -netdev validation on JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Base the validation on presence of JSON as we do with other validated
commands. This will prepare the code for a refactor so that it's the
same for all validated commands.

Signed-off-by: Peter Krempa 
---
tests/qemuxml2argvtest.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 009/103] qemuBuildObjectCommandlineFromJSON: Remove checks for 'type' and 'alias'

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

We validate the generated props agains the QMP schema which makes sure


*against


that the objects are generated properly.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 008/103] qemu: capabilities: Introduce QEMU_CAPS_NETDEV_JSON

[Ján Tomko]

On a Thursday in 2021, Peter Krempa wrote:

Introduce a capability that will be asserted once '-netdev' will accept
JSON. For now it will be dormant (only used by tests).

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_capabilities.c | 1 +
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 2 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] vireventglib: Remove handles with the highest priority

[Michal Prívozník]

On 10/8/21 1:36 PM, Daniel P. Berrangé wrote:
> On Fri, Oct 08, 2021 at 01:12:49PM +0200, Michal Privoznik wrote:
>> When a server decides to close a client, the
>> virNetServerClientCloseLocked() is called. In here various
>> cleanup steps are taken, but the most important part (from this
>> commit's POV at least) is the way that the socket is closed.
>> Firstly, removal of the socket associated with the client from
>> the event loop is signalized and then the socket is unrefed. The
>> socket is not closed just yet though, because the event loop
>> holds a reference to it. This reference will be freed as soon as
>> the event loop wakes up and starts issuing callbacks (in this
>> case virNetSocketEventFree()).
>>
>> So far, this is how things usually work. But if the daemon
>> reaches the number of opened files limit, things start to work
>> differently.
>>
>> If the RLIMIT_NOFILE limit is reached and there's a client that
>> wants to connect then the event loop wakes up, sees POLLIN on the
>> socket and calls virNetServerServiceAccept() which in turn calls
>> virNetSocketAccept(). But because of the limit, accept() fails
>> with EMFILE leaving the POLLIN event unhandled. The dispatch then
>> continues to next FDs with events on them. BUT, it will NOT call
>> the socket removal callback (virNetSocketEventFree()) because it
>> has low priority (G_PRIORITY_DEFAULT_IDLE). Per glib's
>> documentation:
> 
> if virNetSocketAccept fails, there's no client socket to
> remove/close.  AFAIK, we are not removing the server
> socket either.
> 
> So I'm not following which socket is supposedly being
> removed in this scenario ?

Right. I didn't phrase it properly. Let's call the
/var/run/libvirt/libvirt-sock SockA and accepted socket (i.e. per
client) SockB.
Let the NOFILE limit be just the right value so that both SockA and
SockB can exist, but no more. Now, let's have another client trying to
connect(). The event loop wakes up, because there's an event on SockA.
But the connection can't be accept()-ed. Alright, so let the already
connected client disconnect (that is, SockB). This will result in:

  g_idle_add(virEventGLibHandleRemoveIdle, SockB); /* I've made a
shortcut here, in reality there's a data variable that points to SockB,
but you get the idea */

Now, because we still haven't handled the connection attempt at SockA
the idle source won't ever be called because it has lower priority.
Therefore, the event loop is stuck in endless loop trying to accept()
connection on SockA.

> 
> 
>>  * Each event source is assigned a priority. The default priority,
>>  * %G_PRIORITY_DEFAULT, is 0. Values less than 0 denote higher priorities.
>>  * Values greater than 0 denote lower priorities. Events from high priority
>>  * sources are always processed before events from lower priority sources.
>>
>> and per g_idle_add() documentation:
>>
>>  * Adds a function to be called whenever there are no higher priority
>>  * events pending to the default main loop. The function is given the
>>  * default idle priority, %G_PRIORITY_DEFAULT_IDLE.
>>
>> Now, because we did not accept() the client we are constantly
>> seeing POLLIN on the main socket and thus the removal of the
>> client socket won't ever happen.
>>
>> The fix is to set at least the same priority as other sources,
>> but since we want to just close an FD, let's give it the highest
>> priority and call it before handling other events.
>>
>> This issue can be easily reproduced, for instance:
>>
>>  # ulimit -S -n 40 (tweak this number if needed)
>>  # ./src/libvirtd
>>
>> from another terminal:
>>
>>  # for ((i=0; i<100; i++)); do virsh list & done; virsh list
>>
>> The last `virsh list` must not get stuck.
> 
> The bug below describes libvirt core dumps rather than
> hangs. Do you know why it dumped ?

Because of glib. We called some glib function which internally wanted to
open a pipe which failed. Thus glib aborted. I don't remember the exact
function, but I can look it up if you want.

Michal