5:23 p.m.
On 01/20/2017 04:16 PM, Joao Martins wrote:
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 <jfehlig(a)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.martins(a)oracle.com>
Thanks! I've pushed the series now.
Regards,
Jim