Re: [libvirt] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
9:53 a.m.
(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
10:05 a.m.
New subject: [libvirt] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
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
11:31 a.m.
New subject: [libvirt] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
On Thu, 2 Jun 2016 17:05:06 +0200
Peter Krempa <pkrempa(a)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
2:30 a.m.
New subject: [libvirt] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
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(a)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
4:37 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
On Fri, 3 Jun 2016 09:30:09 +0200
Peter Krempa <pkrempa(a)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(a)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
2:36 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
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(a)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(a)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
11:32 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr
On Thu, 2 Jun 2016 11:53:22 -0300
Eduardo Habkost <ehabkost(a)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.
11:55 a.m.
New subject: [libvirt] [PATCH] target-i386: Remove xlevel & hv-spinlocks option fixups
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(a)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
4:07 p.m.
New subject: [libvirt] [PATCH] target-i386: Remove xlevel & hv-spinlocks option fixups
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(a)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(a)redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3072
days inactive
3073
days old
8 comments
4 participants
participants (4)
-
Eduardo Habkost -
Eric Blake -
Igor Mammedov -
Peter Krempa