On 01/20/2017 10:17 PM, Jim Fehlig wrote: > On 01/20/2017 05:00 AM, Joao Martins wrote: >> On 01/20/2017 01:59 AM, Jim Fehlig wrote: >>> Currently xenconfig only supports the hpet timer for HVM domains. >>> Include support for tsc timer for both PV and HVM domains. >>> >>> Signed-off-by: Jim Fehlig <jfeh...@suse.com> >>> --- >>> src/xenconfig/xen_common.c | 87 >>> ++++++++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 77 insertions(+), 10 deletions(-) >>> >>> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c >>> index 7968d40..ad596d7 100644 >>> --- a/src/xenconfig/xen_common.c >>> +++ b/src/xenconfig/xen_common.c >>> @@ -490,6 +490,7 @@ xenParseCPUFeatures(virConfPtr conf, >>> unsigned long count = 0; >>> const char *str = NULL; >>> int val = 0; >>> + virDomainTimerDefPtr timer; >>> >>> if (xenConfigGetULong(conf, "vcpus", &count, 1) < 0) >>> return -1; >>> @@ -514,6 +515,29 @@ xenParseCPUFeatures(virConfPtr conf, >>> if (str && (virBitmapParse(str, &def->cpumask, 4096) < 0)) >>> return -1; >>> >>> + if (xenConfigGetString(conf, "tsc_mode", &str, NULL) < 0) >>> + return -1; >>> + >>> + if (str) { >>> + if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 || >>> + VIR_ALLOC(timer) < 0) >>> + return -1; >>> + >>> + timer->name = VIR_DOMAIN_TIMER_NAME_TSC; >>> + timer->present = 1; >>> + timer->tickpolicy = -1; >>> + timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO; >>> + timer->track = -1; >>> + if (STREQ_NULLABLE(str, "always_emulate")) >>> + timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE; >>> + else if (STREQ_NULLABLE(str, "native")) >>> + timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE; >>> + else if (STREQ_NULLABLE(str, "native_paravirt")) >>> + timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT; >>> + >>> + def->clock.timers[def->clock.ntimers - 1] = timer; >>> + } >>> + >>> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { >>> if (xenConfigGetBool(conf, "pae", &val, 1) < 0) >>> return -1; >>> @@ -545,9 +569,7 @@ xenParseCPUFeatures(virConfPtr conf, >>> return -1; >>> >>> if (val != -1) { >>> - virDomainTimerDefPtr timer; >>> - >>> - if (VIR_ALLOC_N(def->clock.timers, 1) < 0 || >>> + if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 >>> || >>> VIR_ALLOC(timer) < 0) >>> return -1; >>> >>> @@ -557,8 +579,7 @@ xenParseCPUFeatures(virConfPtr conf, >>> timer->mode = -1; >>> timer->track = -1; >>> >>> - def->clock.ntimers = 1; >>> - def->clock.timers[0] = timer; >>> + def->clock.timers[def->clock.ntimers - 1] = timer; >>> } >>> } >>> >>> @@ -1584,8 +1605,9 @@ static int >>> xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr def) >>> { >>> size_t i; >>> + int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM ? 1 : 0; >> You can probably avoid the ternary expression style with turning this into a >> bool and do !!(def->os.type == VIR_DOMAIN_OSTYPE_HVM) or simply just >> (def->os.type == VIR_DOMAIN_OSTYPE_HVM). > > I think this is the preferred style. > >> Or is it that libvirt style prefers >> this way? Looking at the file it looks like the style you used above is kept >> throughout this file. > > I guess a bad habit was started in this file and grew though copy and past > :-). Hehe :)
> >> >>> >>> - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { >>> + if (hvm) { >>> if (xenConfigSetInt(conf, "pae", >>> (def->features[VIR_DOMAIN_FEATURE_PAE] == >>> VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) >>> @@ -1610,12 +1632,57 @@ xenFormatCPUFeatures(virConfPtr conf, >>> virDomainDefPtr def) >>> (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == >>> VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) >>> return -1; >>> + } >>> >>> - 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 && >>> - xenConfigSetInt(conf, "hpet", >>> def->clock.timers[i]->present) < 0) >>> + 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: >>> + if (xenConfigSetString(conf, "tsc_mode", "native") < 0) >>> + return -1; >>> + break; >>> + case VIR_DOMAIN_TIMER_MODE_PARAVIRT: >>> + if (xenConfigSetString(conf, "tsc_mode", >>> "native_paravirt") < 0) >>> + return -1; >>> + break; >>> + case VIR_DOMAIN_TIMER_MODE_EMULATE: >>> + if (xenConfigSetString(conf, "tsc_mode", "always_emulate") >>> < 0) >>> + return -1; >>> + break; >>> + default: >>> + if (xenConfigSetString(conf, "tsc_mode", "default") < 0) >>> + return -1; >>> + } >>> + break; >>> + >>> + case VIR_DOMAIN_TIMER_NAME_HPET: >>> + if (hvm) { >>> + int enable_hpet = def->clock.timers[i]->present == 0 ? 0 : >>> 1; >> Same here i.e. (def->clock.timers[i]->present != 0) ? > > Yep. I'll leave this as an int since it is used in xenConfigSetInt. OK. >> >>> + >>> + /* disable hpet if 'present' is 0, enable otherwise */ >>> + if (xenConfigSetInt(conf, "hpet", enable_hpet) < 0) >>> + return -1; >>> + } else { >>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >>> + _("unsupported timer type (name) '%s'"), >>> + >>> virDomainTimerNameTypeToString(def->clock.timers[i]->name)); >>> return -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; >>> } >>> } > > I've squashed the below diff into my branch. Do you have any additional > comments > on this series? I haven't spotted any particular issues (e.g. memleaks, test failures) so I have no further comments: Acked-by: Joao Martins <joao.m.mart...@oracle.com> > > Regards, > Jim > > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > index ad596d7..72ffb30 100644 > --- a/src/xenconfig/xen_common.c > +++ b/src/xenconfig/xen_common.c > @@ -1605,7 +1605,7 @@ static int > xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr def) > { > size_t i; > - int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM ? 1 : 0; > + bool hvm = !!(def->os.type == VIR_DOMAIN_OSTYPE_HVM); > > if (hvm) { > if (xenConfigSetInt(conf, "pae", > @@ -1658,7 +1658,7 @@ xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr > def) > > case VIR_DOMAIN_TIMER_NAME_HPET: > if (hvm) { > - int enable_hpet = def->clock.timers[i]->present == 0 ? 0 : 1; > + int enable_hpet = def->clock.timers[i]->present != 0; > > /* disable hpet if 'present' is 0, enable otherwise */ > if (xenConfigSetInt(conf, "hpet", enable_hpet) < 0) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list