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

Jim Fehlig jfehlig at suse.com
Fri Jan 20 21:55:36 UTC 2017


On 01/20/2017 05:00 AM, Joao Martins wrote:
> 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);

I'd think <viridian/> OS feature would enable 
LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_BASE

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

And this one would be enabled with

   <clock>
     <timer name='hypervclock'/>
   </clock>

Is that correct? Should a hypervclock timer also include 
LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT?

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

Good points, and agree they could be addressed in a followup series that enables 
viridian enlightenments.

>
>> +        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?

Initially I think it was spurious, but looking again I think it brings some 
needed whitespace between these sections of code.

Regards,
Jim

>
>>      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) {
>>              /*
>>
>




More information about the libvir-list mailing list