[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Default hwclock source for sPAPR to RTC





On 07/13/2017 05:49 PM, Cole Robinson wrote:
On 07/13/2017 04:36 AM, Kothapally Madhu Pavan wrote:
QEMU fails to launch a sPAPR guest with clock sources other that RTC.
Internally qemu only uses RTC timer for hwclock. This patch reports
the right error message instead of qemu erroring out when any other
timer other than RTC is used.

How does it fail exactly? Is it a qemu error message or a guest OS failure?

If it's from qemu, and the error message is reasonably clear what hardware/xml
config option is at fauly, then these checks don't add much functional
benefit, just more code to maintain.

If it's from qemu, and the error message is reasonably clear what hardware/xml
config option is at fauly, then these checks don't add much functional
benefit, just more code to maintain.

When we use kvmclock timer in domain xml as:
  <clock offset='utc'>
    <timer name='kvmclock' present='yes'/>
  </clock>
Domain fails to start with following error:
#virsh start --console virt-tests-vm1
error: Failed to start domain virt-tests-vm1
error: internal error: process exited while connecting to monitor: 2017-04-25T09:31:58.180062Z qemu-system-ppc64: Unable to find CPU definition: qemu64

This is because the qemu cpu command line generated when kvmclock
timer is used is:
 -cpu qemu64,+kvmclock
This happens because in qemuBuildCpuCommandLine has default_model = qemu64,
When I corrected the default model to "host" for ppc64 machine,
qemu cpu commandline generated is:
 -cpu host,+kvmclock
This is a valid qemu command for ppc64 machine. Now the qemu
fails to start with folloeing error:
qemu-system-ppc64: Expected key=value format, found +kvmclock.

Similarly when kvm-pit timer is used qemu warns as below:
sudo ./qemu-system-ppc64 -name migrate_qemu -boot strict=on -device nec-usb-xhci,id=usb,bus=pci.0,addr=0xf -device spapr-vscsi,id=scsi0,reg=0x2000 -smp 1,maxcpus=4,sockets=4,cores=1,threads=1 --machine pseries,accel=kvm,kvm-type=HV,usb=off,dump-guest-core=off -m 4G,slots=32,maxmem=32G -drive file=/home/danielhb/vm_imgs/ubuntu1704.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=none -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x2,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -nographic -global kvm-pit.lost_tick_policy=discard

qemu-system-ppc64: Warning: global kvm-pit.lost_tick_policy has invalid class name

Basically, RTC is the only valid clocksource for sPAPR guests. For other clock sources
qemu either errors out or internally considers RTC as default.


A general point, these types of checks should be considered for
qemuDomainDefValidate which adds the benefit of rejecting the config at XML
define time.
I was of the opinion, the existing the domain definitions would fail to be parsed if I add in qemuDomainDefValidate(). Now, while I reply to you I realise, there is no way someone would have attempted to use non-RTC clock sources. So, its perfectly safe to move these
checks to qemuDomainDefValidate().

Will attempt it in V2.

Thanks,
Cole

Signed-off-by: Kothapally Madhu Pavan <kmp linux vnet ibm com>
---
  src/qemu/qemu_command.c | 28 +++++++++++++++++++++++++++-
  1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97..31561ce 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6440,6 +6440,15 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
              break;
case VIR_DOMAIN_TIMER_NAME_PIT:
+            /* Only RTC timer is supported as hwclock for sPAPR machines */
+            if (ARCH_IS_PPC64(def->os.arch)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("unsupported clock timer '%s' for '%s' architecture"),
+                                 virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+                                 virArchToString(def->os.arch));
+                return -1;
+            }
+
              switch (def->clock.timers[i]->tickpolicy) {
              case -1:
              case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY:
@@ -6483,13 +6492,21 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
              break;
case VIR_DOMAIN_TIMER_NAME_HPET:
+            /* Only RTC timer is supported as hwclock for sPAPR machines */
+            if (ARCH_IS_PPC64(def->os.arch)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("unsupported clock timer '%s' for '%s' architecture"),
+                                 virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+                                 virArchToString(def->os.arch));
+                return -1;
+            }
+
              /* the only meaningful attribute for hpet is "present". If
               * present is -1, that means it wasn't specified, and
               * should be left at the default for the
               * hypervisor. "default" when -no-hpet exists is "yes",
               * and when -no-hpet doesn't exist is "no". "confusing"?
               * "yes"! */
-
              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) {
                  if (def->clock.timers[i]->present == 0)
                      virCommandAddArg(cmd, "-no-hpet");
@@ -7047,6 +7064,15 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
      for (i = 0; i < def->clock.ntimers; i++) {
          virDomainTimerDefPtr timer = def->clock.timers[i];
+ /* Only RTC timer is supported as hwclock for sPAPR machines */
+        if (ARCH_IS_PPC64(def->os.arch) && timer->name != VIR_DOMAIN_TIMER_NAME_RTC) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("unsupported clock timer '%s' for '%s' architecture"),
+                             virDomainTimerNameTypeToString(def->clock.timers[i]->name),
+                             virArchToString(def->os.arch));
+            return -1;
+        }
+
          if (timer->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK &&
              timer->present != -1) {
              virBufferAsprintf(&buf, "%s,%ckvmclock",



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]