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",