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@xxxxxxxx>
>>> ---
>>> 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) {
>>> /*
>>>
>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
