This capability tells whether qemu is capable of waking up the
guest from PM suspend.

Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_capabilities.c                  | 24 +++++++++++++++++++
 src/qemu/qemu_capabilities.h                  |  3 +++
 .../caps_4.0.0.riscv32.replies                | 11 +++++++++
 .../caps_4.0.0.riscv32.xml                    |  1 +
 .../caps_4.0.0.riscv64.replies                | 11 +++++++++
 .../caps_4.0.0.riscv64.xml                    |  1 +
 .../caps_4.0.0.x86_64.replies                 | 11 +++++++++
 .../caps_4.0.0.x86_64.xml                     |  1 +
 8 files changed, 63 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 5b3a2d0c33..389986d924 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -525,6 +525,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
               "scsi-disk.device_id",
               "virtio-pci-non-transitional",
               "query-current-machine",
+
+              /* 330 */
+              "wakeup-suspend-support",
     );
 
 
@@ -2769,6 +2772,25 @@ virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps,
 }
 
 
+static int
+virQEMUCapsProbeQMPCurrentMachine(virQEMUCapsPtr qemuCaps,
+                                  qemuMonitorPtr mon)
+{
+    qemuMonitorCurrentMachineInfo info = { 0 };
+
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CURRENT_MACHINE))
+        return 0;
+
+    if (qemuMonitorGetCurrentMachineInfo(mon, &info) < 0)
+        return -1;
+
+    if (info.wakeupSuspendSupport)
+        virQEMUCapsSet(qemuCaps, QEMU_CAPS_PM_WAKEUP_SUPPORT);
+
+    return 0;
+}
+
+
 bool
 virQEMUCapsCPUFilterFeatures(const char *name,
                              void *opaque)
@@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
         return -1;
     if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
         return -1;
+    if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0)
+        return -1;
 
     virQEMUCapsInitProcessCaps(qemuCaps);
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index d3f90e82a8..721092b91b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -509,6 +509,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
     QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL, /* virtio *-pci-{non-}transitional devices */
     QEMU_CAPS_QUERY_CURRENT_MACHINE, /* query-current-machine command */
 
+    /* 330 */
+    QEMU_CAPS_PM_WAKEUP_SUPPORT, /* domain has wake-up from suspend support */
+
     QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies
index c7dac44289..a4d5786301 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies
@@ -17756,3 +17756,14 @@
   ],
   "id": "libvirt-39"
 }
+
+{
+  "execute": "query-current-machine",
+  "id": "libvirt-40"
+}
+
+{
+  "return": {
+    "wakeup-suspend-support": true
+  }
+}
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml
index 9d20be128a..c82b0d5e74 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml
@@ -164,6 +164,7 @@
   <flag name='scsi-disk.device_id'/>
   <flag name='virtio-pci-non-transitional'/>
   <flag name='query-current-machine'/>
+  <flag name='wakeup-suspend-support'/>
   <version>3001091</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>0</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies
index 6fda8ad2d2..f2d209cd91 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies
@@ -17756,3 +17756,14 @@
   ],
   "id": "libvirt-39"
 }
+
+{
+  "execute": "query-current-machine",
+  "id": "libvirt-40"
+}
+
+{
+  "return": {
+    "wakeup-suspend-support": true
+  }
+}
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml
index a1a59e14cb..bf7f95e3b1 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml
@@ -164,6 +164,7 @@
   <flag name='scsi-disk.device_id'/>
   <flag name='virtio-pci-non-transitional'/>
   <flag name='query-current-machine'/>
+  <flag name='wakeup-suspend-support'/>
   <version>3001091</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>0</microcodeVersion>
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies
index aa9ee38c80..47ed75d359 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies
@@ -21052,6 +21052,17 @@
   }
 }
 
+{
+  "execute": "query-current-machine",
+  "id": "libvirt-50"
+}
+
+{
+  "return": {
+    "wakeup-suspend-support": true
+  }
+}
+
 {
   "execute": "qmp_capabilities",
   "id": "libvirt-1"
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
index 899c0cf7c8..df1b0da731 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
@@ -202,6 +202,7 @@
   <flag name='scsi-disk.device_id'/>
   <flag name='virtio-pci-non-transitional'/>
   <flag name='query-current-machine'/>
+  <flag name='wakeup-suspend-support'/>
   <version>3001050</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>43100758</microcodeVersion>
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
> This capability tells whether qemu is capable of waking up the
> guest from PM suspend.
> 
> Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_capabilities.c                  | 24 +++++++++++++++++++
>  src/qemu/qemu_capabilities.h                  |  3 +++
>  .../caps_4.0.0.riscv32.replies                | 11 +++++++++
>  .../caps_4.0.0.riscv32.xml                    |  1 +
>  .../caps_4.0.0.riscv64.replies                | 11 +++++++++
>  .../caps_4.0.0.riscv64.xml                    |  1 +
>  .../caps_4.0.0.x86_64.replies                 | 11 +++++++++
>  .../caps_4.0.0.x86_64.xml                     |  1 +
>  8 files changed, 63 insertions(+)
> 

[...]


>  bool
>  virQEMUCapsCPUFilterFeatures(const char *name,
>                               void *opaque)
> @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>          return -1;
>      if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
>          return -1;
> +    if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0)
> +        return -1;

This seems wrong by definition. The function is supposed to query
current machine, but the capability lookup code uses 'none' machine
type. IIUC the support for wakeup in some cases depends on the presence
of ACPI in the guest and thus really can't be cached this way.


> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies
> index aa9ee38c80..47ed75d359 100644
> --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies
> +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.replies
> @@ -21052,6 +21052,17 @@
>    }
>  }
>  
> +{
> +  "execute": "query-current-machine",
> +  "id": "libvirt-50"
> +}
> +
> +{
> +  "return": {
> +    "wakeup-suspend-support": true
> +  }

Did you gather this from an actual qemu run? I get the following when
using 'none' machine:

$ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -machine none
{"QMP": {"version": {"qemu": {"micro": 92, "minor": 1, "major": 3}, "package": "v4.0.0-rc2-24-gbcdb5721dd"}, "capabilities": ["oob"]}}
VNC server running on ::1:5900
{"execute":"qmp_capabilities"}
{"return": {}}
{"execute":"query-current-machine"}
{"return": {"wakeup-suspend-support": false}}

Same result is with this patch applied.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/11/19 10:39 AM, Peter Krempa wrote:
> On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
>> This capability tells whether qemu is capable of waking up the
>> guest from PM suspend.
>>
>> Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/qemu/qemu_capabilities.c                  | 24 +++++++++++++++++++
>>   src/qemu/qemu_capabilities.h                  |  3 +++
>>   .../caps_4.0.0.riscv32.replies                | 11 +++++++++
>>   .../caps_4.0.0.riscv32.xml                    |  1 +
>>   .../caps_4.0.0.riscv64.replies                | 11 +++++++++
>>   .../caps_4.0.0.riscv64.xml                    |  1 +
>>   .../caps_4.0.0.x86_64.replies                 | 11 +++++++++
>>   .../caps_4.0.0.x86_64.xml                     |  1 +
>>   8 files changed, 63 insertions(+)
>>
> 
> [...]
> 
> 
>>   bool
>>   virQEMUCapsCPUFilterFeatures(const char *name,
>>                                void *opaque)
>> @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>>           return -1;
>>       if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
>>           return -1;
>> +    if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0)
>> +        return -1;
> 
> This seems wrong by definition. The function is supposed to query
> current machine, but the capability lookup code uses 'none' machine
> type. IIUC the support for wakeup in some cases depends on the presence
> of ACPI in the guest and thus really can't be cached this way.

Yep, I was just about to write this but then got sidetracked.

So I see two options here:
1) Query if guest is capable of PM suspend at runtime. However, this has 
to be done in a clever way. It would need to be done not only in 
qemuProcessLaunch() (or some subordinate functions), it would need to be 
done in qemuProcessReconnect() too (to cover case when user is upgrading 
libvirt). For this, qemuMonitorConnect() looks like the best place - 
it's called from both locations and it also already contains some 
'runtime capabilities' querying. Except not really - the migration 
capabilities queried there are stored in @priv->migrationCaps rather 
than priv->qemuCaps.
Problem with storing runtime capabilities in priv->qemuCaps is that they 
would get formatted into status XML. And this opens whole different can 
of worms. For instance, this feature relies on 'query-commands' to see 
if 'query-current-machine' command is available. Well, we can't re-run 
'query-commands' in runtime because that might change the set of 
qemuCaps used when constructing cmd line.
Way around this is to have 'runtime only' capabilities, that would not 
be stored in status XML. At some point we will need those, but writing 
that infrastructure is not easy and a bit too much to ask for this small 
fix. Especially, when we have another option:

2) Issue 'query-current-machine' from qemuDomainPMSuspendForDuration(). 
In every run. This is a bit suboptimal, but how frequently do we expect 
this API to be called? Caching qemu capabilities is there to speed up 
decission process which makes sense if we'd be making the decission 1000 
times a second (e.g. building cmd line). With this approach the slowdown 
would be negligible. Of course, we would need to run the command in 
'silent' mode, i.e. not report any error if qemu doesn't know it.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/11/19 6:58 AM, Michal Privoznik wrote:
> On 4/11/19 10:39 AM, Peter Krempa wrote:
>> On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
>>> This capability tells whether qemu is capable of waking up the
>>> guest from PM suspend.
>>>
>>> Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>   src/qemu/qemu_capabilities.c                  | 24 
>>> +++++++++++++++++++
>>>   src/qemu/qemu_capabilities.h                  |  3 +++
>>>   .../caps_4.0.0.riscv32.replies                | 11 +++++++++
>>>   .../caps_4.0.0.riscv32.xml                    |  1 +
>>>   .../caps_4.0.0.riscv64.replies                | 11 +++++++++
>>>   .../caps_4.0.0.riscv64.xml                    |  1 +
>>>   .../caps_4.0.0.x86_64.replies                 | 11 +++++++++
>>>   .../caps_4.0.0.x86_64.xml                     |  1 +
>>>   8 files changed, 63 insertions(+)
>>>
>>
>> [...]
>>
>>
>>>   bool
>>>   virQEMUCapsCPUFilterFeatures(const char *name,
>>>                                void *opaque)
>>> @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr 
>>> qemuCaps,
>>>           return -1;
>>>       if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
>>>           return -1;
>>> +    if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0)
>>> +        return -1;
>>
>> This seems wrong by definition. The function is supposed to query
>> current machine, but the capability lookup code uses 'none' machine
>> type. IIUC the support for wakeup in some cases depends on the presence
>> of ACPI in the guest and thus really can't be cached this way.
>
> Yep, I was just about to write this but then got sidetracked.
>
> So I see two options here:
> 1) Query if guest is capable of PM suspend at runtime. However, this 
> has to be done in a clever way. It would need to be done not only in 
> qemuProcessLaunch() (or some subordinate functions), it would need to 
> be done in qemuProcessReconnect() too (to cover case when user is 
> upgrading libvirt). For this, qemuMonitorConnect() looks like the best 
> place - it's called from both locations and it also already contains 
> some 'runtime capabilities' querying. Except not really - the 
> migration capabilities queried there are stored in 
> @priv->migrationCaps rather than priv->qemuCaps.
> Problem with storing runtime capabilities in priv->qemuCaps is that 
> they would get formatted into status XML. And this opens whole 
> different can of worms. For instance, this feature relies on 
> 'query-commands' to see if 'query-current-machine' command is 
> available. Well, we can't re-run 'query-commands' in runtime because 
> that might change the set of qemuCaps used when constructing cmd line.
> Way around this is to have 'runtime only' capabilities, that would not 
> be stored in status XML. At some point we will need those, but writing 
> that infrastructure is not easy and a bit too much to ask for this 
> small fix. Especially, when we have another option:
>
> 2) Issue 'query-current-machine' from 
> qemuDomainPMSuspendForDuration(). In every run. This is a bit 
> suboptimal, but how frequently do we expect this API to be called? 
> Caching qemu capabilities is there to speed up decission process which 
> makes sense if we'd be making the decission 1000 times a second (e.g. 
> building cmd line). With this approach the slowdown would be 
> negligible. Of course, we would need to run the command in 'silent' 
> mode, i.e. not report any error if qemu doesn't know it.

I think the root issue in this series is that I made a mistake by calling
wake-up support a capability. In Libvirt terms, it really doesn't feel like
it - it can't be polled inside qemu_caps because runtime parameters
such as '--no-acpi' can change its value, but at the same time it feels
weird to populate qemuCaps in qemuProcess time.

All that said, I think a good solution would be (2). dompmsuspend isn't a
performance sensitive command, thus the extra time to execute the API
can be ignored. Also, we can still check for QEMU_CAPS_QUERY_CURRENT_MACHINE
inside qemuDomainPMSuspendForDuration to avoid firing up an error in a
QEMU version that doesn't know the API.


Thanks,


DHB



>
> Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
> <snip/>
 >
> All that said, I think a good solution would be (2). dompmsuspend isn't a
> performance sensitive command, thus the extra time to execute the API
> can be ignored. Also, we can still check for 
> QEMU_CAPS_QUERY_CURRENT_MACHINE
> inside qemuDomainPMSuspendForDuration to avoid firing up an error in a
> QEMU version that doesn't know the API.

That's the thing. We can't. The capability doesn't exist yet. So every 
domain that is currently running thinks that qemu doesn't have the 
capability. And when updating libvirt to say next release only freshly 
started domains might/will have the capability. For all already running 
domains libvirt thinks the capability is not there. Only freshly started 
domains will get the capability. IOW, upgrading libvirt won't help you 
unless you restart your domains (might not want to do that).

What I'm suggesting is to just execute the command, do not call 
qemuMonitorJSONCheckReply() and try to fetch info from the reply anyway. 
For instance, this is how qemuMonitorJSONGetKVMState() works.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Apr 11, 2019 at 13:37:49 +0200, Michal Privoznik wrote:
> On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
> > <snip/>
> >
> > All that said, I think a good solution would be (2). dompmsuspend isn't a
> > performance sensitive command, thus the extra time to execute the API
> > can be ignored. Also, we can still check for
> > QEMU_CAPS_QUERY_CURRENT_MACHINE
> > inside qemuDomainPMSuspendForDuration to avoid firing up an error in a
> > QEMU version that doesn't know the API.
> 
> That's the thing. We can't. The capability doesn't exist yet. So every
> domain that is currently running thinks that qemu doesn't have the
> capability. And when updating libvirt to say next release only freshly
> started domains might/will have the capability. For all already running
> domains libvirt thinks the capability is not there. Only freshly started
> domains will get the capability. IOW, upgrading libvirt won't help you
> unless you restart your domains (might not want to do that).

If you just go ahead with the PM suspend in case the capability bit for
the presence of query-current-machine is not present you just will keep
the existing behaviour. I think that is perfectly tolerable.

Error should be reported only if query-current-machine is supported and
it returns false for wakeup-suspend-support. (Obviously also if our caps
indicate that query-current-machine is supported but returns an error).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/11/19 8:43 AM, Peter Krempa wrote:
> On Thu, Apr 11, 2019 at 13:37:49 +0200, Michal Privoznik wrote:
>> On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
>>> <snip/>
>>>
>>> All that said, I think a good solution would be (2). dompmsuspend isn't a
>>> performance sensitive command, thus the extra time to execute the API
>>> can be ignored. Also, we can still check for
>>> QEMU_CAPS_QUERY_CURRENT_MACHINE
>>> inside qemuDomainPMSuspendForDuration to avoid firing up an error in a
>>> QEMU version that doesn't know the API.
>> That's the thing. We can't. The capability doesn't exist yet. So every
>> domain that is currently running thinks that qemu doesn't have the
>> capability. And when updating libvirt to say next release only freshly
>> started domains might/will have the capability. For all already running
>> domains libvirt thinks the capability is not there. Only freshly started
>> domains will get the capability. IOW, upgrading libvirt won't help you
>> unless you restart your domains (might not want to do that).
> If you just go ahead with the PM suspend in case the capability bit for
> the presence of query-current-machine is not present you just will keep
> the existing behaviour. I think that is perfectly tolerable.
>
> Error should be reported only if query-current-machine is supported and
> it returns false for wakeup-suspend-support. (Obviously also if our caps
> indicate that query-current-machine is supported but returns an error).

I agree with this approach.

Michal, do you agree agree with this? If so, let me know if you plan
to re-spin this series again. Otherwise I can do it.


Thanks,


DHB

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/17/19 8:27 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 4/11/19 8:43 AM, Peter Krempa wrote:
>> On Thu, Apr 11, 2019 at 13:37:49 +0200, Michal Privoznik wrote:
>>> On 4/11/19 12:12 PM, Daniel Henrique Barboza wrote:
>>>> <snip/>
>>>>
>>>> All that said, I think a good solution would be (2). dompmsuspend 
>>>> isn't a
>>>> performance sensitive command, thus the extra time to execute the API
>>>> can be ignored. Also, we can still check for
>>>> QEMU_CAPS_QUERY_CURRENT_MACHINE
>>>> inside qemuDomainPMSuspendForDuration to avoid firing up an error in a
>>>> QEMU version that doesn't know the API.
>>> That's the thing. We can't. The capability doesn't exist yet. So every
>>> domain that is currently running thinks that qemu doesn't have the
>>> capability. And when updating libvirt to say next release only freshly
>>> started domains might/will have the capability. For all already running
>>> domains libvirt thinks the capability is not there. Only freshly started
>>> domains will get the capability. IOW, upgrading libvirt won't help you
>>> unless you restart your domains (might not want to do that).
>> If you just go ahead with the PM suspend in case the capability bit for
>> the presence of query-current-machine is not present you just will keep
>> the existing behaviour. I think that is perfectly tolerable.
>>
>> Error should be reported only if query-current-machine is supported and
>> it returns false for wakeup-suspend-support. (Obviously also if our caps
>> indicate that query-current-machine is supported but returns an error).
> 
> I agree with this approach.
> 
> Michal, do you agree agree with this? If so, let me know if you plan
> to re-spin this series again. Otherwise I can do it.

My only concern is that we don't recreate capabilities for running 
domains. That means that domains which are already running won't benefit 
from this feature. On the other hand, I guess this is almost a corner 
case and thus probably not worth spending too much time on it.

So I say go for it. I don't plan to post patches anytime soon (burried 
under tonns of other stuff) so if you want to post the patches, please do.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Apr 11, 2019 at 11:58:04 +0200, Michal Privoznik wrote:
> On 4/11/19 10:39 AM, Peter Krempa wrote:
> > On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
> > > This capability tells whether qemu is capable of waking up the
> > > guest from PM suspend.
> > > 
> > > Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >   src/qemu/qemu_capabilities.c                  | 24 +++++++++++++++++++
> > >   src/qemu/qemu_capabilities.h                  |  3 +++
> > >   .../caps_4.0.0.riscv32.replies                | 11 +++++++++
> > >   .../caps_4.0.0.riscv32.xml                    |  1 +
> > >   .../caps_4.0.0.riscv64.replies                | 11 +++++++++
> > >   .../caps_4.0.0.riscv64.xml                    |  1 +
> > >   .../caps_4.0.0.x86_64.replies                 | 11 +++++++++
> > >   .../caps_4.0.0.x86_64.xml                     |  1 +
> > >   8 files changed, 63 insertions(+)
> > > 
> > 
> > [...]
> > 
> > 
> > >   bool
> > >   virQEMUCapsCPUFilterFeatures(const char *name,
> > >                                void *opaque)
> > > @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> > >           return -1;
> > >       if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
> > >           return -1;
> > > +    if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0)
> > > +        return -1;
> > 
> > This seems wrong by definition. The function is supposed to query
> > current machine, but the capability lookup code uses 'none' machine
> > type. IIUC the support for wakeup in some cases depends on the presence
> > of ACPI in the guest and thus really can't be cached this way.
> 
> Yep, I was just about to write this but then got sidetracked.
> 
> So I see two options here:
> 1) Query if guest is capable of PM suspend at runtime. However, this has to
> be done in a clever way. It would need to be done not only in
> qemuProcessLaunch() (or some subordinate functions), it would need to be
> done in qemuProcessReconnect() too (to cover case when user is upgrading

If you are upgrading the capabilities parsed from the status XML won't
have the bit representing the presence of the query command. This means
that new libvirt would still attempt the suspend ignoring whether it is
supported or not. I don't think it makes sense to re-detect presence of
stuff which was done prior to upgrading.

> libvirt). For this, qemuMonitorConnect() looks like the best place - it's
> called from both locations and it also already contains some 'runtime
> capabilities' querying. Except not really - the migration capabilities

I was considering whether it's a good idea to keep runtime only
capabilities in the qemuCaps array. I can find both arguments for it and
against it. The convenient part is that it's automatically stored in the
status XML.

> queried there are stored in @priv->migrationCaps rather than priv->qemuCaps.
> Problem with storing runtime capabilities in priv->qemuCaps is that they
> would get formatted into status XML. And this opens whole different can of

It is strongly desired to store it in the status XML. That way you don't
have to re-query.

> worms. For instance, this feature relies on 'query-commands' to see if
> 'query-current-machine' command is available. Well, we can't re-run
> 'query-commands' in runtime because that might change the set of qemuCaps
> used when constructing cmd line.

Why bother? If qemu was started prior to the support being added I don't
see a compelling reason to requery everything. In most cases it would be
also wrong to do so.

> Way around this is to have 'runtime only' capabilities, that would not be
> stored in status XML. At some point we will need those, but writing that
> infrastructure is not easy and a bit too much to ask for this small fix.
> Especially, when we have another option:
> 
> 2) Issue 'query-current-machine' from qemuDomainPMSuspendForDuration(). In
> every run. This is a bit suboptimal, but how frequently do we expect this

I don't think it's suboptimal. Normally users don't use PM suspend for
the VM so you can argue that this saves one qemu call in all the cases
where PM suspend is never used.

> API to be called? Caching qemu capabilities is there to speed up decission
> process which makes sense if we'd be making the decission 1000 times a
> second (e.g. building cmd line). With this approach the slowdown would be
> negligible. Of course, we would need to run the command in 'silent' mode,
> i.e. not report any error if qemu doesn't know it.

Or you can still use the capability bit whether the query command is
available.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/11/19 12:19 PM, Peter Krempa wrote:
> On Thu, Apr 11, 2019 at 11:58:04 +0200, Michal Privoznik wrote:
>> On 4/11/19 10:39 AM, Peter Krempa wrote:
>>> On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:
>>>> This capability tells whether qemu is capable of waking up the
>>>> guest from PM suspend.
>>>>
>>>> Based-on-work-of: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>    src/qemu/qemu_capabilities.c                  | 24 +++++++++++++++++++
>>>>    src/qemu/qemu_capabilities.h                  |  3 +++
>>>>    .../caps_4.0.0.riscv32.replies                | 11 +++++++++
>>>>    .../caps_4.0.0.riscv32.xml                    |  1 +
>>>>    .../caps_4.0.0.riscv64.replies                | 11 +++++++++
>>>>    .../caps_4.0.0.riscv64.xml                    |  1 +
>>>>    .../caps_4.0.0.x86_64.replies                 | 11 +++++++++
>>>>    .../caps_4.0.0.x86_64.xml                     |  1 +
>>>>    8 files changed, 63 insertions(+)
>>>>
>>>
>>> [...]
>>>
>>>
>>>>    bool
>>>>    virQEMUCapsCPUFilterFeatures(const char *name,
>>>>                                 void *opaque)
>>>> @@ -4373,6 +4395,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>>>>            return -1;
>>>>        if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
>>>>            return -1;
>>>> +    if (virQEMUCapsProbeQMPCurrentMachine(qemuCaps, mon) < 0)
>>>> +        return -1;
>>>
>>> This seems wrong by definition. The function is supposed to query
>>> current machine, but the capability lookup code uses 'none' machine
>>> type. IIUC the support for wakeup in some cases depends on the presence
>>> of ACPI in the guest and thus really can't be cached this way.
>>
>> Yep, I was just about to write this but then got sidetracked.
>>
>> So I see two options here:
>> 1) Query if guest is capable of PM suspend at runtime. However, this has to
>> be done in a clever way. It would need to be done not only in
>> qemuProcessLaunch() (or some subordinate functions), it would need to be
>> done in qemuProcessReconnect() too (to cover case when user is upgrading
> 
> If you are upgrading the capabilities parsed from the status XML won't
> have the bit representing the presence of the query command. This means
> that new libvirt would still attempt the suspend ignoring whether it is
> supported or not. I don't think it makes sense to re-detect presence of
> stuff which was done prior to upgrading.

Well, then mere upgrade of libvirt won't fix this bug for you. You'd 
have to restart the domain to make this bug go away.

> 
>> libvirt). For this, qemuMonitorConnect() looks like the best place - it's
>> called from both locations and it also already contains some 'runtime
>> capabilities' querying. Except not really - the migration capabilities
> 
> I was considering whether it's a good idea to keep runtime only
> capabilities in the qemuCaps array. I can find both arguments for it and
> against it. The convenient part is that it's automatically stored in the
> status XML.
> 
>> queried there are stored in @priv->migrationCaps rather than priv->qemuCaps.
>> Problem with storing runtime capabilities in priv->qemuCaps is that they
>> would get formatted into status XML. And this opens whole different can of
> 
> It is strongly desired to store it in the status XML. That way you don't
> have to re-query.

Well, what if you upgrade from ver1 to ver2 which introduces a new 
runtime capability? How could it be used if we would not requery caps on 
reconnect? Note, I don't mean full capabilities querying which is 
expensive. I'm talking about runtime capabilities. For instance this 
case. Suppose we have a capability (as proposed here) called 
PM_WAKEUP_SUPPORT.
If we would requery caps at reconnect then we wouldn't fix this bug.
On the other hand, for this specific bug - if you'd pmsuspend a domain 
that is incapable of wakeup, the only thing left for you to do is 
destroy + start combo which will fetch you the capability. But that's 
not the point.

> 
>> worms. For instance, this feature relies on 'query-commands' to see if
>> 'query-current-machine' command is available. Well, we can't re-run
>> 'query-commands' in runtime because that might change the set of qemuCaps
>> used when constructing cmd line.
> 
> Why bother? If qemu was started prior to the support being added I don't
> see a compelling reason to requery everything. In most cases it would be
> also wrong to do so.
> 
>> Way around this is to have 'runtime only' capabilities, that would not be
>> stored in status XML. At some point we will need those, but writing that
>> infrastructure is not easy and a bit too much to ask for this small fix.
>> Especially, when we have another option:
>>
>> 2) Issue 'query-current-machine' from qemuDomainPMSuspendForDuration(). In
>> every run. This is a bit suboptimal, but how frequently do we expect this
> 
> I don't think it's suboptimal. Normally users don't use PM suspend for
> the VM so you can argue that this saves one qemu call in all the cases
> where PM suspend is never used.
> 
>> API to be called? Caching qemu capabilities is there to speed up decission
>> process which makes sense if we'd be making the decission 1000 times a
>> second (e.g. building cmd line). With this approach the slowdown would be
>> negligible. Of course, we would need to run the command in 'silent' mode,
>> i.e. not report any error if qemu doesn't know it.
> 
> Or you can still use the capability bit whether the query command is
> available.

Nope, you can't. Let's just issue the command in 
qemuDomainPMSuspendForDuration(), unconditionally, ignore/silent any 
errors and leave runtime caps for another day.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Apr 11, 2019 at 13:49:03 +0200, Michal Privoznik wrote:
> On 4/11/19 12:19 PM, Peter Krempa wrote:
> > On Thu, Apr 11, 2019 at 11:58:04 +0200, Michal Privoznik wrote:
> > > On 4/11/19 10:39 AM, Peter Krempa wrote:
> > > > On Thu, Apr 11, 2019 at 10:25:11 +0200, Michal Privoznik wrote:

[...]

> > If you are upgrading the capabilities parsed from the status XML won't
> > have the bit representing the presence of the query command. This means
> > that new libvirt would still attempt the suspend ignoring whether it is
> > supported or not. I don't think it makes sense to re-detect presence of
> > stuff which was done prior to upgrading.
> 
> Well, then mere upgrade of libvirt won't fix this bug for you. You'd have to
> restart the domain to make this bug go away.

That is true. On the other hand a "mere" upgrade of libvirt will help in
this case only if you start a VM with qemu-4.0.0 or newer (unreleased).
With libvirt 5.2 or older and then upgrade to a new libvirtd.

Older qemus can't be fixed as they don't support the command and
starting with newer libvirt would not have the problem even if we cache
the presence of query-current machine.

Said that I find it perfectly acceptable to call query-current-machine
unconditionally in qemuDomainPMSuspendForDuration, I just don't see a
problem in caching presence of query-current-machine.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list