[libvirt] [PATCH] qemu: Only issue the rtc-reset-reinjection QMP command on x86.
The command is only defined in QEMU for TARGET_I386, so issuing it on any other architecture can't possibly work. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1211938 --- src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..743ca6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18945,7 +18945,10 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; } - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + /* The rtc-reset-reinjection QMP command is only available on x86 */ + if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) + { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot set time: qemu doesn't support " "rtc-reset-reinjection command")); @@ -18968,13 +18971,16 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; } - qemuDomainObjEnterMonitor(driver, vm); - rv = qemuMonitorRTCResetReinjection(priv->mon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; + /* The rtc-reset-reinjection QMP command is only available on x86 */ + if (ARCH_IS_X86(vm->def->os.arch)) { + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorRTCResetReinjection(priv->mon); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; - if (rv < 0) - goto endjob; + if (rv < 0) + goto endjob; + } ret = 0; -- 2.1.0
On Mon, May 25, 2015 at 16:59:08 +0200, Andrea Bolognani wrote:
The command is only defined in QEMU for TARGET_I386, so issuing it on any other architecture can't possibly work.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1211938 --- src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..743ca6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18945,7 +18945,10 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + /* The rtc-reset-reinjection QMP command is only available on x86 */
Since we properly track support for this command via the capabilities and qemu does not expose it:
+ if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) + { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot set time: qemu doesn't support " "rtc-reset-reinjection command"));
I'd simply remove this error message since it is semantically wrong once PPC does not require to reset the RTC reinjection.
@@ -18968,13 +18971,16 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- qemuDomainObjEnterMonitor(driver, vm); - rv = qemuMonitorRTCResetReinjection(priv->mon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; + /* The rtc-reset-reinjection QMP command is only available on x86 */ + if (ARCH_IS_X86(vm->def->os.arch)) { + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorRTCResetReinjection(priv->mon);
And just conditionally call this when the QEMU_CAPS_RTC_RESET_REINJECTION is present and not in an architecture specific way. By this you get rid of the arch specific hackery.
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;
- if (rv < 0) - goto endjob; + if (rv < 0) + goto endjob; + }
ret = 0;
Peter
On Mon, May 25, 2015 at 05:26:01PM +0200, Peter Krempa wrote:
On Mon, May 25, 2015 at 16:59:08 +0200, Andrea Bolognani wrote:
The command is only defined in QEMU for TARGET_I386, so issuing it on any other architecture can't possibly work.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1211938 --- src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..743ca6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18945,7 +18945,10 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + /* The rtc-reset-reinjection QMP command is only available on x86 */
Since we properly track support for this command via the capabilities and qemu does not expose it:
+ if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) + { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot set time: qemu doesn't support " "rtc-reset-reinjection command"));
I'd simply remove this error message since it is semantically wrong once PPC does not require to reset the RTC reinjection.
@@ -18968,13 +18971,16 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- qemuDomainObjEnterMonitor(driver, vm); - rv = qemuMonitorRTCResetReinjection(priv->mon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; + /* The rtc-reset-reinjection QMP command is only available on x86 */ + if (ARCH_IS_X86(vm->def->os.arch)) { + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorRTCResetReinjection(priv->mon);
And just conditionally call this when the QEMU_CAPS_RTC_RESET_REINJECTION is present and not in an architecture specific way.
By this you get rid of the arch specific hackery.
But on x86 we don't even want to call the SetTime command when we cannot reset the rtc reinjection. On ppc there is no reinjection being done, hence nothing to reset.
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;
- if (rv < 0) - goto endjob; + if (rv < 0) + goto endjob; + }
ret = 0;
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2015-05-25 at 17:43 +0200, Martin Kletzander wrote:
But on x86 we don't even want to call the SetTime command when we cannot reset the rtc reinjection. On ppc there is no reinjection being done, hence nothing to reset.
Exactly. I think we want to keep the checks as they are for x86, because not doing so would mean versions of QEMU older than 2.0.1 get the clock corrected two times. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team $ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"
On Mon, May 25, 2015 at 17:43:05 +0200, Martin Kletzander wrote:
On Mon, May 25, 2015 at 05:26:01PM +0200, Peter Krempa wrote:
On Mon, May 25, 2015 at 16:59:08 +0200, Andrea Bolognani wrote:
The command is only defined in QEMU for TARGET_I386, so issuing it on any other architecture can't possibly work.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1211938 --- src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..743ca6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18945,7 +18945,10 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + /* The rtc-reset-reinjection QMP command is only available on x86 */
Since we properly track support for this command via the capabilities and qemu does not expose it:
+ if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) + { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot set time: qemu doesn't support " "rtc-reset-reinjection command"));
I'd simply remove this error message since it is semantically wrong once PPC does not require to reset the RTC reinjection.
@@ -18968,13 +18971,16 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- qemuDomainObjEnterMonitor(driver, vm); - rv = qemuMonitorRTCResetReinjection(priv->mon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; + /* The rtc-reset-reinjection QMP command is only available on x86 */ + if (ARCH_IS_X86(vm->def->os.arch)) { + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorRTCResetReinjection(priv->mon);
And just conditionally call this when the QEMU_CAPS_RTC_RESET_REINJECTION is present and not in an architecture specific way.
By this you get rid of the arch specific hackery.
But on x86 we don't even want to call the SetTime command when we cannot reset the rtc reinjection. On ppc there is no reinjection being done, hence nothing to reset.
Well I don't think that the missing reinjection request should be a hard requirement on x86 since the guest can modify time independently and in that case the reinjection command won't be even issued. The guest agent command is technically a guest issued time change too, so there's only the added bonus that we'd reset the interrupt backlog. One slight issue is that libvirt does not allow to reset the interrupt backlog while not setting the time though. Even if we decide that the check for the existance of the command should be arch-specific and should report error, the actual place where the command is called should be made dependant on the presence of the command rather than the ARCH check, so that it does not need to be modified once it's implemented somewhere else. Peter
On Tue, May 26, 2015 at 10:08:58AM +0200, Peter Krempa wrote:
On Mon, May 25, 2015 at 17:43:05 +0200, Martin Kletzander wrote:
On Mon, May 25, 2015 at 05:26:01PM +0200, Peter Krempa wrote:
On Mon, May 25, 2015 at 16:59:08 +0200, Andrea Bolognani wrote:
The command is only defined in QEMU for TARGET_I386, so issuing it on any other architecture can't possibly work.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1211938 --- src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..743ca6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18945,7 +18945,10 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + /* The rtc-reset-reinjection QMP command is only available on x86 */
Since we properly track support for this command via the capabilities and qemu does not expose it:
+ if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) + { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot set time: qemu doesn't support " "rtc-reset-reinjection command"));
I'd simply remove this error message since it is semantically wrong once PPC does not require to reset the RTC reinjection.
@@ -18968,13 +18971,16 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- qemuDomainObjEnterMonitor(driver, vm); - rv = qemuMonitorRTCResetReinjection(priv->mon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; + /* The rtc-reset-reinjection QMP command is only available on x86 */ + if (ARCH_IS_X86(vm->def->os.arch)) { + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorRTCResetReinjection(priv->mon);
And just conditionally call this when the QEMU_CAPS_RTC_RESET_REINJECTION is present and not in an architecture specific way.
By this you get rid of the arch specific hackery.
But on x86 we don't even want to call the SetTime command when we cannot reset the rtc reinjection. On ppc there is no reinjection being done, hence nothing to reset.
Well I don't think that the missing reinjection request should be a hard requirement on x86 since the guest can modify time independently and in that case the reinjection command won't be even issued. The guest agent command is technically a guest issued time change too, so there's only the added bonus that we'd reset the interrupt backlog. One slight issue is that libvirt does not allow to reset the interrupt backlog while not setting the time though.
The implementer of the check might remember more reasons for arguing on this, let him shed the light into this discussion. Michal? Anyway, this is broken all the way because the command also makes sense only with some particular timers and their settins, not with all of them.
Even if we decide that the check for the existance of the command should be arch-specific and should report error, the actual place where the command is called should be made dependant on the presence of the command rather than the ARCH check, so that it does not need to be modified once it's implemented somewhere else.
I don't quite catch your drift here. If you mean that the second condition added should be based on virQEMUCapsGet(), then I agree.
On Tue, May 26, 2015 at 10:53:53 +0200, Martin Kletzander wrote:
On Tue, May 26, 2015 at 10:08:58AM +0200, Peter Krempa wrote:
On Mon, May 25, 2015 at 17:43:05 +0200, Martin Kletzander wrote:
On Mon, May 25, 2015 at 05:26:01PM +0200, Peter Krempa wrote:
On Mon, May 25, 2015 at 16:59:08 +0200, Andrea Bolognani wrote:
...
And just conditionally call this when the QEMU_CAPS_RTC_RESET_REINJECTION is present and not in an architecture specific way.
By this you get rid of the arch specific hackery.
But on x86 we don't even want to call the SetTime command when we cannot reset the rtc reinjection. On ppc there is no reinjection being done, hence nothing to reset.
Well I don't think that the missing reinjection request should be a hard requirement on x86 since the guest can modify time independently and in that case the reinjection command won't be even issued. The guest agent command is technically a guest issued time change too, so there's only the added bonus that we'd reset the interrupt backlog. One slight issue is that libvirt does not allow to reset the interrupt backlog while not setting the time though.
The implementer of the check might remember more reasons for arguing on this, let him shed the light into this discussion. Michal?
Anyway, this is broken all the way because the command also makes sense only with some particular timers and their settins, not with all of them.
In that case it would make more sense to have the interrupt reinjection reset as a separate option or it to be enabled with a flag.
Even if we decide that the check for the existance of the command should be arch-specific and should report error, the actual place where the command is called should be made dependant on the presence of the command rather than the ARCH check, so that it does not need to be modified once it's implemented somewhere else.
I don't quite catch your drift here. If you mean that the second condition added should be based on virQEMUCapsGet(), then I agree.
Yes that was my intent. The actual call should be based on the capability. Sanity checks that are arch specific should be present before. Peter
On Tue, 2015-05-26 at 13:28 +0200, Peter Krempa wrote:
The implementer of the check might remember more reasons for arguing on this, let him shed the light into this discussion. Michal?
Anyway, this is broken all the way because the command also makes sense only with some particular timers and their settins, not with all of them.
In that case it would make more sense to have the interrupt reinjection reset as a separate option or it to be enabled with a flag.
I've talked to Michal and he agrees with the proposed approach.
I don't quite catch your drift here. If you mean that the second condition added should be based on virQEMUCapsGet(), then I agree.
Yes that was my intent. The actual call should be based on the capability. Sanity checks that are arch specific should be present before.
Sure, it works much better that way, thanks for the suggestion :) I've updated the patch accordingly. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team $ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"
The QMP command, like the interrupt reinjection logic it's connected to, is only implemented in QEMU when TARGET_I386 is defined, so checking for its availability on any other architecture is pointless. On the other hand, when we're on x86, we shouldn still make sure that rtc-reset-reinjection is available and refuse to set the time otherwise. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1211938 --- src/qemu/qemu_driver.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..db72bad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18945,7 +18945,12 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; } - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + /* On x86, the rtc-reset-reinjection QMP command must be called after + * setting the time to avoid trouble down the line. If the command is + * not available, don't set the time at all and report an error */ + if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) + { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot set time: qemu doesn't support " "rtc-reset-reinjection command")); @@ -18968,13 +18973,16 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; } - qemuDomainObjEnterMonitor(driver, vm); - rv = qemuMonitorRTCResetReinjection(priv->mon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; + /* Don't try to call rtc-reset-reinjection if it's not available */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorRTCResetReinjection(priv->mon); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; - if (rv < 0) - goto endjob; + if (rv < 0) + goto endjob; + } ret = 0; -- 2.1.0
On Tue, May 26, 2015 at 06:13:40PM +0200, Andrea Bolognani wrote:
The QMP command, like the interrupt reinjection logic it's connected to, is only implemented in QEMU when TARGET_I386 is defined, so checking for its availability on any other architecture is pointless.
On the other hand, when we're on x86, we shouldn still make sure that rtc-reset-reinjection is available and refuse to set the time otherwise.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1211938 --- src/qemu/qemu_driver.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
ACK, will push in a while.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa0acde..db72bad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18945,7 +18945,12 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + /* On x86, the rtc-reset-reinjection QMP command must be called after + * setting the time to avoid trouble down the line. If the command is + * not available, don't set the time at all and report an error */ + if (ARCH_IS_X86(vm->def->os.arch) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) + { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot set time: qemu doesn't support " "rtc-reset-reinjection command")); @@ -18968,13 +18973,16 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; }
- qemuDomainObjEnterMonitor(driver, vm); - rv = qemuMonitorRTCResetReinjection(priv->mon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; + /* Don't try to call rtc-reset-reinjection if it's not available */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) { + qemuDomainObjEnterMonitor(driver, vm); + rv = qemuMonitorRTCResetReinjection(priv->mon); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob;
- if (rv < 0) - goto endjob; + if (rv < 0) + goto endjob; + }
ret = 0;
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2015-05-27 at 16:55 +0200, Martin Kletzander wrote:
ACK, will push in a while.
Thanks :) -- Andrea Bolognani Software Engineer - Virtualization Team $ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"
participants (3)
-
Andrea Bolognani -
Martin Kletzander -
Peter Krempa