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

[Madhu Pavan]

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