Re: [libvirt] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
(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. -- Eduardo
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, 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 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),
On Fri, 3 Jun 2016 09:30:09 +0200 Peter Krempa <pkrempa@redhat.com> wrote: 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. -- Eduardo
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.
The "fixup will be removed in future versions" warnings are present since QEMU 1.7.0, at least, so users should have fixed their scripts and configurations, already. In the case of libvirt users, libvirt doesn't use the "xlevel" option, and already rejects HyperV spinlock retry count < 0xFFF. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Igor, feel free to include this in the beginning of your series. I believe it will help making the patches simpler. --- target-i386/cpu.c | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index f3f95cd..940aa22 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1967,23 +1967,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, } else if ((val = strchr(featurestr, '='))) { *val = 0; val++; feat2prop(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); - 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); - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); - } else if (!strcmp(featurestr, "tsc-freq")) { + if (!strcmp(featurestr, "tsc-freq")) { int64_t tsc_freq; char *err; char num[32]; @@ -1997,23 +1981,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, snprintf(num, sizeof(num), "%" PRId64, tsc_freq); object_property_parse(OBJECT(cpu), num, "tsc-frequency", &local_err); - } 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); - 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); - object_property_parse(OBJECT(cpu), num, featurestr, &local_err); } else { object_property_parse(OBJECT(cpu), val, featurestr, &local_err); } -- 2.5.5
On 06/02/2016 10:55 AM, Eduardo Habkost wrote:
The "fixup will be removed in future versions" warnings are present since QEMU 1.7.0, at least, so users should have fixed their scripts and configurations, already.
In the case of libvirt users, libvirt doesn't use the "xlevel" option, and already rejects HyperV spinlock retry count < 0xFFF.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Igor, feel free to include this in the beginning of your series. I believe it will help making the patches simpler. --- target-i386/cpu.c | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eduardo Habkost -
Eric Blake -
Igor Mammedov -
Peter Krempa