| Message ID | 1464799050-11002-5-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 238f69d..618aef9 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > X86CPU *cpu = X86_CPU(cs); > char *featurestr; /* Single 'key=value" string being parsed */ > uint32_t numvalue; > + char num[32]; > + const char *name; > + char *err; > Error *local_err = NULL; > > featurestr = features ? strtok(features, ",") : NULL; > > while (featurestr) { > - char *val; > + char *val = NULL; > if (featurestr[0] == '+') { > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > } else if (featurestr[0] == '-') { > @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > } else if ((val = strchr(featurestr, '='))) { > *val = 0; val++; > feat2prop(featurestr); > + name = featurestr; > if (!strcmp(featurestr, "xlevel")) { > - char *err; > - char num[32]; > - > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > error_setg(errp, "bad numerical value %s", val); > @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > numvalue += 0x80000000; > } > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > + val = num; > } else if (!strcmp(featurestr, "tsc-freq")) { > int64_t tsc_freq; > - char *err; > - char num[32]; > > tsc_freq = qemu_strtosz_suffix_unit(val, &err, > QEMU_STRTOSZ_DEFSUFFIX_B, 1000); > @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > return; > } > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > - object_property_parse(OBJECT(cpu), num, "tsc-frequency", > - &local_err); > + val = num; > + name = "tsc-frequency"; > } else if (!strcmp(featurestr, "hv-spinlocks")) { > - char *err; > const int min = 0xFFF; > - char num[32]; > + > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > error_setg(errp, "bad numerical value %s", val); > @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > numvalue = min; > } > snprintf(num, sizeof(num), "%" PRId32, numvalue); > - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > - } else { > - object_property_parse(OBJECT(cpu), val, featurestr, &local_err); > + val = num; > } > } else { > feat2prop(featurestr); > - object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); > + name = featurestr; > + val = (char *)"on"; You don't need this hack if you do something like this: while (featurestr) { - char *val = NULL; + const char *val = NULL; + char *eq = NULL; if (featurestr[0] == '+') { add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); } else if (featurestr[0] == '-') { add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); - } else if ((val = strchr(featurestr, '='))) { - *val = 0; val++; + } else if ((eq = strchr(featurestr, '='))) { + *eq++ = 0; + val = eq; feat2prop(featurestr); name = featurestr; if (!strcmp(featurestr, "xlevel")) { > } > + > + if (val) { > + object_property_parse(OBJECT(cpu), val, name, &local_err); > + } I would find it easier to read if this part was inside the same block as the code that sets name/val. I would find it even better if object_property_parse() was in the main loop body, and the +feature/-feature code placed inside an if/continue branch (to make it clear that it is an exception, not the rule). Like this: /* untested */ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, Error **errp) { X86CPU *cpu = X86_CPU(cs); char *featurestr; /* Single 'key=value" string being parsed */ char *err; Error *local_err = NULL; if (!features) { return; } for (featurestr = strtok(features, ","); featurestr; featurestr = strtok(NULL, ",")) { const char *name; const char *val = NULL; char *eq = NULL; uint32_t numvalue; char num[32]; /* Compatibility syntax: */ if (featurestr[0] == '+') { add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); continue; } else if (featurestr[0] == '-') { add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); continue; } eq = strchr(featurestr, '='); if (eq) { *eq++ = 0; val = eq; } else { val = "on"; } feat2prop(featurestr); name = featurestr; /* Special cases: */ if (!strcmp(name, "xlevel")) { numvalue = strtoul(val, &err, 0); if (!*val || *err) { error_setg(errp, "bad numerical value %s", val); return; } if (numvalue < 0x80000000) { error_report("xlevel value shall always be >= 0x80000000" ", fixup will be removed in future versions"); numvalue += 0x80000000; } snprintf(num, sizeof(num), "%" PRIu32, numvalue); val = num; } else if (!strcmp(name, "tsc-freq")) { int64_t tsc_freq; tsc_freq = qemu_strtosz_suffix_unit(val, &err, QEMU_STRTOSZ_DEFSUFFIX_B, 1000); if (tsc_freq < 0 || *err) { error_setg(errp, "bad numerical value %s", val); return; } snprintf(num, sizeof(num), "%" PRId64, tsc_freq); val = num; name = "tsc-frequency"; } else if (!strcmp(name, "hv-spinlocks")) { const int min = 0xFFF; numvalue = strtoul(val, &err, 0); if (!*val || *err) { error_setg(errp, "bad numerical value %s", val); return; } if (numvalue < min) { error_report("hv-spinlocks value shall always be >= 0x%x" ", fixup will be removed in future versions", min); numvalue = min; } snprintf(num, sizeof(num), "%" PRId32, numvalue); val = num; } object_property_parse(OBJECT(cpu), val, name, &local_err); if (local_err) { error_propagate(errp, local_err); return; } } } > + > if (local_err) { > error_propagate(errp, local_err); > return; > -- > 1.8.3.1 >
On Wed, 1 Jun 2016 15:46:20 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > target-i386/cpu.c | 32 +++++++++++++++++--------------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 238f69d..618aef9 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > X86CPU *cpu = X86_CPU(cs); > > char *featurestr; /* Single 'key=value" string being parsed */ > > uint32_t numvalue; > > + char num[32]; > > + const char *name; > > + char *err; > > Error *local_err = NULL; > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > while (featurestr) { > > - char *val; > > + char *val = NULL; > > if (featurestr[0] == '+') { > > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > > } else if (featurestr[0] == '-') { > > @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > } else if ((val = strchr(featurestr, '='))) { > > *val = 0; val++; > > feat2prop(featurestr); > > + name = featurestr; > > if (!strcmp(featurestr, "xlevel")) { > > - char *err; > > - char num[32]; > > - > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > numvalue += 0x80000000; > > } > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > > + val = num; > > } else if (!strcmp(featurestr, "tsc-freq")) { > > int64_t tsc_freq; > > - char *err; > > - char num[32]; > > > > tsc_freq = qemu_strtosz_suffix_unit(val, &err, > > QEMU_STRTOSZ_DEFSUFFIX_B, 1000); > > @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > return; > > } > > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > > - object_property_parse(OBJECT(cpu), num, "tsc-frequency", > > - &local_err); > > + val = num; > > + name = "tsc-frequency"; > > } else if (!strcmp(featurestr, "hv-spinlocks")) { > > - char *err; > > const int min = 0xFFF; > > - char num[32]; > > + > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > numvalue = min; > > } > > snprintf(num, sizeof(num), "%" PRId32, numvalue); > > - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > > - } else { > > - object_property_parse(OBJECT(cpu), val, featurestr, &local_err); > > + val = num; > > } > > } else { > > feat2prop(featurestr); > > - object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); > > + name = featurestr; > > + val = (char *)"on"; > > You don't need this hack if you do something like this: > > > while (featurestr) { > - char *val = NULL; > + const char *val = NULL; > + char *eq = NULL; > if (featurestr[0] == '+') { > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > } else if (featurestr[0] == '-') { > add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); > - } else if ((val = strchr(featurestr, '='))) { > - *val = 0; val++; > + } else if ((eq = strchr(featurestr, '='))) { > + *eq++ = 0; > + val = eq; > feat2prop(featurestr); > name = featurestr; > if (!strcmp(featurestr, "xlevel")) { > > > > } > > + > > + if (val) { > > + object_property_parse(OBJECT(cpu), val, name, &local_err); > > + } > > I would find it easier to read if this part was inside the same > block as the code that sets name/val. > > I would find it even better if object_property_parse() was in the > main loop body, and the +feature/-feature code placed inside an > if/continue branch (to make it clear that it is an exception, not > the rule). Like this: Thanks, I'll take your variant with your permission. > /* untested */ > static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > Error **errp) > { > X86CPU *cpu = X86_CPU(cs); > char *featurestr; /* Single 'key=value" string being parsed */ > char *err; > Error *local_err = NULL; > > if (!features) { > return; > } > > for (featurestr = strtok(features, ","); > featurestr; > featurestr = strtok(NULL, ",")) { > const char *name; > const char *val = NULL; > char *eq = NULL; > uint32_t numvalue; > char num[32]; > > /* Compatibility syntax: */ > if (featurestr[0] == '+') { > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > continue; > } else if (featurestr[0] == '-') { > add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); > continue; > } > > eq = strchr(featurestr, '='); > if (eq) { > *eq++ = 0; > val = eq; > } else { > val = "on"; > } > > feat2prop(featurestr); > name = featurestr; > > /* Special cases: */ > if (!strcmp(name, "xlevel")) { > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > error_setg(errp, "bad numerical value %s", val); > return; > } > if (numvalue < 0x80000000) { > error_report("xlevel value shall always be >= 0x80000000" > ", fixup will be removed in future versions"); > numvalue += 0x80000000; > } > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > val = num; > } else if (!strcmp(name, "tsc-freq")) { > int64_t tsc_freq; > > tsc_freq = qemu_strtosz_suffix_unit(val, &err, > QEMU_STRTOSZ_DEFSUFFIX_B, 1000); > if (tsc_freq < 0 || *err) { > error_setg(errp, "bad numerical value %s", val); > return; > } > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > val = num; > name = "tsc-frequency"; > } else if (!strcmp(name, "hv-spinlocks")) { > const int min = 0xFFF; > > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > error_setg(errp, "bad numerical value %s", val); > return; > } > if (numvalue < min) { > error_report("hv-spinlocks value shall always be >= 0x%x" > ", fixup will be removed in future versions", > min); > numvalue = min; > } > snprintf(num, sizeof(num), "%" PRId32, numvalue); > val = num; > } > > object_property_parse(OBJECT(cpu), val, name, &local_err); > > if (local_err) { > error_propagate(errp, local_err); > return; > } > } > } > > > > + > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > -- > > 1.8.3.1 > > >
On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: > On Wed, 1 Jun 2016 15:46:20 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > target-i386/cpu.c | 32 +++++++++++++++++--------------- > > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 238f69d..618aef9 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > > X86CPU *cpu = X86_CPU(cs); > > > char *featurestr; /* Single 'key=value" string being parsed */ > > > uint32_t numvalue; > > > + char num[32]; > > > + const char *name; > > > + char *err; > > > Error *local_err = NULL; > > > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > > > while (featurestr) { > > > - char *val; > > > + char *val = NULL; > > > if (featurestr[0] == '+') { > > > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > > > } else if (featurestr[0] == '-') { > > > @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > > } else if ((val = strchr(featurestr, '='))) { > > > *val = 0; val++; > > > feat2prop(featurestr); > > > + name = featurestr; > > > if (!strcmp(featurestr, "xlevel")) { > > > - char *err; > > > - char num[32]; > > > - > > > numvalue = strtoul(val, &err, 0); > > > if (!*val || *err) { > > > error_setg(errp, "bad numerical value %s", val); > > > @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > > numvalue += 0x80000000; > > > } > > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > > - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > > > + val = num; > > > } else if (!strcmp(featurestr, "tsc-freq")) { > > > int64_t tsc_freq; > > > - char *err; > > > - char num[32]; > > > > > > tsc_freq = qemu_strtosz_suffix_unit(val, &err, > > > QEMU_STRTOSZ_DEFSUFFIX_B, 1000); > > > @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > > return; > > > } > > > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > > > - object_property_parse(OBJECT(cpu), num, "tsc-frequency", > > > - &local_err); > > > + val = num; > > > + name = "tsc-frequency"; > > > } else if (!strcmp(featurestr, "hv-spinlocks")) { > > > - char *err; > > > const int min = 0xFFF; > > > - char num[32]; > > > + > > > numvalue = strtoul(val, &err, 0); > > > if (!*val || *err) { > > > error_setg(errp, "bad numerical value %s", val); > > > @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > > numvalue = min; > > > } > > > snprintf(num, sizeof(num), "%" PRId32, numvalue); > > > - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > > > - } else { > > > - object_property_parse(OBJECT(cpu), val, featurestr, &local_err); > > > + val = num; > > > } > > > } else { > > > feat2prop(featurestr); > > > - object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); > > > + name = featurestr; > > > + val = (char *)"on"; > > > > You don't need this hack if you do something like this: > > > > > > while (featurestr) { > > - char *val = NULL; > > + const char *val = NULL; > > + char *eq = NULL; > > if (featurestr[0] == '+') { > > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > > } else if (featurestr[0] == '-') { > > add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); > > - } else if ((val = strchr(featurestr, '='))) { > > - *val = 0; val++; > > + } else if ((eq = strchr(featurestr, '='))) { > > + *eq++ = 0; > > + val = eq; > > feat2prop(featurestr); > > name = featurestr; > > if (!strcmp(featurestr, "xlevel")) { > > > > > > > } > > > + > > > + if (val) { > > > + object_property_parse(OBJECT(cpu), val, name, &local_err); > > > + } > > > > I would find it easier to read if this part was inside the same > > block as the code that sets name/val. > > > > I would find it even better if object_property_parse() was in the > > main loop body, and the +feature/-feature code placed inside an > > if/continue branch (to make it clear that it is an exception, not > > the rule). Like this: > > Thanks, > I'll take your variant with your permission. Sure: Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > /* untested */ > > static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > Error **errp) > > { > > X86CPU *cpu = X86_CPU(cs); > > char *featurestr; /* Single 'key=value" string being parsed */ > > char *err; > > Error *local_err = NULL; > > > > if (!features) { > > return; > > } > > > > for (featurestr = strtok(features, ","); > > featurestr; > > featurestr = strtok(NULL, ",")) { > > const char *name; > > const char *val = NULL; > > char *eq = NULL; > > uint32_t numvalue; > > char num[32]; > > > > /* Compatibility syntax: */ > > if (featurestr[0] == '+') { > > add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); > > continue; > > } else if (featurestr[0] == '-') { > > add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); > > continue; > > } > > > > eq = strchr(featurestr, '='); > > if (eq) { > > *eq++ = 0; > > val = eq; > > } else { > > val = "on"; > > } > > > > feat2prop(featurestr); > > name = featurestr; > > > > /* Special cases: */ > > if (!strcmp(name, "xlevel")) { > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > return; > > } > > if (numvalue < 0x80000000) { > > error_report("xlevel value shall always be >= 0x80000000" > > ", fixup will be removed in future versions"); > > numvalue += 0x80000000; > > } > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > val = num; > > } else if (!strcmp(name, "tsc-freq")) { > > int64_t tsc_freq; > > > > tsc_freq = qemu_strtosz_suffix_unit(val, &err, > > QEMU_STRTOSZ_DEFSUFFIX_B, 1000); > > if (tsc_freq < 0 || *err) { > > error_setg(errp, "bad numerical value %s", val); > > return; > > } > > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > > val = num; > > name = "tsc-frequency"; > > } else if (!strcmp(name, "hv-spinlocks")) { > > const int min = 0xFFF; > > > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > return; > > } > > if (numvalue < min) { > > error_report("hv-spinlocks value shall always be >= 0x%x" > > ", fixup will be removed in future versions", > > min); > > numvalue = min; > > } > > snprintf(num, sizeof(num), "%" PRId32, numvalue); > > val = num; > > } > > > > object_property_parse(OBJECT(cpu), val, name, &local_err); > > > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > } > > } > > } > > > > > > > + > > > if (local_err) { > > > error_propagate(errp, local_err); > > > return; > > > -- > > > 1.8.3.1 > > > > > >
(CCing libvirt folks) BTW: On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: [...] > > /* Special cases: */ > > if (!strcmp(name, "xlevel")) { > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > return; > > } > > if (numvalue < 0x80000000) { > > error_report("xlevel value shall always be >= 0x80000000" > > ", fixup will be removed in future versions"); > > numvalue += 0x80000000; > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > val = num; [...] > > } else if (!strcmp(name, "hv-spinlocks")) { > > const int min = 0xFFF; > > > > numvalue = strtoul(val, &err, 0); > > if (!*val || *err) { > > error_setg(errp, "bad numerical value %s", val); > > return; > > } > > if (numvalue < min) { > > error_report("hv-spinlocks value shall always be >= 0x%x" > > ", fixup will be removed in future versions", > > min); > > numvalue = min; > > } Those "fixup will be removed in future versions" warnings are present since QEMU 1.7. Assuming that libvirt never allowed those invalid values to be used in the configuration (did it?), I believe we can safely remove the hv-spinlocks and xlevel fixups in QEMU 2.7. The hv-spinlocks setter already rejects invalid values. We just need to make x86_cpu_realizefn() reject invalid xlevel values.
On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote: > (CCing libvirt folks) > > BTW: > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: > [...] > > > /* Special cases: */ > > > if (!strcmp(name, "xlevel")) { > > > numvalue = strtoul(val, &err, 0); > > > if (!*val || *err) { > > > error_setg(errp, "bad numerical value %s", val); > > > return; > > > } > > > if (numvalue < 0x80000000) { > > > error_report("xlevel value shall always be >= 0x80000000" > > > ", fixup will be removed in future versions"); > > > numvalue += 0x80000000; > > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > > val = num; > [...] > > > } else if (!strcmp(name, "hv-spinlocks")) { > > > const int min = 0xFFF; > > > > > > numvalue = strtoul(val, &err, 0); > > > if (!*val || *err) { > > > error_setg(errp, "bad numerical value %s", val); > > > return; > > > } > > > if (numvalue < min) { > > > error_report("hv-spinlocks value shall always be >= 0x%x" > > > ", fixup will be removed in future versions", > > > min); > > > numvalue = min; > > > } > > Those "fixup will be removed in future versions" warnings are > present since QEMU 1.7. Assuming that libvirt never allowed those > invalid values to be used in the configuration (did it?), I > believe we can safely remove the hv-spinlocks and xlevel fixups > in QEMU 2.7. I couldn't find anything regarding xlevel (so we might actually not support it at all), but we indeed do limit the hv_spinlock count: if (def->hyperv_spinlocks < 0xFFF) { virReportError(VIR_ERR_XML_ERROR, "%s", _("HyperV spinlock retry count must be " "at least 4095")); goto error; } Peter
On Thu, 2 Jun 2016 17:05:06 +0200 Peter Krempa <pkrempa@redhat.com> wrote: > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote: > > (CCing libvirt folks) > > > > BTW: > > > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: > > [...] > > > > /* Special cases: */ > > > > if (!strcmp(name, "xlevel")) { > > > > numvalue = strtoul(val, &err, 0); > > > > if (!*val || *err) { > > > > error_setg(errp, "bad numerical value %s", val); > > > > return; > > > > } > > > > if (numvalue < 0x80000000) { > > > > error_report("xlevel value shall always be >= 0x80000000" > > > > ", fixup will be removed in future versions"); > > > > numvalue += 0x80000000; > > > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > > > val = num; > > [...] > > > > } else if (!strcmp(name, "hv-spinlocks")) { > > > > const int min = 0xFFF; > > > > > > > > numvalue = strtoul(val, &err, 0); > > > > if (!*val || *err) { > > > > error_setg(errp, "bad numerical value %s", val); > > > > return; > > > > } > > > > if (numvalue < min) { > > > > error_report("hv-spinlocks value shall always be >= 0x%x" > > > > ", fixup will be removed in future versions", > > > > min); > > > > numvalue = min; > > > > } > > > > Those "fixup will be removed in future versions" warnings are > > present since QEMU 1.7. Assuming that libvirt never allowed those > > invalid values to be used in the configuration (did it?), I > > believe we can safely remove the hv-spinlocks and xlevel fixups > > in QEMU 2.7. > > I couldn't find anything regarding xlevel (so we might actually not > support it at all), but we indeed do limit the hv_spinlock count: > > > if (def->hyperv_spinlocks < 0xFFF) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("HyperV spinlock retry count must be " > "at least 4095")); > goto error; > } > > Peter Peter, Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax or canonical property syntax there feat1=on,feat2=off
On Thu, 2 Jun 2016 11:53:22 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > (CCing libvirt folks) > > BTW: > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: > [...] > > > /* Special cases: */ > > > if (!strcmp(name, "xlevel")) { > > > numvalue = strtoul(val, &err, 0); > > > if (!*val || *err) { > > > error_setg(errp, "bad numerical value %s", val); > > > return; > > > } > > > if (numvalue < 0x80000000) { > > > error_report("xlevel value shall always be >= 0x80000000" > > > ", fixup will be removed in future versions"); > > > numvalue += 0x80000000; > > > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > > > val = num; > [...] > > > } else if (!strcmp(name, "hv-spinlocks")) { > > > const int min = 0xFFF; > > > > > > numvalue = strtoul(val, &err, 0); > > > if (!*val || *err) { > > > error_setg(errp, "bad numerical value %s", val); > > > return; > > > } > > > if (numvalue < min) { > > > error_report("hv-spinlocks value shall always be >= 0x%x" > > > ", fixup will be removed in future versions", > > > min); > > > numvalue = min; > > > } > > Those "fixup will be removed in future versions" warnings are > present since QEMU 1.7. Assuming that libvirt never allowed those > invalid values to be used in the configuration (did it?), I > believe we can safely remove the hv-spinlocks and xlevel fixups > in QEMU 2.7. > > The hv-spinlocks setter already rejects invalid values. We just > need to make x86_cpu_realizefn() reject invalid xlevel values. I'll leave axing them to you.
On Thu, Jun 02, 2016 at 18:31:04 +0200, Igor Mammedov wrote: > On Thu, 2 Jun 2016 17:05:06 +0200 > Peter Krempa <pkrempa@redhat.com> wrote: > > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote: > > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: [...] > > I couldn't find anything regarding xlevel (so we might actually not > > support it at all), but we indeed do limit the hv_spinlock count: > > > > > > if (def->hyperv_spinlocks < 0xFFF) { > > virReportError(VIR_ERR_XML_ERROR, "%s", > > _("HyperV spinlock retry count must be " > > "at least 4095")); > > goto error; > > } > > > > Peter > Peter, > Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax > or canonical property syntax there feat1=on,feat2=off We use the legacy one: -cpu core2duo,+ds,+acpi,+ht,+tm,+ds_cpl, ... and -cpu 'qemu32,hv_relaxed,hv_vapic, ... for the hyperv features. We probably can switch to the new one if there's a reasonable way how to detect that qemu is supporting the new one. Peter
On Fri, 3 Jun 2016 09:30:09 +0200 Peter Krempa <pkrempa@redhat.com> wrote: > On Thu, Jun 02, 2016 at 18:31:04 +0200, Igor Mammedov wrote: > > On Thu, 2 Jun 2016 17:05:06 +0200 > > Peter Krempa <pkrempa@redhat.com> wrote: > > > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote: > > > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: > > [...] > > > > I couldn't find anything regarding xlevel (so we might actually not > > > support it at all), but we indeed do limit the hv_spinlock count: > > > > > > > > > if (def->hyperv_spinlocks < 0xFFF) { > > > virReportError(VIR_ERR_XML_ERROR, "%s", > > > _("HyperV spinlock retry count must be " > > > "at least 4095")); > > > goto error; > > > } > > > > > > Peter > > Peter, > > Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax > > or canonical property syntax there feat1=on,feat2=off > > We use the legacy one: > > -cpu core2duo,+ds,+acpi,+ht,+tm,+ds_cpl, ... > > and > > -cpu 'qemu32,hv_relaxed,hv_vapic, ... > > for the hyperv features. > > We probably can switch to the new one if there's a reasonable way how to > detect that qemu is supporting the new one. for x86 features became properties since 2.4 release (commit 38e5c119), that's the one way to know it. But it's still only +-features for sparc (that's the last remaining target that has legacy parsing). Another way to detect it is to probe via QOM if CPU has a property corresponding to a feature. Maybe Eduardo knows about other ways to do it. > > Peter >
On Fri, Jun 03, 2016 at 11:37:49AM +0200, Igor Mammedov wrote: > On Fri, 3 Jun 2016 09:30:09 +0200 > Peter Krempa <pkrempa@redhat.com> wrote: > > > On Thu, Jun 02, 2016 at 18:31:04 +0200, Igor Mammedov wrote: > > > On Thu, 2 Jun 2016 17:05:06 +0200 > > > Peter Krempa <pkrempa@redhat.com> wrote: > > > > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote: > > > > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote: > > > > [...] > > > > > > I couldn't find anything regarding xlevel (so we might actually not > > > > support it at all), but we indeed do limit the hv_spinlock count: > > > > > > > > > > > > if (def->hyperv_spinlocks < 0xFFF) { > > > > virReportError(VIR_ERR_XML_ERROR, "%s", > > > > _("HyperV spinlock retry count must be " > > > > "at least 4095")); > > > > goto error; > > > > } > > > > > > > > Peter > > > Peter, > > > Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax > > > or canonical property syntax there feat1=on,feat2=off > > > > We use the legacy one: > > > > -cpu core2duo,+ds,+acpi,+ht,+tm,+ds_cpl, ... > > > > and > > > > -cpu 'qemu32,hv_relaxed,hv_vapic, ... > > > > for the hyperv features. > > > > We probably can switch to the new one if there's a reasonable way how to > > detect that qemu is supporting the new one. > for x86 features became properties since 2.4 release (commit 38e5c119), > that's the one way to know it. > But it's still only +-features for sparc (that's the last remaining > target that has legacy parsing). > > Another way to detect it is to probe via QOM if CPU has a property corresponding > to a feature. > > Maybe Eduardo knows about other ways to do it. The properties could be detected by running device-list-properties when libvirt is probing for QEMU binary capabilities. But we still don't return any useful information for abstract classes or X86CPU subclasses. You could work around it by running qom-list on the first VCPU object, but when libvirt probes QEMU binary capabilities, it uses -machine none (which doesn't create any VCPU). I will look for other mechanisms that can be used in QEMU 2.{4,5,6} already, but I am not sure we will find one. If all fails, we can consider making query-command-line-options return useful data for -cpu on QEMU 2.7.
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 238f69d..618aef9 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, X86CPU *cpu = X86_CPU(cs); char *featurestr; /* Single 'key=value" string being parsed */ uint32_t numvalue; + char num[32]; + const char *name; + char *err; Error *local_err = NULL; featurestr = features ? strtok(features, ",") : NULL; while (featurestr) { - char *val; + char *val = NULL; if (featurestr[0] == '+') { add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err); } else if (featurestr[0] == '-') { @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, } else if ((val = strchr(featurestr, '='))) { *val = 0; val++; feat2prop(featurestr); + name = featurestr; if (!strcmp(featurestr, "xlevel")) { - char *err; - char num[32]; - numvalue = strtoul(val, &err, 0); if (!*val || *err) { error_setg(errp, "bad numerical value %s", val); @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, numvalue += 0x80000000; } snprintf(num, sizeof(num), "%" PRIu32, numvalue); - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); + val = num; } else if (!strcmp(featurestr, "tsc-freq")) { int64_t tsc_freq; - char *err; - char num[32]; tsc_freq = qemu_strtosz_suffix_unit(val, &err, QEMU_STRTOSZ_DEFSUFFIX_B, 1000); @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, return; } snprintf(num, sizeof(num), "%" PRId64, tsc_freq); - object_property_parse(OBJECT(cpu), num, "tsc-frequency", - &local_err); + val = num; + name = "tsc-frequency"; } else if (!strcmp(featurestr, "hv-spinlocks")) { - char *err; const int min = 0xFFF; - char num[32]; + numvalue = strtoul(val, &err, 0); if (!*val || *err) { error_setg(errp, "bad numerical value %s", val); @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, numvalue = min; } snprintf(num, sizeof(num), "%" PRId32, numvalue); - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); - } else { - object_property_parse(OBJECT(cpu), val, featurestr, &local_err); + val = num; } } else { feat2prop(featurestr); - object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); + name = featurestr; + val = (char *)"on"; } + + if (val) { + object_property_parse(OBJECT(cpu), val, name, &local_err); + } + if (local_err) { error_propagate(errp, local_err); return;
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)