[libvirt] [PATCH v3 1/2] qemu: Restrict usage of hpet and kvm.pit timers by unsupported architectures

[Cole Robinson]

On 07/25/2017 10:36 AM, Kothapally Madhu Pavan wrote:
> hpet and kvm.pit clock timers are specific to x86 architecture and
> are not suppose to be used in other architectures. This patch restricts
> the usage of hpet and kvm.pit timers in unsupported architectures.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_domain.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2c8c9a7..e5e4208 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3083,12 +3083,28 @@ qemuDomainDefValidate(const virDomainDef *def,
>      virQEMUCapsPtr qemuCaps = NULL;
>      unsigned int topologycpus;
>      int ret = -1;
> +    size_t i;
>  
>      if (!(qemuCaps = virQEMUCapsCacheLookup(caps,
>                                              driver->qemuCapsCache,
>                                              def->emulator)))
>          goto cleanup;
>  
> +    /* Restrict usage of unsupported clock sources */
> +    for (i = 0; i < def->clock.ntimers; i++) {
> +        virDomainTimerDefPtr timer = def->clock.timers[i];
> +        if ((!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_HPET)) &&
> +             (timer->name == VIR_DOMAIN_TIMER_NAME_HPET)) ||
> +            (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_KVM_PIT)) &&
> +             (timer->name == VIR_DOMAIN_TIMER_NAME_PIT))) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unsupported clock timer '%s' for %s architecture"),
> +                           virDomainTimerNameTypeToString(def->clock.timers[i]->name),
> +                           virArchToString(def->os.arch));
> +            goto cleanup;
> +        }
> +    }
> +
>      if (def->mem.min_guarantee) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("Parameter 'min_guarantee' not supported by QEMU."));
> 

The HPET bit isn't strictly correct. See the HPET handling in qemu_command.c,
the only case that's explicitly rejected is when hpet present=yes is
specified, but qemu doesn't have NO_HPET. For example requesting hpet
present=no on PPC64 is arguably a valid request, since there won't ever be
hpet available.

So I think in this case it's better to just drop the HPET checking, OR move
the qemu_command.c error message here. But the benefit of this is very small
(validating hpet present=yes at XML define time vs startup time)

However patch #2 will also reject that HPET case. I think patch #2 should be
dropped entirely. Figure out what timer XML settings produce ambiguous qemu
results (the kvm-pit bit you mentioned previously that qemu won't error about)
and only reject those, and do it with QEMU_CAPS settings.

If qemu/libvirt already throws an error, like it seems to do for hypervclock,
tsc, kvmclock, hpet, platform, then adding an extra validation only adds the
benefit of an improved error message and catching the issue at XML parse time,
but the downside is it runs the risk of being overly restrictive, and it adds
more code to maintain. So IMO for those cases that are going to error anyway
it's typically better to just leave it alone unless the qemu error message is
really ambiguous

For the kvmclock/hypervclock/tsc issue, I posted patches to try and improve
our handling a bit and give a more explicit error, still need to address
reviews/push them though

https://www.redhat.com/archives/libvir-list/2017-July/msg00537.html

- Cole