Re: [PATCH 3/4] xenconfig: add support for more timers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxx>

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux