[libvirt] [PATCH 1/4] libxl: fix timer configuration

[Joao Martins]

On 01/20/2017 01:59 AM, Jim Fehlig wrote:
> The current logic around configuring timers in libxl based on
> virDomainDef object is a bit brain dead. Unsupported timers are
> silently ignored and tsc is only recognized if it is the first
> timer specified.
> 
> Change the logic to reject unsupported timers and honor the tsc
> timer regardless of its order when multiple timers are specified.
> 
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
>  src/libxl/libxl_conf.c | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index a24f9e0..3e6d623 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -313,9 +313,10 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>      for (i = 0; i < virDomainDefGetVcpus(def); i++)
>          libxl_bitmap_set((&b_info->avail_vcpus), i);
>  
> -    if (def->clock.ntimers > 0 &&
> -        def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
> -        switch (def->clock.timers[0]->mode) {
> +    for (i = 0; i < def->clock.ntimers; i++) {
> +        switch ((virDomainTimerNameType) def->clock.timers[i]->name) {
> +        case VIR_DOMAIN_TIMER_NAME_TSC:
> +            switch (def->clock.timers[i]->mode) {
>              case VIR_DOMAIN_TIMER_MODE_NATIVE:
>                  b_info->tsc_mode = 2;
>                  break;
> @@ -324,8 +325,35 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>                  break;
>              default:
>                  b_info->tsc_mode = 1;
> +            }
> +            break;
> +
> +        case VIR_DOMAIN_TIMER_NAME_HPET:
> +            if (!hvm) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("unsupported timer type (name) '%s'"),
> +                               virDomainTimerNameTypeToString(def->clock.timers[i]->name));
> +                return -1;
> +            }
> +            if (def->clock.timers[i]->present == 1)
> +                libxl_defbool_set(&b_info->u.hvm.hpet, 1);
> +            break;
> +
> +        case VIR_DOMAIN_TIMER_NAME_PLATFORM:
> +        case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
> +        case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
Interestingly, and looking at the code [toolstack:
tools/libxl/libxl_dom.c:hvm_set_viridian_features(), hypervisor:
xen/arch/x86/hvm/viridian.c:update_reference_tsc()] it looks like that to some
extent we support the Hyper-V Reference TSC page (Hyper-V Clock?) but it is kept
disabled in the viridian base features in libxl. Even if we wanted to enable it
would have to be more specific than adding <viridian /> feature like with:

libxl_bitmap_set(&b_info->u.hvm.viridian_enable,
LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_BASE);

libxl_bitmap_set(&b_info->u.hvm.viridian_enable,
LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_TSC);

and perhaps behind "#ifdef LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_TSC" as this
is only supported since Xen 4.6.0? And I think it would work with tsc_mode not
being emulated? Relevant commit on Xen is dd94caca5 ("x86/viridian: add
Partition Reference Time enlightenment") Anyhow this is probably not in the
context of this series, just food for thought in case we want to have in the future.

> +        case VIR_DOMAIN_TIMER_NAME_RTC:
> +        case VIR_DOMAIN_TIMER_NAME_PIT:
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unsupported timer type (name) '%s'"),
> +                           virDomainTimerNameTypeToString(def->clock.timers[i]->name));
> +            return -1;
> +
> +        case VIR_DOMAIN_TIMER_NAME_LAST:
> +            break;
>          }
>      }
> +
Spurious whitespace? Or perhaps it was purposely added?

>      b_info->sched_params.weight = 1000;
>      b_info->max_memkb = virDomainDefGetMemoryInitial(def);
>      b_info->target_memkb = def->mem.cur_balloon;
> @@ -341,12 +369,6 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>          libxl_defbool_set(&b_info->u.hvm.acpi,
>                            def->features[VIR_DOMAIN_FEATURE_ACPI] ==
>                            VIR_TRISTATE_SWITCH_ON);
> -        for (i = 0; i < def->clock.ntimers; i++) {
> -            if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET &&
> -                def->clock.timers[i]->present == 1) {
> -                libxl_defbool_set(&b_info->u.hvm.hpet, 1);
> -            }
> -        }
>  
>          if (def->nsounds > 0) {
>              /*
>