5:14 p.m.
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(a)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) {
>> /*
>>
>