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

[Joao Martins]

On 01/20/2017 09:55 PM, Jim Fehlig wrote:
> 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
Yeap.

> 
>> 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?
IIUC the TIME_REF_COUNT enlightenment is for older versions of windwos, whereas
REFERENCE_TSC is the newer one and most performant. Presumably TIME_REF is
included in the base viridian features.

> 
>> 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.
Yeap that's what it sounded like, that is to separate these two sections.

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