Re: [libvirt] [PATCH 1/4] libxl: fix timer configuration
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
>>> ---
>>> 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 feature like with:
>>
>> libxl_bitmap_set(&b_info->u.hvm.viridian_enable,
>> LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_BASE);
>
> I'd think 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
>
>
>
>
>
> 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_ballo
Re: [libvirt] [PATCH 1/4] libxl: fix timer configuration
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
---
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 feature like with:
libxl_bitmap_set(&b_info->u.hvm.viridian_enable,
LIBXL_VIRIDIAN_ENLIGHTENMENT_REFERENCE_BASE);
I'd think 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
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) {
/*
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] libxl: fix timer configuration
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
> ---
> 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 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) {
> /*
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] libxl: fix timer configuration
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
---
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:
+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;
}
}
+
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) {
/*
--
2.9.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
