[PATCH 0/5] qemu_monitor_json: Assume existence of some commands

[Michal Privoznik]

Just like with QEMU capabilities, we can safely assume existence of some
monitor commands because they were introduced in older than minimal
required version and do not depend on QEMU build arguments.

Michal Prívozník (5):
  qemuMonitorJSONGetMigrationParams: Don't return early on
CommandNotFound
  qemuMonitorJSONGetDumpGuestMemoryCapability: Don't return early on
CommandNotFound
  qemuMonitorJSONGetKVMState: Don't return early on CommandNotFound
  qemuMonitorJSONGetMemoryDeviceInfo: Don't return early on
CommandNotFound
  qemuMonitorJSONGetMigrationCapabilities: Don't return early on
CommandNotFound

 src/qemu/qemu_monitor_json.c | 23 ---
 1 file changed, 23 deletions(-)

-- 
2.32.0

Re: [PATCH 0/5] qemu_monitor_json: Assume existence of some commands

[Michal Prívozník]

On 10/21/21 10:56 AM, Michal Privoznik wrote:
> Just like with QEMU capabilities, we can safely assume existence of some
> monitor commands because they were introduced in older than minimal
> required version and do not depend on QEMU build arguments.
> 
> Michal Prívozník (5):
>   qemuMonitorJSONGetMigrationParams: Don't return early on
> CommandNotFound
>   qemuMonitorJSONGetDumpGuestMemoryCapability: Don't return early on
> CommandNotFound
>   qemuMonitorJSONGetKVMState: Don't return early on CommandNotFound
>   qemuMonitorJSONGetMemoryDeviceInfo: Don't return early on
> CommandNotFound
>   qemuMonitorJSONGetMigrationCapabilities: Don't return early on
> CommandNotFound
> 
>  src/qemu/qemu_monitor_json.c | 23 ---
>  1 file changed, 23 deletions(-)
> 

Please ignore this. I'll send v2 shortly.

Michal

Re: [PATCH 5/5] qemu: Prefer -numa cpu over -numa node,cpus=

[Igor Mammedov]

On Wed, 20 Oct 2021 13:07:59 +0200
Michal Prívozník  wrote:

> On 10/6/21 3:32 PM, Igor Mammedov wrote:
> > On Thu, 30 Sep 2021 14:08:34 +0200
> > Peter Krempa  wrote:
> >   
> >> On Tue, Sep 21, 2021 at 16:50:31 +0200, Michal Privoznik wrote:  
> >>> QEMU is trying to obsolete -numa node,cpus= because that uses
> >>> ambiguous vCPU id to [socket, die, core, thread] mapping. The new
> >>> form is:
> >>>
> >>>   -numa cpu,node-id=N,socket-id=S,die-id=D,core-id=C,thread-id=T
> >>>
> >>> which is repeated for every vCPU and places it at [S, D, C, T]
> >>> into guest NUMA node N.
> >>>
> >>> While in general this is magic mapping, we can deal with it.
> >>> Firstly, with QEMU 2.7 or newer, libvirt ensures that if topology
> >>> is given then maxvcpus must be sockets * dies * cores * threads
> >>> (i.e. there are no 'holes').
> >>> Secondly, if no topology is given then libvirt itself places each
> >>> vCPU into a different socket (basically, it fakes topology of:
> >>> [maxvcpus, 1, 1, 1])
> >>> Thirdly, we can copy whatever QEMU is doing when mapping vCPUs
> >>> onto topology, to make sure vCPUs don't start to move around.
> >>
> >> There's a problem with this premise though and unfortunately we don't
> >> seem to have qemuxml2argvtest for it.
> >>
> >> On PPC64, in certain situations the CPU can be configured such that
> >> threads are visible only to VMs. This has substantial impact on how CPUs
> >> are configured using the modern parameters (until now used only for
> >> cpu hotplug purposes, and that's the reason vCPU hotplug has such
> >> complicated incantations when starting the VM).
> >>
> >> In the above situation a CPU with topology of:
> >>  sockets=1, cores=4, threads=8 (thus 32 cpus)
> >>
> >> will only expose 4 CPU "devices".
> >>
> >>  core-id: 0,  core-id: 8, core-id: 16 and core-id: 24
> >>
> >> yet the guest will correctly see 32 cpus when used as such.
> >>
> >> You can see this in:
> >>
> >> tests/qemuhotplugtestcpus/ppc64-modern-individual-monitor.json
> >>
> >> Also note that the 'props' object does _not_ have any socket-id, and
> >> management apps are supposed to pass in 'props' as is. (There's a bunch
> >> of code to do that on hotplug).
> >>
> >> The problem is that you need to query the topology first (unless we want
> >> to duplicate all of qemu code that has to do with topology state and
> >> keep up with changes to it) to know how it's behaving on current
> >> machine.  This historically was not possible. The supposed solution for
> >> this was the pre-config state where we'd be able to query and set it up
> >> via QMP, but I was not keeping up sufficiently with that work, so I
> >> don't know if it's possible.
> >>
> >> If preconfig is a viable option we IMO should start using it sooner
> >> rather than later and avoid duplicating qemu's logic here.  
> > 
> > using preconfig is preferable variant otherwise libvirt
> > would end up duplicating topology logic which differs not only
> > between targets but also between machine/cpu types.
> > 
> > Closest example how to use preconfig is in pc_dynamic_cpu_cfg()
> > test case. Though it uses query-hotpluggable-cpus only for
> > verification, but one can use the command at the preconfig
> > stage to get topology for given -smp/-machine type combination.  
> 
> Alright, -preconfig should be pretty easy. However, I do have some
> points to raise/ask:
> 
> 1) currently, exit-preconfig is marked as experimental (hence its "x-"
> prefix). Before libvirt consumes it, QEMU should make it stable. Is
> there anything that stops QEMU from doing so or is it just a matter of
> sending patches (I volunteer to do that)?

if I recall correctly, it was made experimental due to lack of
actual users (it was supposed that libvirt would consume it
once available but it didn't happen for quite a long time).

So patches to make it stable interface should be fine.

> 
> 2) In my experiments I try to mimic what libvirt does. Here's my cmd
> line:
> 
> qemu-system-x86_64 \
> -S \
> -preconfig \
> -cpu host \
> -smp 120,sockets=2,dies=3,cores=4,threads=5 \
> -object 
> '{"qom-type":"memory-backend-memfd","id":"ram-node0","size":4294967296,"host-nodes":[0],"policy":"bind"}'
>  \
> -numa node,nodeid=0,memdev=ram-node0 \
> -no-user-config \
> -nodefaults \
> -no-shutdown \
> -qmp stdio
> 
> and here is my QMP log:
> 
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 6}, 
> "package": "v6.1.0-1552-g362534a643"}, "capabilities": ["oob"]}}
> 
> {"execute":"qmp_capabilities"}
> {"return": {}}
> 
> {"execute":"query-hotpluggable-cpus"}
> {"return": [{"props": {"core-id": 3, "thread-id": 4, "die-id": 2, 
> "socket-id": 1}, "vcpus-count": 1, "type": "host-x86_64-cpu"}, {"props": 
> {"core-id": 3, "thread-id": 3, "die-id": 2, "socket-id": 1}, "vcpus-count": 
> 1, "type": "host-x86_64-cpu"}, {"props": {"core-id": 3, "thread-id": 2, 
> "die-id": 2, "socket-id": 1}, "vcpus-count": 1, "type": "host-x86_64-cpu"}, 
> {"props": {"core-id": 3, "thread-id": 1, 

Re: [RFC PATCH 00/10] VirtioNet RSS support

[Nikolay Shirokovskiy]

чт, 21 окт. 2021 г. в 01:28, Andrew Melnichenko :

> Hi,
> Yes, the work is in progress. Now. I'm working with a proper solution for
> the eBPF RSS helper.
>

Ok. Thank you!



>
> On Wed, Oct 20, 2021 at 3:23 PM Nikolay Shirokovskiy <
> nshirokovs...@virtuozzo.com> wrote:
>
>> Hi, Andrew.
>>
>> We in Virtuozzo are interested in this functionality too. Do you plan to
>> continue your work on it?
>>
>> Nikolay
>>
>> пн, 16 авг. 2021 г. в 15:00, Andrew Melnichenko :
>>
>>> Ping
>>>
>>> On Wed, Jul 28, 2021 at 11:17 AM Andrew Melnychenko 
>>> wrote:
>>>
 This series of patches add RSS property support for virtio-net-pci.

 Virtio RSS effectively works with TAP devices, it requires additional
 vectors for VirtioNet, queues for TAP device, and vCPU cores.
 Example of device configuration:
 ```
 
   
   
   
   
   >>> function="0x0"/>
 
 ```

 Capability "rss" enables RSS, "rss_hash_report" - enables hashes in
 vheader.
 Qemu uses eBPF program as RSS driver.
 For loading RSS eBPF program, the helper is used.
 Path to the helper is provided by Qemu through "query-helper-paths" qmp
 command.
 The helper "qemu-ebpf-rss-helper" is built with Qemu and may differ
 from build to build.
 So it's required that the Qemu should provide a proper helper path.
 Libvirt would call the helper and receive the program and map fd
 through unix socket.
 Fds would be passed to Qemu in "ebpf_rss_fds" property by passing to
 child process or unix socket.
 If libvirt would fail at helper call or Qemu didn't provide the path,
 the Qemu would be launched without "ebpf_rss_fds" property.
 Without "ebpf_rss_fds" property, Qemu would try to load eBPF program by
 itself - usually, it would require additional system permissions.
 Qemu may use "in-qemu" RSS as a fallback option, which will not require
 system
 permissions, but doesn't work with vhost TAP.

 Qemu patches:
 https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg03535.html

 Andrew Melnychenko (10):
   domain_conf: Added configs for RSS and Hash report.
   qemu_capabilities: Added capabilites for qemu's "rss" and "hash".
   qemu_command: Added "rss" and "hash" properties.
   virsocket: Added receive for multiple fds.
   qemu_capabilities: Added capability for qemu's "ebpf_rss_fds".
   qemu_capabilities: Added capability for ebpf helper path.
   qemu_interface: Added ebpf helper call.
   qemu_command: Added ebpf RSS helper call for NIC creation.
   qemu_hotplug: Added helper call for hotplug NIC.
   docs: Added descriptions for "rss" and "rss_hash_report"
 configurations.

  docs/formatdomain.rst| 16 +++
  src/conf/domain_conf.c   | 31 +-
  src/conf/domain_conf.h   |  2 +
  src/libvirt_private.syms |  1 +
  src/qemu/qemu_capabilities.c | 48 +
  src/qemu/qemu_capabilities.h |  5 +++
  src/qemu/qemu_command.c  | 46 +++-
  src/qemu/qemu_command.h  |  2 +
  src/qemu/qemu_hotplug.c  | 30 -
  src/qemu/qemu_interface.c| 54 +++
  src/qemu/qemu_interface.h|  2 +
  src/qemu/qemu_monitor.c  |  9 
  src/qemu/qemu_monitor.h  |  3 ++
  src/qemu/qemu_monitor_json.c | 50 ++
  src/qemu/qemu_monitor_json.h |  3 ++
  src/qemu/qemu_validate.c | 16 +++
  src/util/virsocket.c | 83 
  src/util/virsocket.h |  2 +
  18 files changed, 399 insertions(+), 4 deletions(-)

 --
 2.31.1

Re: [PATCH v4 1/5] qemu: add disk post parse to qemublocktest

[Peter Krempa]

On Thu, Oct 07, 2021 at 14:21:17 -0500, Or Ozeri wrote:
> The post parse callback is part of the real (non-test) processing flow.
> This commit adds it (for disks) to the qemublocktest flow as well.
> Specifically, this will be needed for tests that use luks encryption,
> so that the default encryption engine (which is added in an upcoming commit)
> will be overridden by qemu.
> 
> Signed-off-by: Or Ozeri 
> ---
>  src/qemu/qemu_domain.c |  2 +-
>  src/qemu/qemu_domain.h |  3 +++
>  tests/qemublocktest.c  | 29 -
>  3 files changed, 16 insertions(+), 18 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v4 2/5] qemu: capablities: Detect presence of 'rbd-encryption' as QEMU_CAPS_RBD_ENCRYPTION

[Peter Krempa]

On Thu, Oct 07, 2021 at 14:21:18 -0500, Or Ozeri wrote:
> rbd encryption is new in qemu 6.1.0.
> This commit adds capability probing for it.
> 
> Signed-off-by: Or Ozeri 
> ---
>  src/qemu/qemu_capabilities.c | 2 ++
>  src/qemu/qemu_capabilities.h | 1 +
>  tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
>  3 files changed, 4 insertions(+)

Some new test files were added in the meanwhile. I'll update the patch
to accomodate those.

Reviewed-by: Peter Krempa 

[PATCH v2 2/6] qemuMonitorJSONGetMigrationParams: Don't return early on CommandNotFound

[Michal Privoznik]

The qemuMonitorJSONGetMigrationParams() function executes
'query-migrate-parameters' command and returns early if QEMU
doesn't know the command. Well, the command was introduced in
QEMU release 2.4 (specifically in commit v2.4.0-rc0~147^2~3) and
since the minimum required version is 2.11.0 we can be sure that
the command will always exist.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_monitor_json.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7bf3a9981b..08cd535045 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3281,11 +3281,6 @@ qemuMonitorJSONGetMigrationParams(qemuMonitor *mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;
 
-if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
-ret = 0;
-goto cleanup;
-}
-
 if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
 goto cleanup;
 
-- 
2.32.0

[PATCH v2 0/6] qemu_monitor_json: Assume existence of some commands

[Michal Privoznik]

Technically a v2 of:

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

but I've cancelled sending in the middle of v1. Anyway, patch 1/6 is new
(yeah, I've noticed a test failing so I've cancelled sending v1).

Michal Prívozník (6):
  qemumigparamstest: Drop "unsupported" test case
  qemuMonitorJSONGetMigrationParams: Don't return early on
CommandNotFound
  qemuMonitorJSONGetDumpGuestMemoryCapability: Don't return early on
CommandNotFound
  qemuMonitorJSONGetKVMState: Don't return early on CommandNotFound
  qemuMonitorJSONGetMemoryDeviceInfo: Don't return early on
CommandNotFound
  qemuMonitorJSONGetMigrationCapabilities: Don't return early on
CommandNotFound

 src/qemu/qemu_monitor_json.c  | 23 ---
 tests/qemumigparamsdata/unsupported.json  |  3 ---
 tests/qemumigparamsdata/unsupported.reply |  7 ---
 tests/qemumigparamsdata/unsupported.xml   |  4 
 tests/qemumigparamstest.c |  1 -
 5 files changed, 38 deletions(-)
 delete mode 100644 tests/qemumigparamsdata/unsupported.json
 delete mode 100644 tests/qemumigparamsdata/unsupported.reply
 delete mode 100644 tests/qemumigparamsdata/unsupported.xml

-- 
2.32.0

[PATCH v2 1/6] qemumigparamstest: Drop "unsupported" test case

[Michal Privoznik]

The aim of "unsupported" test case is to check whether our code
handles 'CommandNotFound' error returned for
'query-migrate-parameters' monitor command. Well, the command is
pretty old and every QEMU that we are dealing with supports it.
Thus this test case is useless. Drop it.

Signed-off-by: Michal Privoznik 
---
 tests/qemumigparamsdata/unsupported.json  | 3 ---
 tests/qemumigparamsdata/unsupported.reply | 7 ---
 tests/qemumigparamsdata/unsupported.xml   | 4 
 tests/qemumigparamstest.c | 1 -
 4 files changed, 15 deletions(-)
 delete mode 100644 tests/qemumigparamsdata/unsupported.json
 delete mode 100644 tests/qemumigparamsdata/unsupported.reply
 delete mode 100644 tests/qemumigparamsdata/unsupported.xml

diff --git a/tests/qemumigparamsdata/unsupported.json 
b/tests/qemumigparamsdata/unsupported.json
deleted file mode 100644
index 0db3279e44..00
--- a/tests/qemumigparamsdata/unsupported.json
+++ /dev/null
@@ -1,3 +0,0 @@
-{
-
-}
diff --git a/tests/qemumigparamsdata/unsupported.reply 
b/tests/qemumigparamsdata/unsupported.reply
deleted file mode 100644
index 2b88ba10c3..00
--- a/tests/qemumigparamsdata/unsupported.reply
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "id": "libvirt-1",
-  "error": {
-"class": "CommandNotFound",
-"desc": "The command query-migrate-parameters has not been found"
-  }
-}
diff --git a/tests/qemumigparamsdata/unsupported.xml 
b/tests/qemumigparamsdata/unsupported.xml
deleted file mode 100644
index 8aa3abefc0..00
--- a/tests/qemumigparamsdata/unsupported.xml
+++ /dev/null
@@ -1,4 +0,0 @@
-
-  
-  
-
diff --git a/tests/qemumigparamstest.c b/tests/qemumigparamstest.c
index f445c92ff8..4ab40d9d2e 100644
--- a/tests/qemumigparamstest.c
+++ b/tests/qemumigparamstest.c
@@ -219,7 +219,6 @@ mymain(void)
 ret = -1; \
 } while (0)
 
-DO_TEST("unsupported");
 DO_TEST("empty");
 DO_TEST("basic");
 DO_TEST("tls");
-- 
2.32.0

[PATCH v2 3/6] qemuMonitorJSONGetDumpGuestMemoryCapability: Don't return early on CommandNotFound

[Michal Privoznik]

The qemuMonitorJSONGetDumpGuestMemoryCapability() command
executes 'query-dump-guest-memory-capability' command and returns
early if QEMU doesn't know the command. Well, the command was
introduced in QEMU release 2.0 (specifically in commit
v2.0.0-rc0~43^2~16) and since the minimum required version is
2.11.0 we can be sure that command will always exist.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_monitor_json.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 08cd535045..df3e14a153 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3666,11 +3666,6 @@ qemuMonitorJSONGetDumpGuestMemoryCapability(qemuMonitor 
*mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;
 
-if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
-ret = 0;
-goto cleanup;
-}
-
 if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
 goto cleanup;
 
-- 
2.32.0

[PATCH v2 4/6] qemuMonitorJSONGetKVMState: Don't return early on CommandNotFound

[Michal Privoznik]

The qemuMonitorJSONGetKVMState() command executes 'query-kvm'
command and returns early if QEMU doesn't know the command. Well,
the command was introduced in QEMU release 0.14 and since the
minimum required version is 2.11.0 we can be sure that command
will always exist.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_monitor_json.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index df3e14a153..70e3c70441 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6020,11 +6020,6 @@ int qemuMonitorJSONGetKVMState(qemuMonitor *mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;
 
-if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
-ret = 0;
-goto cleanup;
-}
-
 if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
 goto cleanup;
 
-- 
2.32.0

[PATCH v2 5/6] qemuMonitorJSONGetMemoryDeviceInfo: Don't return early on CommandNotFound

[Michal Privoznik]

The qemuMonitorJSONGetMemoryDeviceInfo() command executes
'query-memory-devices' command and returns early if QEMU
doesn't know the command. Well, the command was introduced in
QEMU release 2.1 (specifically in commit v2.1.0-rc0~41^2~9) and
since the minimum required version is 2.11.0 we can be sure that
command will always exist.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_monitor_json.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 70e3c70441..586b30763b 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -7788,11 +7788,6 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;
 
-if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
-ret = -2;
-goto cleanup;
-}
-
 if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
 goto cleanup;
 
-- 
2.32.0

Re: [PATCH v2 2/5] qapi: Add feature flags to enum members

[Markus Armbruster]

Kevin Wolf  writes:

> Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
>> This is quite similar to commit 84ab008687 "qapi: Add feature flags to
>> struct members", only for enums instead of structs.
>> 
>> Special feature flag 'deprecated' is silently ignored there.  This is
>> okay only because it will be implemented shortly.
>> 
>> Signed-off-by: Markus Armbruster 
>> Reviewed-by: Eric Blake 
>
> Should we have a test case for an invalid value for 'features'?

We have coverage, just not in every context.

struct context:

tests/qapi-schema/features-bad-type.json
tests/qapi-schema/features-deprecated-type.json
tests/qapi-schema/features-duplicate-name.json
tests/qapi-schema/features-if-invalid.json
tests/qapi-schema/features-missing-name.json
tests/qapi-schema/features-name-bad-type.json
tests/qapi-schema/features-no-list.json
tests/qapi-schema/features-unknown-key.json

struct member context:

tests/qapi-schema/features-member-bad-type.json
tests/qapi-schema/features-member-duplicate-name.json
tests/qapi-schema/features-member-if-invalid.json
tests/qapi-schema/features-member-missing-name.json
tests/qapi-schema/features-member-name-bad-type.json
tests/qapi-schema/features-member-no-list.json
tests/qapi-schema/features-member-unknown-key.json

These are basically the same, except for features-deprecated-type.json,
which makes sense only in struct context.

The other contexts are enum, union, alternate, command, event, and now
enum member.

My excuse for skipping contexts is that the errors come from
check_features() for all them.

Re: [PATCH 5/5] qemu: Prefer -numa cpu over -numa node,cpus=

[Igor Mammedov]

On Wed, 20 Oct 2021 16:15:29 +0200
Michal Prívozník  wrote:

> On 10/20/21 1:18 PM, Peter Krempa wrote:
> > On Wed, Oct 20, 2021 at 13:07:59 +0200, Michal Prívozník wrote:  
> >> On 10/6/21 3:32 PM, Igor Mammedov wrote:  
> >>> On Thu, 30 Sep 2021 14:08:34 +0200
> >>> Peter Krempa  wrote:  
> > 
> > [...]
> >   
> >> 2) In my experiments I try to mimic what libvirt does. Here's my cmd
> >> line:
> >>
> >> qemu-system-x86_64 \
> >> -S \
> >> -preconfig \
> >> -cpu host \
> >> -smp 120,sockets=2,dies=3,cores=4,threads=5 \
> >> -object 
> >> '{"qom-type":"memory-backend-memfd","id":"ram-node0","size":4294967296,"host-nodes":[0],"policy":"bind"}'
> >>  \
> >> -numa node,nodeid=0,memdev=ram-node0 \
> >> -no-user-config \
> >> -nodefaults \
> >> -no-shutdown \
> >> -qmp stdio
> >>
> >> and here is my QMP log:
> >>
> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 6}, 
> >> "package": "v6.1.0-1552-g362534a643"}, "capabilities": ["oob"]}}
> >>
> >> {"execute":"qmp_capabilities"}
> >> {"return": {}}
> >>
> >> {"execute":"query-hotpluggable-cpus"}
> >> {"return": [{"props": {"core-id": 3, "thread-id": 4, "die-id": 2, 
> >> "socket-id": 1}, "vcpus-count": 1, "type": "host-x86_64-cpu"}, {"props": 
> >> {"core-id": 3, "thread-id": 3, "die-id": 2, "socket-id": 1}, 
> >> "vcpus-count": 1, "type": "host-x86_64-cpu"}, {"props": {"core-id": 3, 
> >> "thread-id": 2, "die-id": 2, "socket-id": 1}, "vcpus-count": 1, "type": 
> >> "host-x86_64-cpu"}, {"props": {"core-id": 3, "thread-id": 1, "die-id": 2, 
> >> "socket-id": 1}, "vcpus-count": 1, "type": "host-x86_64-cpu"}, {"props": 
> >> {"core-id": 3, "thread-id": 0, "die-id": 2, "socket-id": 1}, 
> >> "vcpus-count": 1, "type": "host-x86_64-cpu"}, {"props": {"core-id": 2, 
> >> "thread-id": 4, "die-id": 2, "socket-id": 1}, "vcpus-count": 1, "type": 
> >> "host-x86_64-cpu"},
> >> 
> >> {"props": {"core-id": 0, "thread-id": 0, "die-id": 0, "socket-id": 0}, 
> >> "vcpus-count": 1, "type": "host-x86_64-cpu"}]}
> >>
> >>
> >> I can see that query-hotpluggable-cpus returns an array. Can I safely
> >> assume that vCPU ID == index in the array? I mean, if I did have -numa  
> > 
> > No, this assumption would be incorrect on the aforementioned PPC
> > platform where one entry in the returned array can describe multiple
> > cores.
> > 
> > qemuDomainFilterHotplugVcpuEntities is the code that cross-references
> > the libvirt "index" with the data returned by query-hotpluggable cpus.
> > 
> > The important bit is the 'vcpus-count' property. The code which deals
> > with hotplug is already fetching everything that's needed.  
> 
> Ah, I see. So my assumption would be correct if vcpus-count would be 1
> for all entries. If it isn't then I need to account for how much
only for some boards.
An entry in array describes an single entity that should be handled
as a single device by user (-device/plug/unplug/other mapping options)
(and the entity might have 1 or more vCPUs (threads) depending on
target arch/board).

> vcpus-count is in each entity. Fair enough. But
> qemuDomainFilterHotplugVcpuEntities() doesn't really do vCPU ID ->
> [socket, core, thread] translation, does it?
> 
> 
> But even if it did, I am still wondering what the purpose of this whole
> exercise is. QEMU won't be able to drop ID -> [socket, core, thread]
> mapping. The only thing it would be able to drop is a few lines of code
> handling command line. Am I missing something obvious?
I described in other email why QEMU is dropping cpu_idex on external
interfaces (it's possible to drop it internally too, but I don't see
much gain there vs effort such refactoring would require).

Sure thing, you can invent/maintain libvirt internal
 "vCPU ID" -> [topo props]
mapping if it's necessary.

However using just a "vCPU ID" will obscure topology information
from upper layers.
Maybe providing a list of CPUs as an external interface would be better,
then user can pick up which CPUs they wish to add/delete/assign/...
using items from that list.

> Michal
> 

Re: [PATCH v2 4/5] qapi: Implement deprecated-input={reject,crash} for enum values

[Markus Armbruster]

Eric Blake  writes:

> On Sat, Oct 09, 2021 at 02:09:43PM +0200, Markus Armbruster wrote:
>> This copies the code implementing the policy from qapi/qmp-dispatch.c
>> to qapi/qobject-input-visitor.c.  Tolerable, but if we acquire more
>> copes, we should look into factoring them out.
>
> copies

Fixing, thanks!

>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  docs/devel/qapi-code-gen.rst |  6 --
>>  qapi/compat.json |  3 ++-
>>  include/qapi/util.h  |  6 +-
>>  qapi/qapi-visit-core.c   | 18 +++---
>>  scripts/qapi/types.py| 17 -
>>  5 files changed, 42 insertions(+), 8 deletions(-)
>> 
>> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
>> index 00334e9fb8..006a6f4a9a 100644
>> --- a/docs/devel/qapi-code-gen.rst
>> +++ b/docs/devel/qapi-code-gen.rst
>> @@ -708,8 +708,10 @@ QEMU shows a certain behaviour.
>>  Special features
>>  
>>  
>> -Feature "deprecated" marks a command, event, or struct member as
>> -deprecated.  It is not supported elsewhere so far.
>> +Feature "deprecated" marks a command, event, struct or enum member as
>
> Do we want the comma before the conjunction?  (I've seen style guides
> that recommend it, and style guides that discourage it; while I tend
> to write by the former style, I usually don't care about the latter.
> Rather, switching styles mid-patch caught my attention).

With a comma there, we claim structs can be marked, which is actually
wrong.  Correct is "command, event, struct member, or enum member".

I'll rephrase to "marks a command, event, enum value, or struct member
deprecated."

>> +deprecated.  It is not supported elsewhere so far.  Interfaces so
>> +marked may be withdrawn in future releases in accordance with QEMU's
>> +deprecation policy.
>>  
>>  
>> +++ b/qapi/qapi-visit-core.c
>> @@ -393,7 +393,7 @@ static bool input_type_enum(Visitor *v, const char 
>> *name, int *obj,
>>  const QEnumLookup *lookup, Error **errp)
>>  {
>>  int64_t value;
>> -char *enum_str;
>> +g_autofree char *enum_str = NULL;
>
> Nice change while touching the code.  Is it worth mentioning in the
> commit message?

I figure it would be more distracting than useful.

>>  
>>  if (!visit_type_str(v, name, _str, errp)) {
>>  return false;
>> @@ -402,11 +402,23 @@ static bool input_type_enum(Visitor *v, const char 
>> *name, int *obj,
>>  value = qapi_enum_parse(lookup, enum_str, -1, NULL);
>>  if (value < 0) {
>>  error_setg(errp, QERR_INVALID_PARAMETER, enum_str);
>> -g_free(enum_str);
>>  return false;
>>  }
>>  
>> -g_free(enum_str);
>> +if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
>> +switch (v->compat_policy.deprecated_input) {
>> +case COMPAT_POLICY_INPUT_ACCEPT:
>> +break;
>> +case COMPAT_POLICY_INPUT_REJECT:
>> +error_setg(errp, "Deprecated value '%s' disabled by policy",
>> +   enum_str);
>> +return false;
>> +case COMPAT_POLICY_INPUT_CRASH:
>> +default:
>> +abort();
>> +}
>> +}
>> +
>>  *obj = value;
>>  return true;
>>  }
>
> Grammar fixes are minor, so:
>
> Reviewed-by: Eric Blake 

Thanks!

[PATCH v3 3/5] qapi: Move compat policy from QObject to generic visitor

[Markus Armbruster]

The next commit needs to access compat policy from the generic visitor
core.  Move it there from qobject input and output visitor.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 include/qapi/qobject-input-visitor.h  |  4 
 include/qapi/qobject-output-visitor.h |  4 
 include/qapi/visitor-impl.h   |  3 +++
 include/qapi/visitor.h|  9 +
 qapi/qapi-visit-core.c|  9 +
 qapi/qmp-dispatch.c   |  4 ++--
 qapi/qobject-input-visitor.c  | 14 +-
 qapi/qobject-output-visitor.c | 14 +-
 8 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/include/qapi/qobject-input-visitor.h 
b/include/qapi/qobject-input-visitor.h
index 8d69388810..95985e25e5 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -15,7 +15,6 @@
 #ifndef QOBJECT_INPUT_VISITOR_H
 #define QOBJECT_INPUT_VISITOR_H
 
-#include "qapi/qapi-types-compat.h"
 #include "qapi/visitor.h"
 
 typedef struct QObjectInputVisitor QObjectInputVisitor;
@@ -59,9 +58,6 @@ typedef struct QObjectInputVisitor QObjectInputVisitor;
  */
 Visitor *qobject_input_visitor_new(QObject *obj);
 
-void qobject_input_visitor_set_policy(Visitor *v,
-  CompatPolicyInput deprecated);
-
 /*
  * Create a QObject input visitor for @obj for use with keyval_parse()
  *
diff --git a/include/qapi/qobject-output-visitor.h 
b/include/qapi/qobject-output-visitor.h
index f2a2f92a00..2b1726baf5 100644
--- a/include/qapi/qobject-output-visitor.h
+++ b/include/qapi/qobject-output-visitor.h
@@ -15,7 +15,6 @@
 #define QOBJECT_OUTPUT_VISITOR_H
 
 #include "qapi/visitor.h"
-#include "qapi/qapi-types-compat.h"
 
 typedef struct QObjectOutputVisitor QObjectOutputVisitor;
 
@@ -54,7 +53,4 @@ typedef struct QObjectOutputVisitor QObjectOutputVisitor;
  */
 Visitor *qobject_output_visitor_new(QObject **result);
 
-void qobject_output_visitor_set_policy(Visitor *v,
-   CompatPolicyOutput deprecated);
-
 #endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 3b950f6e3d..72b6537bef 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -122,6 +122,9 @@ struct Visitor
 /* Must be set */
 VisitorType type;
 
+/* Optional */
+struct CompatPolicy compat_policy;
+
 /* Must be set for output visitors, optional otherwise. */
 void (*complete)(Visitor *v, void *opaque);
 
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b3c9ef7a81..dcb96018a9 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -16,6 +16,7 @@
 #define QAPI_VISITOR_H
 
 #include "qapi/qapi-builtin-types.h"
+#include "qapi/qapi-types-compat.h"
 
 /*
  * The QAPI schema defines both a set of C data types, and a QMP wire
@@ -477,6 +478,14 @@ bool visit_deprecated_accept(Visitor *v, const char *name, 
Error **errp);
  */
 bool visit_deprecated(Visitor *v, const char *name);
 
+/*
+ * Set policy for handling deprecated management interfaces.
+ *
+ * Intended use: call visit_set_policy(v, _policy) when
+ * visiting management interface input or output.
+ */
+void visit_set_policy(Visitor *v, CompatPolicy *policy);
+
 /*
  * Visit an enum value.
  *
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index a641adec51..066f77a26d 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -19,6 +19,10 @@
 #include "qapi/visitor-impl.h"
 #include "trace.h"
 
+/* Zero-initialization must result in default policy */
+QEMU_BUILD_BUG_ON(COMPAT_POLICY_INPUT_ACCEPT || COMPAT_POLICY_OUTPUT_ACCEPT);
+
+
 void visit_complete(Visitor *v, void *opaque)
 {
 assert(v->type != VISITOR_OUTPUT || v->complete);
@@ -153,6 +157,11 @@ bool visit_deprecated(Visitor *v, const char *name)
 return true;
 }
 
+void visit_set_policy(Visitor *v, CompatPolicy *policy)
+{
+v->compat_policy = *policy;
+}
+
 bool visit_is_input(Visitor *v)
 {
 return v->type == VISITOR_INPUT;
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 59600210ce..7e943a0af5 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -32,7 +32,7 @@ Visitor *qobject_input_visitor_new_qmp(QObject *obj)
 {
 Visitor *v = qobject_input_visitor_new(obj);
 
-qobject_input_visitor_set_policy(v, compat_policy.deprecated_input);
+visit_set_policy(v, _policy);
 return v;
 }
 
@@ -40,7 +40,7 @@ Visitor *qobject_output_visitor_new_qmp(QObject **result)
 {
 Visitor *v = qobject_output_visitor_new(result);
 
-qobject_output_visitor_set_policy(v, compat_policy.deprecated_output);
+visit_set_policy(v, _policy);
 return v;
 }
 
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 04b790412e..71b24a4429 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -14,7 +14,6 @@
 
 #include "qemu/osdep.h"
 #include 
-#include "qapi/compat-policy.h"
 #include 

Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

[Markus Armbruster]

Kevin Wolf  writes:

> Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
>> The next commit will add feature flags to enum members.  There's a
>> problem, though: query-qmp-schema shows an enum type's members as an
>> array of member names (SchemaInfoEnum member @values).  If it showed
>> an array of objects with a name member, we could simply add more
>> members to these objects.  Since it's just strings, we can't.
>> 
>> I can see three ways to correct this design mistake:
>> 
>> 1. Do it the way we should have done it, plus compatibility goo.
>> 
>>We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum.  Since
>>changing @values would be a compatibility break, add a new member
>>@members instead.
>> 
>>@values is now redundant.  In my testing, output of
>>qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).
>> 
>>We can deprecate @values now and drop it later.  This will break
>>outmoded clients.  Well-behaved clients such as libvirt are
>>expected to break cleanly.
>> 
>> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>> 
>>@values does not become redundant.  @members augments it.  Somewhat
>>cumbersome, but output of query-qmp-schema grows only as we make
>>enum members non-boring.
>> 
>>There is nothing to deprecate here.
>> 
>> 3. Versioned query-qmp-schema.
>> 
>>query-qmp-schema provides either @values or @members.  The QMP
>>client can select which version it wants.  There is no redundant
>>output.
>> 
>>We can deprecate old versions and eventually drop them.  This will
>>break outmoded clients.  Breaking cleanly is easier than for 1.
>> 
>>While 1 and 2 operate within the common rules for compatible
>>evolution apply (section "Compatibility considerations" in
>>docs/devel/qapi-code-gen.rst), 3 bypasses them.  Attractive when
>>operating within the rules is just too awkward.  Not the case here.
>> 
>> This commit implements 1.  Libvirt developers prefer it.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  qapi/introspect.json   | 21 +++--
>>  scripts/qapi/introspect.py | 18 ++
>>  2 files changed, 33 insertions(+), 6 deletions(-)
>
> docs/devel/qapi-code-gen.rst still says:
>
>   The SchemaInfo for an enumeration type has meta-type "enum" and
>   variant member "values".  The values are listed in no particular
>   order; clients must search the entire enum when learning whether a
>   particular value is supported.
>
>   Example: the SchemaInfo for MyEnum from section `Enumeration types`_ ::
>
>   { "name": "MyEnum", "meta-type": "enum",
> "values": [ "value1", "value2", "value3" ] }
>
> It probably needs an update.

It sure does.  Thanks!

[PATCH v3 4/5] qapi: Implement deprecated-input={reject, crash} for enum values

[Markus Armbruster]

This copies the code implementing the policy from qapi/qmp-dispatch.c
to qapi/qobject-input-visitor.c.  Tolerable, but if we acquire more
copies, we should look into factoring them out.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Tested-by: Peter Krempa 
Acked-by: Peter Krempa 
---
 qapi/compat.json   |  3 ++-
 include/qapi/util.h|  6 +-
 qapi/qapi-visit-core.c | 18 +++---
 scripts/qapi/types.py  | 17 -
 4 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/qapi/compat.json b/qapi/compat.json
index 1d2b76f00c..74a8493d3d 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -42,7 +42,8 @@
 # with feature 'deprecated'.  We may want to extend it to cover
 # semantic aspects, CLI, and experimental features.
 #
-# Limitation: not implemented for deprecated enumeration values.
+# Limitation: deprecated-output policy @hide is not implemented for
+# enumeration values.  They behave the same as with policy @accept.
 #
 # @deprecated-input: how to handle deprecated input (default 'accept')
 # @deprecated-output: how to handle deprecated output (default 'accept')
diff --git a/include/qapi/util.h b/include/qapi/util.h
index d7bfb30e25..257c600f99 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,9 +11,13 @@
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H
 
+/* QEnumLookup flags */
+#define QAPI_ENUM_DEPRECATED 1
+
 typedef struct QEnumLookup {
 const char *const *array;
-int size;
+const unsigned char *const flags;
+const int size;
 } QEnumLookup;
 
 const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 066f77a26d..49136ae88e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -393,7 +393,7 @@ static bool input_type_enum(Visitor *v, const char *name, 
int *obj,
 const QEnumLookup *lookup, Error **errp)
 {
 int64_t value;
-char *enum_str;
+g_autofree char *enum_str = NULL;
 
 if (!visit_type_str(v, name, _str, errp)) {
 return false;
@@ -402,11 +402,23 @@ static bool input_type_enum(Visitor *v, const char *name, 
int *obj,
 value = qapi_enum_parse(lookup, enum_str, -1, NULL);
 if (value < 0) {
 error_setg(errp, QERR_INVALID_PARAMETER, enum_str);
-g_free(enum_str);
 return false;
 }
 
-g_free(enum_str);
+if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
+switch (v->compat_policy.deprecated_input) {
+case COMPAT_POLICY_INPUT_ACCEPT:
+break;
+case COMPAT_POLICY_INPUT_REJECT:
+error_setg(errp, "Deprecated value '%s' disabled by policy",
+   enum_str);
+return false;
+case COMPAT_POLICY_INPUT_CRASH:
+default:
+abort();
+}
+}
+
 *obj = value;
 return true;
 }
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 831294fe42..ab2441adc9 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -38,6 +38,8 @@
 def gen_enum_lookup(name: str,
 members: List[QAPISchemaEnumMember],
 prefix: Optional[str] = None) -> str:
+max_index = c_enum_const(name, '_MAX', prefix)
+flags = ''
 ret = mcgen('''
 
 const QEnumLookup %(c_name)s_lookup = {
@@ -52,13 +54,26 @@ def gen_enum_lookup(name: str,
 ''',
  index=index, name=memb.name)
 ret += memb.ifcond.gen_endif()
+if 'deprecated' in (f.name for f in memb.features):
+flags += mcgen('''
+[%(index)s] = QAPI_ENUM_DEPRECATED,
+''',
+   index=index)
+
+if flags:
+ret += mcgen('''
+},
+.flags = (const unsigned char[%(max_index)s]) {
+''',
+ max_index=max_index)
+ret += flags
 
 ret += mcgen('''
 },
 .size = %(max_index)s
 };
 ''',
- max_index=c_enum_const(name, '_MAX', prefix))
+ max_index=max_index)
 return ret
 
 
-- 
2.31.1

[PATCH v3 2/5] qapi: Add feature flags to enum members

[Markus Armbruster]

This is quite similar to commit 84ab008687 "qapi: Add feature flags to
struct members", only for enums instead of structs.

Special feature flag 'deprecated' is silently ignored there.  This is
okay only because it will be implemented shortly.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 docs/devel/qapi-code-gen.rst  | 16 +-
 qapi/compat.json  |  2 ++
 qapi/introspect.json  |  5 -
 scripts/qapi/expr.py  |  3 ++-
 scripts/qapi/introspect.py|  5 +++--
 scripts/qapi/schema.py| 22 +--
 tests/qapi-schema/doc-good.json   |  5 -
 tests/qapi-schema/doc-good.out|  3 +++
 tests/qapi-schema/doc-good.txt|  3 +++
 .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
 tests/qapi-schema/qapi-schema-test.json   |  3 ++-
 tests/qapi-schema/qapi-schema-test.out|  1 +
 tests/qapi-schema/test-qapi.py|  1 +
 13 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index d267889d2c..4f12f57bfb 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -200,7 +200,9 @@ Syntax::
  '*if': COND,
  '*features': FEATURES }
 ENUM-VALUE = STRING
-   | { 'name': STRING, '*if': COND }
+   | { 'name': STRING,
+   '*if': COND,
+   '*features': FEATURES }
 
 Member 'enum' names the enum type.
 
@@ -706,8 +708,10 @@ QEMU shows a certain behaviour.
 Special features
 
 
-Feature "deprecated" marks a command, event, or struct member as
-deprecated.  It is not supported elsewhere so far.
+Feature "deprecated" marks a command, event, enum value, or struct
+member deprecated.  It is not supported elsewhere so far.  Interfaces
+so marked may be withdrawn in future releases in accordance with
+QEMU's deprecation policy.
 
 
 Naming rules and reserved names
@@ -1157,7 +1161,8 @@ and "variants".
 
 "members" is a JSON array describing the object's common members, if
 any.  Each element is a JSON object with members "name" (the member's
-name), "type" (the name of its type), and optionally "default".  The
+name), "type" (the name of its type), "features" (a JSON array of
+feature strings), and "default".  The latter two are optional.  The
 member is optional if "default" is present.  Currently, "default" can
 only have value null.  Other values are reserved for future
 extensions.  The "members" array is in no particular order; clients
@@ -1234,7 +1239,8 @@ The SchemaInfo for an enumeration type has meta-type 
"enum" and
 variant member "members".
 
 "members" is a JSON array describing the enumeration values.  Each
-element is a JSON object with member "name" (the member's name).  The
+element is a JSON object with member "name" (the member's name), and
+optionally "features" (a JSON array of feature strings).  The
 "members" array is in no particular order; clients must search the
 entire array when learning whether a particular value is supported.
 
diff --git a/qapi/compat.json b/qapi/compat.json
index ae3afc22df..1d2b76f00c 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -42,6 +42,8 @@
 # with feature 'deprecated'.  We may want to extend it to cover
 # semantic aspects, CLI, and experimental features.
 #
+# Limitation: not implemented for deprecated enumeration values.
+#
 # @deprecated-input: how to handle deprecated input (default 'accept')
 # @deprecated-output: how to handle deprecated output (default 'accept')
 #
diff --git a/qapi/introspect.json b/qapi/introspect.json
index f806bd7281..4a3b76464e 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -163,10 +163,13 @@
 #
 # @name: the member's name, as defined in the QAPI schema.
 #
+# @features: names of features associated with the member, in no
+#particular order.
+#
 # Since: 6.2
 ##
 { 'struct': 'SchemaInfoEnumMember',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str', '*features': [ 'str' ] } }
 
 ##
 # @SchemaInfoArray:
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 819ea6ad97..3cb389e875 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -472,7 +472,7 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> 
None:
   for m in members]
 for member in members:
 source = "'data' member"
-check_keys(member, info, source, ['name'], ['if'])
+check_keys(member, info, source, ['name'], ['if', 'features'])
 member_name = member['name']
 check_name_is_str(member_name, info, source)
 source = "%s '%s'" % (source, member_name)
@@ -483,6 +483,7 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> 
None:
  permit_upper=permissive,
  

[PATCH v3 1/5] qapi: Enable enum member introspection to show more than name

[Markus Armbruster]

The next commit will add feature flags to enum members.  There's a
problem, though: query-qmp-schema shows an enum type's members as an
array of member names (SchemaInfoEnum member @values).  If it showed
an array of objects with a name member, we could simply add more
members to these objects.  Since it's just strings, we can't.

I can see three ways to correct this design mistake:

1. Do it the way we should have done it, plus compatibility goo.

   We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum.  Since
   changing @values would be a compatibility break, add a new member
   @members instead.

   @values is now redundant.  In my testing, output of
   qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).

   We can deprecate @values now and drop it later.  This will break
   outmoded clients.  Well-behaved clients such as libvirt are
   expected to break cleanly.

2. Like 1, but omit "boring" elements of @member, and empty @member.

   @values does not become redundant.  @members augments it.  Somewhat
   cumbersome, but output of query-qmp-schema grows only as we make
   enum members non-boring.

   There is nothing to deprecate here.

3. Versioned query-qmp-schema.

   query-qmp-schema provides either @values or @members.  The QMP
   client can select which version it wants.  There is no redundant
   output.

   We can deprecate old versions and eventually drop them.  This will
   break outmoded clients.  Breaking cleanly is easier than for 1.

   While 1 and 2 operate within the common rules for compatible
   evolution apply (section "Compatibility considerations" in
   docs/devel/qapi-code-gen.rst), 3 bypasses them.  Attractive when
   operating within the rules is just too awkward.  Not the case here.

This commit implements 1.  Libvirt developers prefer it.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Tested-by: Peter Krempa 
Acked-by: Peter Krempa 
---
 docs/devel/qapi-code-gen.rst | 15 +++
 qapi/introspect.json | 21 +++--
 scripts/qapi/introspect.py   | 18 ++
 3 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index b2569de486..d267889d2c 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1231,14 +1231,21 @@ Example: the SchemaInfo for ['str'] ::
   "element-type": "str" }
 
 The SchemaInfo for an enumeration type has meta-type "enum" and
-variant member "values".  The values are listed in no particular
-order; clients must search the entire enum when learning whether a
-particular value is supported.
+variant member "members".
+
+"members" is a JSON array describing the enumeration values.  Each
+element is a JSON object with member "name" (the member's name).  The
+"members" array is in no particular order; clients must search the
+entire array when learning whether a particular value is supported.
 
 Example: the SchemaInfo for MyEnum from section `Enumeration types`_ ::
 
 { "name": "MyEnum", "meta-type": "enum",
-  "values": [ "value1", "value2", "value3" ] }
+  "members": [
+{ "name": "value1" },
+{ "name": "value2" },
+{ "name": "value3" }
+  ] }
 
 The SchemaInfo for a built-in type has the same name as the type in
 the QAPI schema (see section `Built-in Types`_), with one exception
diff --git a/qapi/introspect.json b/qapi/introspect.json
index 39bd303778..f806bd7281 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -142,14 +142,31 @@
 #
 # Additional SchemaInfo members for meta-type 'enum'.
 #
-# @values: the enumeration type's values, in no particular order.
+# @members: the enum type's members, in no particular order
+#   (since 6.2).
+#
+# @values: the enumeration type's member names, in no particular order.
+#  Redundant with @members.  Just for backward compatibility.
 #
 # Values of this type are JSON string on the wire.
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoEnum',
-  'data': { 'values': ['str'] } }
+  'data': { 'members': [ 'SchemaInfoEnumMember' ],
+'values': ['str'] } }
+
+##
+# @SchemaInfoEnumMember:
+#
+# An object member.
+#
+# @name: the member's name, as defined in the QAPI schema.
+#
+# Since: 6.2
+##
+{ 'struct': 'SchemaInfoEnumMember',
+  'data': { 'name': 'str' } }
 
 ##
 # @SchemaInfoArray:
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4c079ee627..6334546363 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -68,6 +68,7 @@
 # TypedDict constructs, so they are broadly typed here as simple
 # Python Dicts.
 SchemaInfo = Dict[str, object]
+SchemaInfoEnumMember = Dict[str, object]
 SchemaInfoObject = Dict[str, object]
 SchemaInfoObjectVariant = Dict[str, object]
 SchemaInfoObjectMember = Dict[str, object]
@@ -274,8 +275,16 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, 
object],
 obj['features'] = 

[PATCH RFC v3 5/5] block: Deprecate transaction type drive-backup

[Markus Armbruster]

Several moons ago, Vladimir posted

Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
Date: Wed,  5 May 2021 16:58:03 +0300
Message-Id: <20210505135803.67896-4-vsement...@virtuozzo.com>
https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html

with this

TODO: We also need to deprecate drive-backup transaction action..
But union members in QAPI doesn't support 'deprecated' feature. I tried
to dig a bit, but failed :/ Markus, could you please help with it? At
least by advice?

This is one way to resolve it.  Sorry it took so long.

John explored another way, namely adding feature flags to union
branches.  Could also be useful, say to add different features to
branches in multiple unions sharing the same tag enum.

Signed-off-by: Markus Armbruster 
---
 qapi/transaction.json | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index d175b5f863..381a2df782 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -54,6 +54,10 @@
 # @blockdev-snapshot-sync: since 1.1
 # @drive-backup: Since 1.6
 #
+# Features:
+# @deprecated: Member @drive-backup is deprecated.  Use member
+#  @blockdev-backup instead.
+#
 # Since: 1.1
 ##
 { 'enum': 'TransactionActionKind',
@@ -62,7 +66,7 @@
 'block-dirty-bitmap-disable', 'block-dirty-bitmap-merge',
 'blockdev-backup', 'blockdev-snapshot',
 'blockdev-snapshot-internal-sync', 'blockdev-snapshot-sync',
-'drive-backup' ] }
+{ 'name': 'drive-backup', 'features': [ 'deprecated' ] } ] }
 
 ##
 # @AbortWrapper:
-- 
2.31.1

[PATCH v3 0/5] qapi: Add feature flags to enum members

[Markus Armbruster]

PATCH 1+2 add feature flags to enum members.  Awkward due to an
introspection design mistake; see PATCH 1 for details.

PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
values.

Policy deprecated-output=hide is not implemented, because we can't
hide a value without hiding the entire member, which is almost
certainly more than the requester of this policy bargained for.
Perhaps we want a new policy deprecated-output=hide-or-else-crash to
help us catch unwanted use of deprecated enum values.  Perhaps we want
deprecated-output=hide to behave that way together with
deprecated-input=crash.  Or even always.  Thoughts?

PATCH 5 puts the new feature flags to use.  It's RFC because it makes
sense only on top of Vladimir's deprecation of drive-backup.  See its
commit message for a reference.

I prefer to commit new features together with a use outside tests/.
PATCH 5 adds such a use, but it's RFC, because it depends on
Vladimir's work.  Perhaps another use pops up.  I can delay this work
in the hope of a use becoming ready, but the feature flags work I have
in the pipeline will eventually force my hand.

v3:
* PATCH 1+2: Update qapi-code-gen.rst [Kevin, Eric]
* PATCH 4: Commit message typo [Eric], doc update moved to PATCH 2
* PATCH 5: Doc comment FIXME resolved [Kevin]

v2:
* Rebased with straightforward conflicts.
* PATCH 1-4: No longer RFC.
* PATCH 1: "Since" information fixed [Eric].  Commit message updated
  to reflect feedback.
* PATCH 2: Commit message amended to point out special feature flag
 'deprecated' is ignored at this stage.
* PATCH 4: Documentation updated.  Commit message tweaked.

Markus Armbruster (5):
  qapi: Enable enum member introspection to show more than name
  qapi: Add feature flags to enum members
  qapi: Move compat policy from QObject to generic visitor
  qapi: Implement deprecated-input={reject,crash} for enum values
  block: Deprecate transaction type drive-backup

 docs/devel/qapi-code-gen.rst  | 29 ++-
 qapi/compat.json  |  3 ++
 qapi/introspect.json  | 24 +--
 qapi/transaction.json |  6 +++-
 include/qapi/qobject-input-visitor.h  |  4 ---
 include/qapi/qobject-output-visitor.h |  4 ---
 include/qapi/util.h   |  6 +++-
 include/qapi/visitor-impl.h   |  3 ++
 include/qapi/visitor.h|  9 ++
 qapi/qapi-visit-core.c| 27 +++--
 qapi/qmp-dispatch.c   |  4 +--
 qapi/qobject-input-visitor.c  | 14 +
 qapi/qobject-output-visitor.c | 14 +
 scripts/qapi/expr.py  |  3 +-
 scripts/qapi/introspect.py| 19 +---
 scripts/qapi/schema.py| 22 --
 scripts/qapi/types.py | 17 ++-
 tests/qapi-schema/doc-good.json   |  5 +++-
 tests/qapi-schema/doc-good.out|  3 ++
 tests/qapi-schema/doc-good.txt|  3 ++
 .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
 tests/qapi-schema/qapi-schema-test.json   |  3 +-
 tests/qapi-schema/qapi-schema-test.out|  1 +
 tests/qapi-schema/test-qapi.py|  1 +
 24 files changed, 164 insertions(+), 62 deletions(-)

-- 
2.31.1

Re: [PATCH RFC v2 5/5] block: Deprecate transaction type drive-backup

[Markus Armbruster]

Kevin Wolf  writes:

> Am 11.10.2021 um 20:58 hat Eric Blake geschrieben:
>> On Sat, Oct 09, 2021 at 02:09:44PM +0200, Markus Armbruster wrote:
>> > Several moons ago, Vladimir posted
>> > 
>> > Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
>> > Date: Wed,  5 May 2021 16:58:03 +0300
>> > Message-Id: <20210505135803.67896-4-vsement...@virtuozzo.com>
>> > https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
>> > 
>> > with this
>> > 
>> > TODO: We also need to deprecate drive-backup transaction action..
>> > But union members in QAPI doesn't support 'deprecated' feature. I tried
>> > to dig a bit, but failed :/ Markus, could you please help with it? At
>> > least by advice?
>> > 
>> > This is one way to resolve it.  Sorry it took so long.
>> > 
>> > John explored another way, namely adding feature flags to union
>> > branches.  Could also be useful, say to add different features to
>> > branches in multiple unions sharing the same tag enum.
>> > 
>> > Signed-off-by: Markus Armbruster 
>> > ---
>> >  qapi/transaction.json | 5 -
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/qapi/transaction.json b/qapi/transaction.json
>> > index d175b5f863..0564a893b3 100644
>> > --- a/qapi/transaction.json
>> > +++ b/qapi/transaction.json
>> > @@ -54,6 +54,9 @@
>> >  # @blockdev-snapshot-sync: since 1.1
>> >  # @drive-backup: Since 1.6
>> >  #
>> > +# Features:
>> > +# @deprecated: Member @drive-backup is deprecated.  Use FIXME instead.
>> 
>> Obviously, we'd need to flesh this out ("'blockdev-backup' with proper
>> node names"? something else?) before dropping RFC on this patch.
>
> What does 'blockdev-backup' with improper node names look like?
>
> I think it's sufficient to say "Use @blockdev-backup instead", which is
> already documented to take a node/device name instead of a file name.

Sold!

>> And we'd want to edit docs/about/deprecated.rst to match.
>
> Yes.

My patch makes sense only together with Vladimir's patch to deprecate
the drive-backup command.  That one updates deprecated.rst.  The
combination of the two might need further tweaking, but let's not worry
about that now.

[PATCH v2 6/6] qemuMonitorJSONGetMigrationCapabilities: Don't return early on CommandNotFound

[Michal Privoznik]

The qemuMonitorJSONGetMigrationCapabilities() command executes
'query-migrate-capabilities' command and returns early if QEMU
doesn't know the command. Well, the command was introduced in
QEMU release 1.2 (specifically in commit v1.2.0-rc0~29^2~11) and
since the minimum required version is 2.11.0 we can be sure that
command will always exist.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_monitor_json.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 586b30763b..a7a980fccd 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6518,9 +6518,6 @@ qemuMonitorJSONGetMigrationCapabilities(qemuMonitor *mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 return -1;
 
-if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
-return 0;
-
 if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
 return -1;
 
-- 
2.32.0

Re: [PATCH v4 4/5] qemu: add librbd encryption engine

[Peter Krempa]

On Thu, Oct 21, 2021 at 11:40:20 +, Or Ozeri wrote:
>Thanks for reviewing all of my patches!
>I'm fine with you making any of the changes you suggested.
>So the only change I need to make is "specify what's happening in the
>storage driver"?
>Can you elaborate what do you mean by that?
>I can add something like:
>For librbd engine, the encryption happens inside the librbd storage
>driver, so block read/write requests coming in from the hypervisor (qemu)
>are plaintext,
>but encrypted by the storage driver before being persisted.
>Is this the kind of thing you were thinking about?

I meant the libvirt storage driver, which provides the storage
pool/volume functionality.

The code in the storage driver can create encrypted qcow2 images. (not
on RBD IIRC), but is using qemu-img to do that, which doesn't use the
same code we use in the qemu driver to instantiate VMs.

So while qemu-img could use the librbd encryption engine, the storage
driver code can't control it in such way.

Similarly the code doesn't share the 'qemu' validation/post-parse checks
so the librbd and luks2 combinations are not rejected.

Re: [PATCH v4 4/5] qemu: add librbd encryption engine

[Peter Krempa]

On Thu, Oct 07, 2021 at 14:21:20 -0500, Or Ozeri wrote:
> rbd encryption is new in qemu 6.1.0.
> This commit adds a new encryption engine property which
> allows the user to use this new encryption engine.
> 
> Signed-off-by: Or Ozeri 
> ---
>  docs/formatstorageencryption.html.in  |  7 +-
>  docs/schemas/storagecommon.rng|  1 +
>  src/conf/storage_encryption_conf.c|  2 +-
>  src/conf/storage_encryption_conf.h|  1 +
>  src/qemu/qemu_block.c | 26 +++
>  src/qemu/qemu_domain.c| 34 +
>  ...sk-network-rbd-encryption.x86_64-6.0.0.err |  1 +
>  ...-network-rbd-encryption.x86_64-latest.args | 45 
>  .../disk-network-rbd-encryption.xml   | 63 +
>  tests/qemuxml2argvtest.c  |  2 +
>  ...k-network-rbd-encryption.x86_64-latest.xml | 70 +++
>  tests/qemuxml2xmltest.c   |  1 +
>  12 files changed, 251 insertions(+), 2 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-6.0.0.err
>  create mode 100644 
> tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-encryption.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xml
> 
> diff --git a/docs/formatstorageencryption.html.in 
> b/docs/formatstorageencryption.html.in
> index 178fcd0d7c..02ee8f8ca3 100644
> --- a/docs/formatstorageencryption.html.in
> +++ b/docs/formatstorageencryption.html.in
> @@ -27,7 +27,12 @@
>The encryption tag supports an optional 
> engine
>tag, which allows selecting which component actually handles
>the encryption. Currently defined values of engine are
> -  qemu.
> +  qemu and librbd.
> +  Both qemu and librbd require using the qemu 
> driver.
> +  The librbd engine requires qemu version >= 6.1.0,
> +  and is only applicable for RBD network disks.
> +  If the engine tag is not specified, the qemu engine will 
> be
> +  used by default (assuming the qemu driver is used).

Okay, this is slightly better but it doesn't specify what's happening in
the storage driver.

>  
>  
>The encryption tag can currently contain a sequence of

[...]


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 354f65c6d5..13869dd79b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4814,6 +4814,40 @@ qemuDomainValidateStorageSource(virStorageSource *src,
>  if (src->encryption) {
>  switch (src->encryption->engine) {
>  case VIR_STORAGE_ENCRYPTION_ENGINE_QEMU:
> +switch ((virStorageEncryptionFormatType) 
> src->encryption->format) {
> +case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS:
> +case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW:
> +break;
> +
> +case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT:
> +case VIR_STORAGE_ENCRYPTION_FORMAT_LAST:
> +default:
> +
> virReportEnumRangeError(virStorageEncryptionFormatType,
> +src->encryption->format);
> +return -1;

This here is okay, because both are basically impossible.

> +}
> +
> +break;
> +case VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD:
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RBD_ENCRYPTION)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("librbd encryption is not supported by 
> this QEMU binary"));
> +return -1;
> +}
> +
> +switch ((virStorageEncryptionFormatType) 
> src->encryption->format) {
> +case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS:
> +break;
> +
> +case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT:
> +case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW:
> +case VIR_STORAGE_ENCRYPTION_FORMAT_LAST:
> +default:
> +
> virReportEnumRangeError(virStorageEncryptionFormatType,
> +src->encryption->format);

This creates an error message which is not very informative.
Specifically for VIR_STORAGE_ENCRYPTION_FORMAT_QCOW which is a
legitimate configuration we need a proper error message.

> +return -1;
> +}
> +
>  break;
>  case VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT:
>  case VIR_STORAGE_ENCRYPTION_ENGINE_LAST:

[...]

The test input files are okay. The output files will need some tweaking
after recent changes. I think adding the error message is trivial enough
so that I'll do it before pushing 

Re: [PATCH v4 5/5] conf: add luks2 encryption format

[Peter Krempa]

On Thu, Oct 07, 2021 at 14:21:21 -0500, Or Ozeri wrote:
> This commit extends libvirt XML configuration to support luks2 encryption 
> format.
> This means that  becomes valid.
> Currently librbd is the only engine that supports this new format.
> 
> Signed-off-by: Or Ozeri 
> ---
>  docs/formatstorageencryption.html.in | 12 +++-
>  docs/schemas/storagecommon.rng   |  1 +
>  src/conf/storage_encryption_conf.c   |  2 +-
>  src/conf/storage_encryption_conf.h   |  1 +
>  src/qemu/qemu_block.c|  5 +
>  src/qemu/qemu_domain.c   |  5 -
>  ...isk-network-rbd-encryption.x86_64-latest.args | 16 ++--
>  .../disk-network-rbd-encryption.xml  | 12 
>  ...disk-network-rbd-encryption.x86_64-latest.xml | 13 +
>  9 files changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/formatstorageencryption.html.in 
> b/docs/formatstorageencryption.html.in
> index 02ee8f8ca3..6cf1f94a9f 100644
> --- a/docs/formatstorageencryption.html.in
> +++ b/docs/formatstorageencryption.html.in

[...]

> @@ -121,6 +121,16 @@
>
>  
>  
> +"luks2" format
> +
> +  The luks2 format is currently supported only by the
> +  librbd engine, and can only be applied to RBD network 
> disks.
> +  luks2 encrypted RBD disks can be decrypted by the domain,
> +  but creation of such disks is currently not supported through libvirt.
> +  A single
> +  secret type='passphrase'... element is expected.
> +

As noted before this doesn't really tell what's happening in the storage
driver, so it will need some tweaking.


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 13869dd79b..8c2a5408da 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1228,7 +1228,8 @@ static bool
>  qemuDomainDiskHasEncryptionSecret(virStorageSource *src)
>  {
>  if (!virStorageSourceIsEmpty(src) && src->encryption &&
> -src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS &&
> +(src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS ||
> + src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS2) &&
>  src->encryption->nsecrets > 0)
>  return true;
>  
> @@ -4820,6 +4821,7 @@ qemuDomainValidateStorageSource(virStorageSource *src,
>  break;
>  
>  case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT:
> +case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS2:
>  case VIR_STORAGE_ENCRYPTION_FORMAT_LAST:
>  default:
>  
> virReportEnumRangeError(virStorageEncryptionFormatType,

Same problem as in previous patch. This creates an error message which
is not really descriptive.

Again both seem to be easy enough for me to do before pushing if you are
okay with it.

Reviewed-by: Peter Krempa 

(Note that I'm on PTO until Monday).

RE: [PATCH v4 4/5] qemu: add librbd encryption engine

[Or Ozeri]

Thanks for reviewing all of my patches!I'm fine with you making any of the changes you suggested.So the only change I need to make is "specify what's happening in the storage driver"?Can you elaborate what do you mean by that?I can add something like:For librbd engine, the encryption happens inside the librbd storage driver, so block read/write requests coming in from the hypervisor (qemu) are plaintext,but encrypted by the storage driver before being persisted.Is this the kind of thing you were thinking about?-"Peter Krempa"  wrote: ->To: "Or Ozeri" >From: "Peter Krempa" >Date: 10/21/2021 02:16PM>Cc: libvir-list@redhat.com, idryo...@gmail.com,>to.my.troc...@gmail.com, dan...@il.ibm.com>Subject: [EXTERNAL] Re: [PATCH v4 4/5] qemu: add librbd encryption>engine>>On Thu, Oct 07, 2021 at 14:21:20 -0500, Or Ozeri wrote:>> rbd encryption is new in qemu 6.1.0.>> This commit adds a new encryption engine property which>> allows the user to use this new encryption engine.>> >> Signed-off-by: Or Ozeri >> --->>  docs/formatstorageencryption.html.in  |  7 +->>  docs/schemas/storagecommon.rng|  1 +>>  src/conf/storage_encryption_conf.c|  2 +->>  src/conf/storage_encryption_conf.h|  1 +>>  src/qemu/qemu_block.c | 26 +++>>  src/qemu/qemu_domain.c| 34 +>>  ...sk-network-rbd-encryption.x86_64-6.0.0.err |  1 +>>  ...-network-rbd-encryption.x86_64-latest.args | 45 >>  .../disk-network-rbd-encryption.xml   | 63>+>>  tests/qemuxml2argvtest.c  |  2 +>>  ...k-network-rbd-encryption.x86_64-latest.xml | 70>+++>>  tests/qemuxml2xmltest.c   |  1 +>>  12 files changed, 251 insertions(+), 2 deletions(-)>>  create mode 100644>tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-6.0.0.err>>  create mode 100644>tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args>>  create mode 100644>tests/qemuxml2argvdata/disk-network-rbd-encryption.xml>>  create mode 100644>tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xm>l>> >> diff --git a/docs/formatstorageencryption.html.in>b/docs/formatstorageencryption.html.in>> index 178fcd0d7c..02ee8f8ca3 100644>> --- a/docs/formatstorageencryption.html.in>> +++ b/docs/formatstorageencryption.html.in>> @@ -27,7 +27,12 @@>>The encryption tag supports an optional>engine>>tag, which allows selecting which component actually handles>>the encryption. Currently defined values of>engine are>> -  qemu.>> +  qemu and librbd.>> +  Both qemu and librbd require using>the qemu driver.>> +  The librbd engine requires qemu version >=>6.1.0,>> +  and is only applicable for RBD network disks.>> +  If the engine tag is not specified, the qemu>engine will be>> +  used by default (assuming the qemu driver is used).>>Okay, this is slightly better but it doesn't specify what's happening>in>the storage driver.>>>  >>  >>The encryption tag can currently contain a>sequence of>>[...] diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c>> index 354f65c6d5..13869dd79b 100644>> --- a/src/qemu/qemu_domain.c>> +++ b/src/qemu/qemu_domain.c>> @@ -4814,6 +4814,40 @@>qemuDomainValidateStorageSource(virStorageSource *src,>>  if (src->encryption) {>>  switch (src->encryption->engine) {>>  case VIR_STORAGE_ENCRYPTION_ENGINE_QEMU:>> +switch ((virStorageEncryptionFormatType)>src->encryption->format) {>> +case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS:>> +case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW:>> +break;>> +>> +case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT:>> +case VIR_STORAGE_ENCRYPTION_FORMAT_LAST:>> +default:>> +>virReportEnumRangeError(virStorageEncryptionFormatType,>> +>src->encryption->format);>> +return -1;>>This here is okay, because both are basically impossible.>>> +}>> +>> +break;>> +case VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD:>> +if (!virQEMUCapsGet(qemuCaps,>QEMU_CAPS_RBD_ENCRYPTION)) {>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,>"%s",>> +   _("librbd encryption is not>supported by this QEMU binary"));>> +return -1;>> +}>> +>> +switch ((virStorageEncryptionFormatType)>src->encryption->format) {>> +case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS:>> +break;>> +>> +case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT:>> +case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW:>> +case VIR_STORAGE_ENCRYPTION_FORMAT_LAST:>> +

Re: [PATCH v4 3/5] conf: add encryption engine property

[Peter Krempa]

On Thu, Oct 07, 2021 at 14:21:19 -0500, Or Ozeri wrote:
> This commit extends libvirt XML configuration to support a custom encryption 
> engine.
> This means that   becomes valid.
> The only engine for now is qemu. However, a new engine (librbd) will be added 
> in an upcoming commit.
> If no engine is specified, qemu will be used (assuming qemu driver is used).
> 
> Signed-off-by: Or Ozeri 
> ---
>  docs/formatstorageencryption.html.in  |  6 +
>  docs/schemas/domainbackup.rng |  7 +
>  docs/schemas/storagecommon.rng|  7 +
>  src/conf/storage_encryption_conf.c| 27 ++-
>  src/conf/storage_encryption_conf.h|  9 +++
>  src/qemu/qemu_block.c |  2 ++
>  src/qemu/qemu_domain.c| 20 ++
>  tests/qemustatusxml2xmldata/upgrade-out.xml   |  6 ++---
>  tests/qemuxml2argvdata/disk-nvme.xml  |  2 +-
>  .../qemuxml2argvdata/encrypted-disk-usage.xml |  2 +-
>  tests/qemuxml2argvdata/luks-disks.xml |  4 +--
>  tests/qemuxml2argvdata/user-aliases.xml   |  2 +-
>  .../disk-slices.x86_64-latest.xml |  4 +--
>  tests/qemuxml2xmloutdata/encrypted-disk.xml   |  2 +-
>  .../luks-disks-source-qcow2.x86_64-latest.xml | 14 +-
>  .../qemuxml2xmloutdata/luks-disks-source.xml  | 10 +++
>  16 files changed, 100 insertions(+), 24 deletions(-)
> 
> diff --git a/docs/formatstorageencryption.html.in 
> b/docs/formatstorageencryption.html.in
> index 7215c307d7..178fcd0d7c 100644
> --- a/docs/formatstorageencryption.html.in
> +++ b/docs/formatstorageencryption.html.in
> @@ -23,6 +23,12 @@
>content of the encryption tag.  Other format values may be
>defined in the future.
>  
> +
> +  The encryption tag supports an optional 
> engine
> +  tag, which allows selecting which component actually handles
> +  the encryption. Currently defined values of engine are
> +  qemu.
> +

I'll add a note and possibly also a check that this works only in the
qemu VM driver, and not in the storage driver as this part of the docs
is shared between those two.

>  
>The encryption tag can currently contain a sequence of
>secret tags, each with mandatory attributes 
> type



> @@ -217,6 +223,7 @@ virStorageEncryptionParseNode(xmlNodePtr node,
>  xmlNodePtr *nodes = NULL;
>  virStorageEncryption *encdef = NULL;
>  virStorageEncryption *ret = NULL;
> +g_autofree char *engine_str = NULL;

This is unused. I'll remove it before pushing.

>  g_autofree char *format_str = NULL;
>  int n;
>  size_t i;



Reviewed-by: Peter Krempa 

Re: [PATCH 3/4] virt-aa-helper: Purge profile if corrupted

[Ján Tomko]

On a Wednesday in 2021, Ioanna Alifieraki wrote:

On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko  wrote:


On a Thursday in 2021, Ioanna Alifieraki wrote:
>This commit aims to address the bug reported in [1] and [2].
>If the profile is corrupted (0-size) the VM cannot be launched.
>To overcome this  check if the profile exists and if it has 0 size
>remove it and create it again.
>
>[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084
>[2] https://bugs.launchpad.net/bugs/1927519
>
>Signed-off-by: Ioanna Alifieraki 
>---
> src/security/virt-aa-helper.c | 23 +++
> 1 file changed, 23 insertions(+)
>
>diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>index 5ec0fb8807..5e13b29053 100644
>--- a/src/security/virt-aa-helper.c
>+++ b/src/security/virt-aa-helper.c
>@@ -1489,6 +1489,7 @@ main(int argc, char **argv)
> int rc = -1;
> char *profile = NULL;
> char *include_file = NULL;
>+off_t size;
>
> if (virGettextInitialize() < 0 ||
> virErrorInitialize() < 0) {
>@@ -1534,6 +1535,28 @@ main(int argc, char **argv)
> if (ctl->cmd == 'c' && virFileExists(profile))
> vah_error(ctl, 1, _("profile exists"));
>
>+/*
>+ * Rare cases can leave corrupted empty files behind breaking
>+ * the guest. An empty file is never correct as virt-aa-helper
>+ * would at least add the basic rules, therefore clean this up
>+ * for a proper refresh.
>+ */
>+
>+if (virFileExists(profile)) {
>+size = virFileLength(profile, -1);
>+if (size == 0) {
>+char temp;
>+vah_warning(_("Profile of 0 size detected, will attempt to remove 
and re-create it"));
>+temp = ctl->cmd;



Thank you very much for the feedback provided.
I'd like some clarifications.


Please do not pass 'ctl' to the helper functions. It makes it more
diffuclt to see what's going on.


Do you suggest this for both remove_profile and create_profile helper
functions ?
Not passing 'ctl' to the helper functions would make it difficult to
tell between
the different options.
For example, not passing ctl to  remove_profile we cannot tell if it's called
for D,R or P option.
I guess we could overcome this by passing an extra 'cmd' argument and
not the 'ctl'.


I think the remove_profile function is not necessary - you can just
call parserRemove directly in main() and unlink the file there.

But passing the uuid and cmd as arguments instead of ctl would at least
remove the need to replace ctl->cmd temporarily.


Regarding create_profile 'ctl' is used to check the 'ctl->dryrun' case.
Again, an alternative could be an extra argument for the dryrun.


Passing dryrun in ctl is ok.


What do you think?



>+ctl->cmd = 'P';
>+if ((rc = remove_profile(ctl, profile, include_file)) != 0)
>+vah_error(ctl, 1, _("could not remove corrupted 
profile"));

Easier to read as:
 if (parserRemove(ctl->uuid) < 0)
 goto cleanup;
 unlink(profile);


In that case, I question whether patch 2/4 is needed after all.
Adding the '-P' option targets (at least for the time being) that specific case.



That depends on whether this should be available to the users of
virt-aa-helper, or it's just for internal use when creating the profile.



>+ctl->cmd = temp;
>+if ((rc = create_profile(ctl, profile, include_file)) != 0)
>+vah_error(ctl, 1, _("could not re-create profile"));
>+}

Since we did not honour ctl->dryrun in the previous step,
it should be safe to call create_profile directly, without the helper
introduced in 1/4.



I'm a bit concerned here, as all tests in virt-aa-helper-test are run with the
dryrun option set.


Above, I suggested that creating the profile here does not need to check
for ctl->dryrun, because the remove_profile('P') call right above does
not check for it. That sounds wrong to me now - nothing should be purged
if the dryrun option was specified.

Also, is there a need to call create_profile here? For 'c' it would get
called twice. Adding a 'bool purged' variable and only creating the
profile
  if (ctl->cmd == 'c' || purged)
would get rid of the extra invocation.

Jano



Thanks,
Jo


Jano

>+}
>+
> if (ctl->append && ctl->newfile) {
> if (vah_add_file(, ctl->newfile, "rwk") != 0)
> goto cleanup;
>--
>2.17.1
>




signature.asc
Description: PGP signature

Re: [PATCH 4/4] virt-aa-helper: test: add test for new option -P

[Andrea Bolognani]

On Mon, Oct 11, 2021 at 07:59:47AM +0200, Christian Ehrhardt wrote:
> On Thu, Oct 7, 2021 at 7:25 PM Ioanna Alifieraki
> > +# For the next test to run apparmor needs to be installed and enabled.
> > +# In some environments (e.g. containers) even though apparmor is
> > +# installed, it is not enabled because securityfs is not mounted.
> > +# In those environments this test cannot run so skip it.
> > +# This test also needs to be run as root.
> > +if [ `eval id -u` = 0 ] && [ -x "$(command -v aa-enabled)" ] && [ `eval 
> > aa-enabled` = "Yes" ]; then
>
> This is great to be checked before causing a failure, but a question
> to the libvirt-CI experts,
> how doable (or not) would it be to get apparmor installed on those
> distro testbeds that support it?

Assuming the necessary packages are included in the container image,
what else is needed to have apparmor running? Does apparmor need to
be running in the host OS as well for it to work in containers? Does
the "securityfs" thing mentioned in the comment above need to be
passed through from the host OS?

Our CI pipeline uses containers running on the GitLab infrastructure.
I'm not sure what they're using as host OS, but if it's something
like Fedora for example I would expect that running apparmor would be
a problem. If special filesystems need to be passed to the container,
that's probably going to pose a challenge too.

> Are there any good pointers one would start to look at adapting those 
> testbeds?

The container images are generated from the Dockerfiles in
ci/containers, which in turn are generated using the lcitool utility
that's being developed as part of

  https://gitlab.com/libvirt/libvirt-ci/

If you want to include more packages, you should start by defining a
mapping for it in

  guests/lcitool/lcitool/ansible/vars/mappings.yml

and then adding it to

  guests/lcitool/lcitool/ansible/vars/projects/libvirt.yml

That's the short version. If you're looking for more information,
just let me know and I'll be happy to help :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 02/10] Revert "qemuxml2xmltest: Convert all acpi-hotplug control related tests to DO_TEST_CAPS_LATEST"

[Ján Tomko]

On a Thursday in 2021, Laine Stump wrote:

This reverts commit da896d440c7267e0b4575e4a3f2780bebf3fbfca.

Signed-off-by: Laine Stump 
---
...i-hotplug-bridge-disable.x86_64-latest.xml | 36 -
.../pc-i440fx-acpi-hotplug-bridge-disable.xml |  1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 36 -
.../pc-i440fx-acpi-hotplug-bridge-enable.xml  |  1 +
...cpi-root-hotplug-disable.x86_64-latest.xml | 33 
.../pc-i440fx-acpi-root-hotplug-disable.xml   |  1 +
...acpi-root-hotplug-enable.x86_64-latest.xml | 33 
.../pc-i440fx-acpi-root-hotplug-enable.xml|  1 +
...i-hotplug-bridge-disable.x86_64-latest.xml | 53 ---
.../q35-acpi-hotplug-bridge-disable.xml   |  1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 53 ---
.../q35-acpi-hotplug-bridge-enable.xml|  1 +
tests/qemuxml2xmltest.c   | 24 ++---
13 files changed, 24 insertions(+), 250 deletions(-)
delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml
create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-latest.xml
create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
delete mode 100644 
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml
delete mode 100644 
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.xml

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index b0a1212a54..d6effbdac6 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -424,12 +424,24 @@ mymain(void)
DO_TEST_NOCAPS("input-usbtablet");
DO_TEST_NOCAPS("misc-acpi");
DO_TEST("misc-disable-s3", QEMU_CAPS_PIIX_DISABLE_S3);
-DO_TEST_CAPS_LATEST("pc-i440fx-acpi-root-hotplug-disable");
-DO_TEST_CAPS_LATEST("pc-i440fx-acpi-root-hotplug-enable");
-DO_TEST_CAPS_LATEST("pc-i440fx-acpi-hotplug-bridge-disable");
-DO_TEST_CAPS_LATEST("pc-i440fx-acpi-hotplug-bridge-enable");
-DO_TEST_CAPS_LATEST("q35-acpi-hotplug-bridge-disable");
-DO_TEST_CAPS_LATEST("q35-acpi-hotplug-bridge-enable");
+DO_TEST("pc-i440fx-acpi-root-hotplug-disable",
+QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG);
+DO_TEST("pc-i440fx-acpi-root-hotplug-enable",
+QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG);


Please only do a partial revert here and leave the i440fx root hotplug
tests using the _LATEST form, since you're only reverting bridge hotplug
tests by the end of the series.

(The only conflict will be in context in the second-to-last patch.
 Well, and the NEWS addition, because I pushed a patch there in the
 meantime)

Jano


+DO_TEST("pc-i440fx-acpi-hotplug-bridge-disable",
+QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE);
+DO_TEST("pc-i440fx-acpi-hotplug-bridge-enable",
+QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE);


signature.asc
Description: PGP signature

Re: [libvirt PATCH v7 1/5] Add a PCI/PCIe device VPD Parser

[Daniel P . Berrangé]

On Wed, Oct 20, 2021 at 11:30:31AM +0300, Dmitrii Shcherbakov wrote:
> Add support for deserializing the binary PCI/PCIe VPD format and storing
> results in memory.
> 
> The VPD format is specified in "I.3. VPD Definitions" in PCI specs
> (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0
> notes, the PCI Local Bus and PCIe VPD formats are binary compatible
> and PCIe 4.0 merely started incorporating what was already present in
> PCI specs.
> 
> Linux kernel exposes a binary blob in the VPD format via sysfs since
> v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires
> a parser to interpret.
> 
> Signed-off-by: Dmitrii Shcherbakov 
> ---
>  build-aux/syntax-check.mk |   4 +-
>  po/POTFILES.in|   1 +
>  src/libvirt_private.syms  |  18 +
>  src/util/meson.build  |   1 +
>  src/util/virpcivpd.c  | 754 +++
>  src/util/virpcivpd.h  |  76 
>  src/util/virpcivpdpriv.h  |  83 
>  tests/meson.build |   1 +
>  tests/testutils.c |  35 ++
>  tests/testutils.h |   4 +
>  tests/virpcivpdtest.c | 809 ++
>  11 files changed, 1784 insertions(+), 2 deletions(-)
>  create mode 100644 src/util/virpcivpd.c
>  create mode 100644 src/util/virpcivpd.h
>  create mode 100644 src/util/virpcivpdpriv.h
>  create mode 100644 tests/virpcivpdtest.c
> 
> diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
> index cb12b64532..2a6e2f86a1 100644
> --- a/build-aux/syntax-check.mk
> +++ b/build-aux/syntax-check.mk
> @@ -775,9 +775,9 @@ sc_prohibit_windows_special_chars_in_filename:
>   { echo '$(ME): Windows special chars in filename not allowed' 1>&2; 
> echo exit 1; } || :
>  
>  sc_prohibit_mixed_case_abbreviations:
> - @prohibit='Pci|Usb|Scsi' \
> + @prohibit='Pci|Usb|Scsi|Vpd' \
>   in_vc_files='\.[ch]$$' \
> - halt='Use PCI, USB, SCSI, not Pci, Usb, Scsi' \
> + halt='Use PCI, USB, SCSI, VPD, not Pci, Usb, Scsi, Vpd' \
> $(_sc_search_regexp)
>  
>  # Require #include  in all files that call setlocale()
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index b554cf08ca..8a726f624e 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -302,6 +302,7 @@
>  @SRCDIR@src/util/virnvme.c
>  @SRCDIR@src/util/virobject.c
>  @SRCDIR@src/util/virpci.c
> +@SRCDIR@src/util/virpcivpd.c
>  @SRCDIR@src/util/virperf.c
>  @SRCDIR@src/util/virpidfile.c
>  @SRCDIR@src/util/virpolkit.c
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c5d788285e..444b51c880 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3576,6 +3576,24 @@ virVHBAManageVport;
>  virVHBAPathExists;
>  
>  
> +# util/virpcivpd.h
> +
> +virPCIVPDParse;
> +virPCIVPDParseVPDLargeResourceFields;
> +virPCIVPDParseVPDLargeResourceString;
> +virPCIVPDReadVPDBytes;
> +virPCIVPDResourceCustomCompareIndex;
> +virPCIVPDResourceCustomFree;
> +virPCIVPDResourceCustomUpsertValue;
> +virPCIVPDResourceFree;
> +virPCIVPDResourceGetFieldValueFormat;
> +virPCIVPDResourceIsValidTextValue;
> +virPCIVPDResourceROFree;
> +virPCIVPDResourceRONew;
> +virPCIVPDResourceRWFree;
> +virPCIVPDResourceRWNew;
> +virPCIVPDResourceUpdateKeyword;
> +
>  # util/virvsock.h
>  virVsockAcquireGuestCid;
>  virVsockSetGuestCid;
> diff --git a/src/util/meson.build b/src/util/meson.build
> index 05934f6841..24350a3e67 100644
> --- a/src/util/meson.build
> +++ b/src/util/meson.build
> @@ -105,6 +105,7 @@ util_sources = [
>'virutil.c',
>'viruuid.c',
>'virvhba.c',
> +  'virpcivpd.c',
>'virvsock.c',
>'virxml.c',
>  ]
> diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
> new file mode 100644
> index 00..a208224228
> --- /dev/null
> +++ b/src/util/virpcivpd.c

> +
> +bool
> +virPCIVPDResourceCustomCompareIndex(virPCIVPDResourceCustom *a, 
> virPCIVPDResourceCustom *b)
> +{
> +if (a == b) {
> +return true;
> +} else if (a == NULL || b == NULL) {
> +return false;
> +} else {
> +return a->idx == b->idx;
> +}
> +return true;
> +}

Subtle issue here - this function is used as a callback with GLib
and thus it needs to still use gboolean / TRUE / FALSE, because thats
a different sized data type to bool. This caused a horribly
non-deterministic problem on 32-bit builds.



> +
> +#endif /* __linux__ */

We needed an '#else' block here with stubs for non-Linux otherwise
we got link failures on Windows builds due to symbols not being
exported.


> +static int
> +mymain(void)
> +{
> +int ret = 0;
> +
> +if (virTestRun("Basic functionality of virPCIVPDResource ", 
> testPCIVPDResourceBasic, NULL) < 0)
> +ret = -1;
> +if (virTestRun("Custom field index comparison",
> +   testPCIVPDResourceCustomCompareIndex, NULL) < 0)
> +ret = -1;
> +if (virTestRun("Custom field value insertion and updates ",
> +   testPCIVPDResourceCustomUpsertValue, NULL) < 0)
> + 

[libvirt PATCH 08/10] Revert "qemu: command: add support for acpi-bridge-hotplug feature"

[Laine Stump]

This reverts commit bef0f0d8be6baa1d9359be208b53d6b8a37ddc95.

Conflicts:
 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args

  * this file had been renamed from its original, then renamed back,
which understandably confused git. It's being completely removed
here anyway, so the contents don't matter.

 tests/qemuxml2argvtest.c

  * change in context around removed chunk

Signed-off-by: Laine Stump 
---
 src/qemu/qemu_command.c   | 20 ---
 .../aarch64-acpi-hotplug-bridge-disable.err   |  1 -
 ...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 -
 .../pc-i440fx-acpi-hotplug-bridge-disable.err |  1 -
 .../q35-acpi-hotplug-bridge-disable.args  | 33 ---
 .../q35-acpi-hotplug-bridge-disable.err   |  1 -
 tests/qemuxml2argvtest.c  | 16 -
 7 files changed, 103 deletions(-)
 delete mode 100644 
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
 delete mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
 delete mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
 delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args
 delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 989ddcadb8..55d26d9af6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6415,7 +6415,6 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
 {
 virQEMUCaps *qemuCaps = priv->qemuCaps;
-int acpihp_br = def->pci_features[VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG];
 
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
 /* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6461,25 +6460,6 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
 }
 
-if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) {
-const char *pm_object = NULL;
-
-if (!qemuDomainIsQ35(def) &&
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE))
-pm_object = "PIIX4_PM";
-
-if (qemuDomainIsQ35(def) &&
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
-pm_object = "ICH9-LPC";
-
-if (pm_object != NULL) {
-virCommandAddArg(cmd, "-global");
-virCommandAddArgFormat(cmd, 
"%s.acpi-pci-hotplug-with-bridge-support=%s",
-   pm_object,
-   virTristateSwitchTypeToString(acpihp_br));
-}
-}
-
 return 0;
 }
 
diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err 
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
deleted file mode 100644
index 9f0a88b826..00
--- a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
+++ /dev/null
@@ -1 +0,0 @@
-unsupported configuration: acpi-bridge-hotplug is not available for 
architecture 'aarch64'
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
deleted file mode 100644
index a1e5dc85c7..00
--- a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
+++ /dev/null
@@ -1,31 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/tmp/lib/domain--1-i440fx \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
-XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
-XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
-QEMU_AUDIO_DRV=none \
-/usr/bin/qemu-system-x86_64 \
--name guest=i440fx,debug-threads=on \
--S \
--object 
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \
--machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \
--m 1024 \
--realtime mlock=off \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid 56f5055c-1b8d-490c-844a-ad646a1c \
--display none \
--no-user-config \
--nodefaults \
--chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off
 \
--mon chardev=charmonitor,id=monitor,mode=control \
--rtc base=utc \
--no-shutdown \
--no-acpi \
--global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \
--boot strict=on \
--usb \
--device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
--msg timestamp=on
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
deleted file mode 100644
index 8c09a3cd76..00
--- a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
+++ /dev/null
@@ -1 +0,0 @@
-unsupported configuration: acpi-bridge-hotplug is not available with this QEMU 
binary
diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args 
b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args
deleted file mode 

[libvirt PATCH 10/10] Revert "qemu: capablities: detect acpi-pci-hotplug-with-bridge-support"

[Laine Stump]

This reverts commit 58ba0f6a3d7342fba29edbbf2bb9cb5497c870e5.

Conflict:
  * src/qemu/qemu_capabilities.[ch]

Because other new cap flags had been added since the original
commit, reformatting was necessary to follow the "groups of
five" pattern.

  * tests.qemucapabilitiesdata/caps_6.2.0.x86_64.xml

This file was added after the original commit that we
are reverting, so had to be manually edited to remove
the two capabilities.

Signed-off-by: Laine Stump 
---
 src/qemu/qemu_capabilities.c  | 8 ++--
 src/qemu/qemu_capabilities.h  | 6 ++
 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 --
 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml  | 2 --
 15 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c5ebf5d156..96ecf27032 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -644,12 +644,10 @@ 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 */
-
-  /* 415 */
   "netdev.json", /* QEMU_CAPS_NETDEV_JSON */
   "chardev.json", /* QEMU_CAPS_CHARDEV_JSON */
+
+  /* 415 */
   "device.json", /* QEMU_CAPS_DEVICE_JSON */
   "query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */
 );
@@ -1470,7 +1468,6 @@ 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[] = {
@@ -1521,7 +1518,6 @@ 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 ac8a94a0af..796714438e 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -624,12 +624,10 @@ 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 */
-
-/* 415 */
 QEMU_CAPS_NETDEV_JSON, /* -netdev accepts JSON */
 QEMU_CAPS_CHARDEV_JSON, /* -chardev accepts JSON */
+
+/* 415 */
 QEMU_CAPS_DEVICE_JSON, /* -device accepts JSON */
 QEMU_CAPS_QUERY_DIRTY_RATE, /* accepts query-dirty-rate */
 
diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
index 6544b78730..559bf16766 100644
--- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml
@@ -167,7 +167,6 @@
   
   
   
-  
   2011000
   0
   43100288
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
index c66a140f8d..745110142f 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
@@ -179,7 +179,6 @@
   
   
   
-  
   2011090
   0
   43100289
diff --git 

Re: [PATCH v2 0/6] qemu_monitor_json: Assume existence of some commands

[Ján Tomko]

On a Thursday in 2021, Michal Privoznik wrote:

Technically a v2 of:

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

but I've cancelled sending in the middle of v1. Anyway, patch 1/6 is new
(yeah, I've noticed a test failing so I've cancelled sending v1).

Michal Prívozník (6):
 qemumigparamstest: Drop "unsupported" test case
 qemuMonitorJSONGetMigrationParams: Don't return early on
   CommandNotFound


qemuMonitorGetMigrationParams' comment can now lose the comment about QEMU 
support


 qemuMonitorJSONGetDumpGuestMemoryCapability: Don't return early on
   CommandNotFound
 qemuMonitorJSONGetKVMState: Don't return early on CommandNotFound
 qemuMonitorJSONGetMemoryDeviceInfo: Don't return early on
   CommandNotFound


qemuDomainUpdateMemoryDeviceInfo no longer needs to consider -2 and
the comment about -2 in qemuMonitorGetMemoryDeviceInfo's description
can be dropped


 qemuMonitorJSONGetMigrationCapabilities: Don't return early on
   CommandNotFound

src/qemu/qemu_monitor_json.c  | 23 ---
tests/qemumigparamsdata/unsupported.json  |  3 ---
tests/qemumigparamsdata/unsupported.reply |  7 ---
tests/qemumigparamsdata/unsupported.xml   |  4 
tests/qemumigparamstest.c |  1 -
5 files changed, 38 deletions(-)
delete mode 100644 tests/qemumigparamsdata/unsupported.json
delete mode 100644 tests/qemumigparamsdata/unsupported.reply
delete mode 100644 tests/qemumigparamsdata/unsupported.xml



Regardless of whether you squash in the above suggestions, resend them
separately or leave it up to future me:

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[libvirt PATCH 01/10] Revert "qemu: capabilities: Remove QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE"

[Laine Stump]

This reverts commit 618e8665db2e4c1a8e9a227045b99b48f6110c06.

This is the first in a series of 10 commits that revert (in reverse
order) the changes to add the 
switch to libvirt domain XML, which unfortunately needs to be removed
due to QEMU developers discovering a flaw with the design of the QEMU
commandline switch used to implement the libvirt switch that will
likely result in a new and different method of selecting hotplug
modes. Because the libvirt switch has not been in any official
releases of libvirt, we are still able to remove it completely, rather
than deprecating it.

The original commits began with commit
58ba0f6a3d7342fba29edbbf2bb9cb5497c870e5. The other original commit
IDs are documented in each revert commit.

Signed-off-by: Laine Stump 
---
 src/qemu/qemu_capabilities.c  | 4 +++-
 src/qemu/qemu_capabilities.h  | 3 ++-
 src/qemu/qemu_command.c   | 3 ++-
 src/qemu/qemu_validate.c  | 4 ++--
 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  | 1 +
 tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml  | 1 +
 17 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index cddd39924d..c5ebf5d156 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -644,10 +644,11 @@ 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 */
-  "netdev.json", /* QEMU_CAPS_NETDEV_JSON */
 
   /* 415 */
+  "netdev.json", /* QEMU_CAPS_NETDEV_JSON */
   "chardev.json", /* QEMU_CAPS_CHARDEV_JSON */
   "device.json", /* QEMU_CAPS_DEVICE_JSON */
   "query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */
@@ -1469,6 +1470,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[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index bb53d9ae46..ac8a94a0af 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -624,10 +624,11 @@ 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_NETDEV_JSON, /* -netdev accepts JSON */
 
 /* 415 */
+QEMU_CAPS_NETDEV_JSON, /* -netdev accepts JSON */
 QEMU_CAPS_CHARDEV_JSON, /* -chardev accepts JSON */
 QEMU_CAPS_DEVICE_JSON, /* -device accepts JSON */
 QEMU_CAPS_QUERY_DIRTY_RATE, /* accepts query-dirty-rate */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d6df50ec73..989ddcadb8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6464,7 +6464,8 @@ qemuBuildPMCommandLine(virCommand *cmd,
 if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) {
 const char *pm_object = NULL;
 
-if (!qemuDomainIsQ35(def))
+if (!qemuDomainIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE))
 pm_object = "PIIX4_PM";
 
 if (qemuDomainIsQ35(def) &&
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 3045e4b64b..1ffc261c58 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -196,8 +196,8 @@ 

[libvirt PATCH 00/10] Revert

[Laine Stump]

Starting with commit 58ba0f6a3d7342fba29edbbf2bb9cb5497c870e5, support
for overriding the default hotplug method for a guest using the
 subelement of / was added to
libvirt. This uses the QEMU global commandline switch:

  ICH9-LPC.acpi-pci-hotplug-with-bridge-support=

that was added to QEMU in qemu-6.1 (along with QEMU switching the
default hotplug type for new Q35-based machinetypes from "native PCIe"
to "ACPI").

Unfortunately, soon after we pushed the  patches
to libvirt (2 days after, to be exact), during a regular weekly
meeting I attend with some of the QEMU developers, along with bugs
that had been found in ACPI hotplug since it was made the default,
they were also discussing issues they'd found with the implementation
of the QEMU commandline switch to change back to native PCIe hotplug.

Apparently the current method used by
acpi-pci-hotplug-with-bridge-support is causing "confusion" in guests,
so they were talking about the possibility of changing what the switch
does (or replacing it), and suggested that libvirt should "hold off"
on supporting it for now. (oops) (they didn't know that we had just
pushed Ani's patches that used it)

Since the current QEMU option is doomed to either changed behavior
(which would result in a guest-visible ABI change) or full deprecation
and replacement, it would be better for libvirt to not use it at all,
and re-implement based on whatever replacement QEMU comes up
with.

Once a new XML element has appeared in a libvirt upstream
release, we are committed to keeping it there "forever". However,
since there has not yet been an upstream release since
 was added, there is still time to revert and
avoid the endless support/maintenance burden.

Not much time though! And that is the purpose of this patch
series. They revert, in reverse order, Ani's 4 original patches adding
the feature, along with Peter's 6 followup patches that correct a
logic error and streamline/beef up the unit tests.

(Note that it is still possible that QEMU would figure out a way out
of this without modifying their current flag in any significant way,
and in that case we could always "re-apply" the original
patches. However, if we don't revert before the next release (Andrea
has pointed out the freeze for RC1 is next Tuesday, and it would be
best to have it done before then), we will no longer have the option
to remove it.)

There was some conflict resolution necessary after the "git revert"
commands, due to other unrelated patches that changed test case data,
and due to a couple of Peter's patches also touching up the i440fx
pci-root-hotplug disabling test cases (which are *not* being reverted
- they work as advertised!).

Note that this series involves *re-adding* some things, just to remove
them again in a later revert - this is because Peter's patches
effectively reverted the addition of a QEMU capability flag that had
been added in Ani's original patches. If anyone would prefer, I can
squash those patches together so that the extra dance is eliminated
from the diffs; it just seemed more mechanical to do it this way
though.

A more detailed explanation of the issue:

Current Behavior of existing QEMU option


As far as I understand (and keep in mind that I have misunderstood and
misinterpreted this *at least* 4 separate times since it was first
explained to me!), the effect of this QEMU setting:

  -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=on

is to:

  1) enable ACPI hotplug (i.e. expose it to the guest)
  2) disable native PCIe hotplug (don't expose it to the guest)

By having only one of the two types of hotplug enabled, the intent is
to force the guest to use the still-enabled type of hotplug.

Unfortunately, after QEMU 6.1 was released with acpi-pci-hotplug=on as
the default for new machinetypes, problems were encountered with ACPI
hotplug, which caused more attention to be called to the QEMU switch,
and the people looking into that found that enabling ACPI and
disabling native PCIe hotplug doesn't necessarily have the desired
effect of causing the guest to use ACPI for hotplug (or maybe it was
the opposite direction). Instead, it "gets confused" (a very technical
term, I know. You can ask Julia or Michael for a definition :-)).

One possible fix that they talked about was changing the behavior of
ICH9-LPC.acpi-pci-hotplug-with-bridge-support:

Potential Change to Behavior of QEMU option
===

I know it's more complex than this (again, Julia or Michael can
explain), but my basic understanding of the way that they're currently
thinking of modifying the acpi-pci-hotplug-with-bridge-support option
is to have everything the same, *except* that when acpi-hotplug=off,
ACPI hotplug will *still be enabled* along with native PCIe hotplug;
but because guest OSes prefer native hotplug over ACPI, native PCIe
hotplug will be chosen in that case. (Presumably this change prevents
the "confusion" that is seen with 

[libvirt PATCH 05/10] Revert "qemuValidateDomainDefPCIFeature: Fix validation logic"

[Laine Stump]

This reverts commit bdc3e8f47be108fa552b72a6d913528869e61097.

Signed-off-by: Laine Stump 
---
 src/qemu/qemu_validate.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 1ffc261c58..d3b9691db5 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -179,6 +179,9 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def,
 int feature)
 {
 size_t i;
+bool q35Dom = qemuDomainIsQ35(def);
+bool q35cap = q35Dom && virQEMUCapsGet(qemuCaps,
+   QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE);
 
 if (def->features[feature] == VIR_TRISTATE_SWITCH_ABSENT)
 return 0;
@@ -195,9 +198,9 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def,
virArchToString(def->os.arch));
 return -1;
 }
-
-if ((qemuDomainIsQ35(def) && !virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE)) ||
-(!qemuDomainIsQ35(def) && !virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE))) {
+if (!q35cap &&
+!virQEMUCapsGet(qemuCaps,
+QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("acpi-bridge-hotplug is not available 
with this QEMU binary"));
 return -1;
-- 
2.31.1

[libvirt PATCH 03/10] Revert "qemuxml2argvtest: Add '-enable' variants for ACPI-hotplug related cases"

[Laine Stump]

This reverts commit 64146031058906804b3c5f519570fadc9ff4df48.

Conflicts:
 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args
 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-latest.args
 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args

These files are unrelated to the functionality we need to remove, so
they weren't removed, and the associated test cases weren't removed
from qemuxml2argvtest.c

Signed-off-by: Laine Stump 
---
 ...i-hotplug-bridge-enable.x86_64-latest.args | 34 -
 ...cpi-hotplug-bridge-enable.x86_64-6.0.0.err |  1 -
 ...i-hotplug-bridge-enable.x86_64-latest.args | 37 ---
 tests/qemuxml2argvtest.c  |  3 --
 4 files changed, 75 deletions(-)
 delete mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args
 delete mode 100644 
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err
 delete mode 100644 
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args

diff --git 
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args
 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args
deleted file mode 100644
index 77ef82f25b..00
--- 
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args
+++ /dev/null
@@ -1,34 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/tmp/lib/domain--1-i440fx \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
-XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
-XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
-/usr/bin/qemu-system-x86_64 \
--name guest=i440fx,debug-threads=on \
--S \
--object 
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-i440fx/master-key.aes"}'
 \
--machine 
pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
--cpu qemu64 \
--m 1024 \
--object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
--overcommit mem-lock=off \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid 56f5055c-1b8d-490c-844a-ad646a1c \
--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 \
--no-acpi \
--global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on \
--boot strict=on \
--device 
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
--audiodev id=audio1,driver=none \
--device 
'{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \
--sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--msg timestamp=on
diff --git 
a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err 
b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err
deleted file mode 100644
index 8c09a3cd76..00
--- a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err
+++ /dev/null
@@ -1 +0,0 @@
-unsupported configuration: acpi-bridge-hotplug is not available with this QEMU 
binary
diff --git 
a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args 
b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args
deleted file mode 100644
index 4ff07ad3b8..00
--- a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args
+++ /dev/null
@@ -1,37 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/tmp/lib/domain--1-q35 \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/tmp/lib/domain--1-q35/.local/share \
-XDG_CACHE_HOME=/tmp/lib/domain--1-q35/.cache \
-XDG_CONFIG_HOME=/tmp/lib/domain--1-q35/.config \
-/usr/bin/qemu-system-x86_64 \
--name guest=q35,debug-threads=on \
--S \
--object 
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-q35/master-key.aes"}'
 \
--machine 
pc-q35-2.5,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
--cpu qemu64 \
--m 1024 \
--object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
--overcommit mem-lock=off \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid 56f5055c-1b8d-490c-844a-ad646a1c \
--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 \
--no-acpi \
--global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=on \
--boot strict=on \
--device 
'{"driver":"i82801b11-bridge","id":"pci.1","bus":"pcie.0","addr":"0x1e"}' \
--device 
'{"driver":"pci-bridge","chassis_nr":2,"id":"pci.2","bus":"pci.1","addr":"0x0"}'
 \
--device 
'{"driver":"ioh3420","port":8,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x1"}'
 \
--device '{"driver":"qemu-xhci","id":"usb","bus":"pci.3","addr":"0x0"}' \
--audiodev id=audio1,driver=none \
--device 
'{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.2","addr":"0x1"}' \

[libvirt PATCH 04/10] Revert "qemuxml2argvtest: Use real-caps testing for 'acpi-hotplug-bridge-disable'"

[Laine Stump]

This reverts commit 2d20f0bb05175d18513220bccd1dbaa207cc4c6a.

Conflicts:
 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args

  the test output of these files was regenerated because the tests
  were changed upstream to use JSON on the commandline at a later
  commit than the commit being reverted here (where they were changed
  to use latest caps, but the patches to use JSON on the commandline
  hadn't been committed yet).

Signed-off-by: Laine Stump 
---
 ...> aarch64-acpi-hotplug-bridge-disable.err} |  0
 .../aarch64-acpi-hotplug-bridge-disable.xml   | 22 ++-
 ...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 
 ...pc-i440fx-acpi-hotplug-bridge-disable.err} |  0
 ...-hotplug-bridge-disable.x86_64-latest.args | 34 -
 .../q35-acpi-hotplug-bridge-disable.args  | 33 +
 .../q35-acpi-hotplug-bridge-disable.err   |  1 +
 ...-hotplug-bridge-disable.x86_64-latest.args | 37 ---
 tests/qemuxml2argvtest.c  | 17 +++--
 9 files changed, 99 insertions(+), 76 deletions(-)
 rename 
tests/qemuxml2argvdata/{aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err 
=> aarch64-acpi-hotplug-bridge-disable.err} (100%)
 create mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
 rename 
tests/qemuxml2argvdata/{q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err => 
pc-i440fx-acpi-hotplug-bridge-disable.err} (100%)
 delete mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args
 create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err
 delete mode 100644 
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args

diff --git 
a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err 
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
similarity index 100%
rename from 
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
rename to tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err
diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml 
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
index 47107e93f3..0d5f945bd7 100644
--- a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
+++ b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
@@ -1,13 +1,33 @@
 
-  test
+  i440fx
+  56f5055c-1b8d-490c-844a-ad646a1c
   1048576
+  1048576
   1
   
 hvm
+
   
   
 
   
 
   
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-aarch64
+
+  
+
+
+
+
+
+
+  
+
+  
 
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
new file mode 100644
index 00..a1e5dc85c7
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object 
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \
+-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \
+-m 1024 \
+-realtime mlock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 56f5055c-1b8d-490c-844a-ad646a1c \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \
+-boot strict=on \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
+-msg timestamp=on
diff --git 
a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
similarity index 100%
rename from 
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
rename to tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err
diff --git 
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
 
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
deleted file mode 100644
index 8a3e91c95e..00
--- 
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
+++ /dev/null
@@ -1,34 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/tmp/lib/domain--1-i440fx \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share 

[libvirt PATCH 02/10] Revert "qemuxml2xmltest: Convert all acpi-hotplug control related tests to DO_TEST_CAPS_LATEST"

[Laine Stump]

This reverts commit da896d440c7267e0b4575e4a3f2780bebf3fbfca.

Signed-off-by: Laine Stump 
---
 ...i-hotplug-bridge-disable.x86_64-latest.xml | 36 -
 .../pc-i440fx-acpi-hotplug-bridge-disable.xml |  1 +
 ...pi-hotplug-bridge-enable.x86_64-latest.xml | 36 -
 .../pc-i440fx-acpi-hotplug-bridge-enable.xml  |  1 +
 ...cpi-root-hotplug-disable.x86_64-latest.xml | 33 
 .../pc-i440fx-acpi-root-hotplug-disable.xml   |  1 +
 ...acpi-root-hotplug-enable.x86_64-latest.xml | 33 
 .../pc-i440fx-acpi-root-hotplug-enable.xml|  1 +
 ...i-hotplug-bridge-disable.x86_64-latest.xml | 53 ---
 .../q35-acpi-hotplug-bridge-disable.xml   |  1 +
 ...pi-hotplug-bridge-enable.x86_64-latest.xml | 53 ---
 .../q35-acpi-hotplug-bridge-enable.xml|  1 +
 tests/qemuxml2xmltest.c   | 24 ++---
 13 files changed, 24 insertions(+), 250 deletions(-)
 delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
 create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
 delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
 create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
 delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml
 create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
 delete mode 100644 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-latest.xml
 create mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
 delete mode 100644 
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
 create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml
 delete mode 100644 
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
 create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.xml

diff --git 
a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
 
b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
deleted file mode 100644
index 1b63ff9539..00
--- 
a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
+++ /dev/null
@@ -1,36 +0,0 @@
-
-  i440fx
-  56f5055c-1b8d-490c-844a-ad646a1c
-  1048576
-  1048576
-  1
-  
-hvm
-
-  
-  
-
-  
-
-  
-  
-qemu64
-  
-  
-  destroy
-  restart
-  destroy
-  
-/usr/bin/qemu-system-x86_64
-
-  
-
-
-
-
-
-
-  
-
-  
-
diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml 
b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
new file mode 12
index 00..8154897401
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
\ No newline at end of file
diff --git 
a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
 
b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
deleted file mode 100644
index f7b2cbb9ce..00
--- 
a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
+++ /dev/null
@@ -1,36 +0,0 @@
-
-  i440fx
-  56f5055c-1b8d-490c-844a-ad646a1c
-  1048576
-  1048576
-  1
-  
-hvm
-
-  
-  
-
-  
-
-  
-  
-qemu64
-  
-  
-  destroy
-  restart
-  destroy
-  
-/usr/bin/qemu-system-x86_64
-
-  
-
-
-
-
-
-
-  
-
-  
-
diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml 
b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
new file mode 12
index 00..6b9e5492f8
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
\ No newline at end of file
diff --git 
a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml
 
b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml
deleted file mode 100644
index 5a78fe638d..00
--- 
a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml
+++ /dev/null
@@ -1,33 +0,0 @@
-
-  i440fx
-  56f5055c-1b8d-490c-844a-ad646a1c
-  1048576
-  1048576
-  1
-  
-hvm
-
-  
-  
-qemu64
-  
-  
-  destroy
-  restart
-  destroy
-  
-/usr/bin/qemu-system-x86_64
-
-  
-
-
-  
-
-
-
-
-
-  
-
-  
-
diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml 
b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
new file mode 12
index 00..0570e40273
--- /dev/null
+++ 

[libvirt PATCH 07/10] Revert "NEWS: document new acpi pci hotplug config option"

[Laine Stump]

This reverts commit 5ee4f3e1d4f173f7e1b64b745ab9ef5dc8c8f393.

Signed-off-by: Laine Stump 
---
 NEWS.rst | 8 
 1 file changed, 8 deletions(-)

diff --git a/NEWS.rst b/NEWS.rst
index f3b9e5f0cb..bbed7a8228 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -32,14 +32,6 @@ v7.9.0 (unreleased)
 controller. The default behavior is unchanged (hotplug is
 allowed).
 
-  * Add a new global feature for controlling ACPI PCI hotplug on bridges
-
-A new  config option  under 
-sub-element have been added to allow users enable/disable ACPI based PCI
-hotplug for cold plugged bridges (that is, bridges that were present in the
-domain definition when the guest was first started).This setting is only
-applicable for x86 guests using qemu driver.
-
 * **Improvements**
 
   * Use of JSON syntax with ``-device`` with upcoming QEMU-6.2
-- 
2.31.1

[libvirt PATCH 06/10] Revert "qemuValidateDomainDefPCIFeature: un-break error messages"

[Laine Stump]

This reverts commit 7d074c56830c5d435f87667299cc102650dbbb4f.

Signed-off-by: Laine Stump 
---
 src/qemu/qemu_validate.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index d3b9691db5..0cb4542efd 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -194,7 +194,8 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def,
 case VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG:
 if (!ARCH_IS_X86(def->os.arch)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("acpi-bridge-hotplug is not available for 
architecture '%s'"),
+   _("acpi-bridge-hotplug is not available "
+   "for architecture '%s'"),
virArchToString(def->os.arch));
 return -1;
 }
@@ -202,7 +203,8 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def,
 !virQEMUCapsGet(qemuCaps,
 QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("acpi-bridge-hotplug is not available 
with this QEMU binary"));
+   _("acpi-bridge-hotplug is not available "
+   "with this QEMU binary"));
 return -1;
 }
 break;
-- 
2.31.1

[libvirt PATCH 09/10] Revert "conf: introduce support for acpi-bridge-hotplug feature"

[Laine Stump]

This reverts commit 7300ccc9b3eddb38306868534e7fc2d505a0a13c.

Signed-off-by: Laine Stump 
---
 docs/formatdomain.rst | 29 --
 docs/schemas/domaincommon.rng | 15 
 src/conf/domain_conf.c| 89 +--
 src/conf/domain_conf.h|  9 --
 src/qemu/qemu_validate.c  | 46 --
 .../aarch64-acpi-hotplug-bridge-disable.xml   | 33 ---
 .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 33 ---
 .../pc-i440fx-acpi-hotplug-bridge-enable.xml  | 33 ---
 .../q35-acpi-hotplug-bridge-disable.xml   | 47 --
 .../q35-acpi-hotplug-bridge-enable.xml| 47 --
 .../pc-i440fx-acpi-hotplug-bridge-disable.xml |  1 -
 .../pc-i440fx-acpi-hotplug-bridge-enable.xml  |  1 -
 .../q35-acpi-hotplug-bridge-disable.xml   |  1 -
 .../q35-acpi-hotplug-bridge-enable.xml|  1 -
 tests/qemuxml2xmltest.c   | 14 ---
 15 files changed, 1 insertion(+), 398 deletions(-)
 delete mode 100644 
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
 delete mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
 delete mode 100644 
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
 delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
 delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
 delete mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
 delete mode 12 
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
 delete mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml
 delete mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9bf59936e5..58768f7e5e 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1847,9 +1847,6 @@ Hypervisors may allow certain CPU / machine features to 
be toggled on/off.


  
- 
-   
- 
  
  
  
@@ -1945,32 +1942,6 @@ are:
passthrough Enable IOMMU mappings allowing PCI passthrough on, off; mode - 
optional string sync_pt or share_pt :since:`6.3.0`
=== == 
=== ==
 
-``pci``
-   Various PCI bus related features of the hypervisor.
-
-    
=
 === ==
-   Feature  Description
   Value   Since
-    
=
 === ==
-   acpi-bridge-hotplug  Enable ACPI based hotplug on the cold-plugged PCI 
bridges (pc) and pcie-root-ports (q35) (also see notes) on, off :since:`7.9.0`
-    
=
 === ==
-
-   Note: pc machine types (i440fx) have ACPI hotplug enabled by
-   default on cold plugged bridges (bridges that were present in the
-   VM's domain definition before it was started).  Disabling ACPI
-   hotplug leaves only SHPC hotplug enabled; many OSes don't
-   support SHPC hotplug, so this may have the effect of completely
-   disabling hotplug.
-
-   On q35 machinetypes earlier than pc-q35-6.1 (regardless of the QEMU
-   binary version), ACPI hotplug for cold plugged bridges is disabled
-   by default, and native PCIe hotplug is enabled instead. Enabling
-   ACPI hotplug will disable native PCIe hotplug.
-
-   Starting with the pc-q35-6.1 machinetype, ACPI hotplug is enabled
-   on cold plugged bridges by default while native PCIe hotplug is
-   disabled. In this case, disabling ACPI hotplug will re-enable PCIe
-   native hotplug.
-
 ``pmu``
Depending on the ``state`` attribute (values ``on``, ``off``, default 
``on``)
enable or disable the performance monitoring unit for the guest.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 26990c4d6d..f71e375a33 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -6169,9 +6169,6 @@
   
 
   
-  
-
-  
   
 
   
@@ -6403,18 +6400,6 @@
 
   
 
-  
-
-  
-
-  
-
-  
-
-  
-
-  
-
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 15228d1e38..2c794626f8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -172,7 +172,6 @@ VIR_ENUM_IMPL(virDomainFeature,
   "cfpc",
   "sbbc",
   "ibs",
- 

Re: [libvirt PATCH 00/10] Revert

[Ján Tomko]

On a Thursday in 2021, Laine Stump wrote:

[...]


Laine Stump (10):
 Revert "qemu: capabilities: Remove
   QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE"
 Revert "qemuxml2xmltest: Convert all acpi-hotplug control related
   tests to DO_TEST_CAPS_LATEST"
 Revert "qemuxml2argvtest: Add '-enable' variants for ACPI-hotplug
   related cases"
 Revert "qemuxml2argvtest: Use real-caps testing for
   'acpi-hotplug-bridge-disable'"
 Revert "qemuValidateDomainDefPCIFeature: Fix validation logic"
 Revert "qemuValidateDomainDefPCIFeature: un-break error messages"
 Revert "NEWS: document new acpi pci hotplug config option"
 Revert "qemu: command: add support for acpi-bridge-hotplug feature"
 Revert "conf: introduce support for acpi-bridge-hotplug feature"
 Revert "qemu: capablities: detect
   acpi-pci-hotplug-with-bridge-support"

NEWS.rst  |  8 --
docs/formatdomain.rst | 29 --
docs/schemas/domaincommon.rng | 15 


[...]


...i-hotplug-bridge-disable.x86_64-latest.xml | 53 ---
...pi-hotplug-bridge-enable.x86_64-latest.xml | 53 ---
tests/qemuxml2xmltest.c   | 10 +--
33 files changed, 9 insertions(+), 794 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 00/10] Revert

[Ani Sinha]

On Thu, 21 Oct 2021, Ján Tomko wrote:

> On a Thursday in 2021, Laine Stump wrote:
>
> [...]
>
> > Laine Stump (10):
> >  Revert "qemu: capabilities: Remove
> >QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE"
> >  Revert "qemuxml2xmltest: Convert all acpi-hotplug control related
> >tests to DO_TEST_CAPS_LATEST"
> >  Revert "qemuxml2argvtest: Add '-enable' variants for ACPI-hotplug
> >related cases"
> >  Revert "qemuxml2argvtest: Use real-caps testing for
> >'acpi-hotplug-bridge-disable'"
> >  Revert "qemuValidateDomainDefPCIFeature: Fix validation logic"
> >  Revert "qemuValidateDomainDefPCIFeature: un-break error messages"
> >  Revert "NEWS: document new acpi pci hotplug config option"
> >  Revert "qemu: command: add support for acpi-bridge-hotplug feature"
> >  Revert "conf: introduce support for acpi-bridge-hotplug feature"
> >  Revert "qemu: capablities: detect
> >acpi-pci-hotplug-with-bridge-support"
> >
> > NEWS.rst  |  8 --
> > docs/formatdomain.rst | 29 --
> > docs/schemas/domaincommon.rng | 15 
>
> [...]
>
> > ...i-hotplug-bridge-disable.x86_64-latest.xml | 53 ---
> > ...pi-hotplug-bridge-enable.x86_64-latest.xml | 53 ---
> > tests/qemuxml2xmltest.c   | 10 +--
> > 33 files changed, 9 insertions(+), 794 deletions(-)
>
>
> Reviewed-by: Ján Tomko 

Acked-by: Ani Sinha 

Re: [libvirt PATCH 00/10] Revert

[Ani Sinha]

On Thu, 21 Oct 2021, Laine Stump wrote:

> Starting with commit 58ba0f6a3d7342fba29edbbf2bb9cb5497c870e5, support
>
> for overriding the default hotplug method for a guest using the
>
>  subelement of / was added to
>
> libvirt. This uses the QEMU global commandline switch:
>
>
>
>   ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
>
>
>
> that was added to QEMU in qemu-6.1 (along with QEMU switching the
>
> default hotplug type for new Q35-based machinetypes from "native PCIe"
>
> to "ACPI").
>
>
>
> Unfortunately, soon after we pushed the  patches
>
> to libvirt (2 days after, to be exact), during a regular weekly
>
> meeting I attend with some of the QEMU developers, along with bugs
>
> that had been found in ACPI hotplug since it was made the default,
>
> they were also discussing issues they'd found with the implementation
>
> of the QEMU commandline switch to change back to native PCIe hotplug.

Just to be clear, this mess is for q35 side of that QEMU flag
(acpi-pci-hotplug-with-bridge-support) only, not i440fx
side. My libvirt patch implemented support in libvirt for both i440fx
side as well as the q35 side. The i440fx flag has existed for a very long
time in QEMU and hence it seems it is stable as there has been no known
issues around it reported (there was one issue I found in QEMU last year
which I since fixed). That being said, up until now there has been no
motivation from the upstream community to implement an equivalent switch in
libvirt for only i440fx. Clearly few people really care about that QEMU flag
on i440fx side and those who do can use the libvirt bypass mechanism to
pass the QEMU commandline directly. The main motiovation for my libvirt
work also was to allow users control/flip ACPI based hotplug on the q35 side
for pcie-root-ports in a libvirt friendly manner. i440fx side came along
the way for a free ride.

>
>
>
> Apparently the current method used by
>
> acpi-pci-hotplug-with-bridge-support is causing "confusion" in guests,
>
> so they were talking about the possibility of changing what the switch
>
> does (or replacing it), and suggested that libvirt should "hold off"
>
> on supporting it for now. (oops) (they didn't know that we had just
>
> pushed Ani's patches that used it)
>
>
>
> Since the current QEMU option is doomed to either changed behavior
>
> (which would result in a guest-visible ABI change) or full deprecation
>
> and replacement, it would be better for libvirt to not use it at all,
>
> and re-implement based on whatever replacement QEMU comes up
>
> with.
>
>
>
> Once a new XML element has appeared in a libvirt upstream
>
> release, we are committed to keeping it there "forever". However,
>
> since there has not yet been an upstream release since
>
>  was added, there is still time to revert and
>
> avoid the endless support/maintenance burden.
>
>
>
> Not much time though! And that is the purpose of this patch
>
> series. They revert, in reverse order, Ani's 4 original patches adding
>
> the feature, along with Peter's 6 followup patches that correct a
>
> logic error and streamline/beef up the unit tests.
>
>
>
> (Note that it is still possible that QEMU would figure out a way out
>
> of this without modifying their current flag in any significant way,
>
> and in that case we could always "re-apply" the original
>
> patches. However, if we don't revert before the next release (Andrea
>
> has pointed out the freeze for RC1 is next Tuesday, and it would be
>
> best to have it done before then), we will no longer have the option
>
> to remove it.)
>
>
>
> There was some conflict resolution necessary after the "git revert"
>
> commands, due to other unrelated patches that changed test case data,
>
> and due to a couple of Peter's patches also touching up the i440fx
>
> pci-root-hotplug disabling test cases (which are *not* being reverted
>
> - they work as advertised!).
>
>
>
> Note that this series involves *re-adding* some things, just to remove
>
> them again in a later revert - this is because Peter's patches
>
> effectively reverted the addition of a QEMU capability flag that had
>
> been added in Ani's original patches. If anyone would prefer, I can
>
> squash those patches together so that the extra dance is eliminated
>
> from the diffs; it just seemed more mechanical to do it this way
>
> though.
>
>
>
> A more detailed explanation of the issue:
>
>
>
> Current Behavior of existing QEMU option
>
> 
>
>
>
> As far as I understand (and keep in mind that I have misunderstood and
>
> misinterpreted this *at least* 4 separate times since it was first
>
> explained to me!), the effect of this QEMU setting:
>
>
>
>   -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=on
>
>
>
> is to:
>
>
>
>   1) enable ACPI hotplug (i.e. expose it to the guest)
>
>   2) disable native PCIe hotplug (don't expose it to the guest)
>
>
>
> By having only one of the two types of hotplug enabled, the intent is
>
> to force the guest to use