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.

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

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

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.

A general point, these types of checks should be considered for
qemuDomainDefValidate which adds the benefit of rejecting the config at XML
define time.

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

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

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

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

On 07/13/2017 09:52 AM, Madhu Pavan wrote:
> 
> 
> 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.
> 

Hmm, well that qemu64 bit seems like something else to fix, but not a blocker
for this.

> 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.
> 

If qemu just prints a warning in that case, then I agree we should explicitly
reject it in libvirt. Though that does mean that guests that were possibly
working before, but with a qemu warning, will now fail to redefine. Not sure
we care enough for this case though

>>
>> 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.
> 

That was the case at one point, but nowadays this is skipped when reading XML
from disk, see the VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag.

Thanks,
Cole

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