From patchwork Mon Feb 18 21:27:16 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eduardo Habkost X-Patchwork-Id: 221499 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id AE6EF2C0291 for ; Tue, 19 Feb 2013 08:25:59 +1100 (EST) Received: from localhost ([::1]:39895 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U7YDl-0002v6-RR for incoming@patchwork.ozlabs.org; Mon, 18 Feb 2013 16:25:57 -0500 Received: from eggs.gnu.org ([208.118.235.92]:54030) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U7YDZ-0002uc-83 for qemu-devel@nongnu.org; Mon, 18 Feb 2013 16:25:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U7YDW-0004Pr-RV for qemu-devel@nongnu.org; Mon, 18 Feb 2013 16:25:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:28039) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U7YDW-0004Pk-7M for qemu-devel@nongnu.org; Mon, 18 Feb 2013 16:25:42 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1ILPejd015587 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 18 Feb 2013 16:25:40 -0500 Received: from blackpad.lan.raisama.net (vpn1-4-163.gru2.redhat.com [10.97.4.163]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r1ILPdJY026522; Mon, 18 Feb 2013 16:25:39 -0500 Received: by blackpad.lan.raisama.net (Postfix, from userid 500) id E475F204B7D; Mon, 18 Feb 2013 18:27:16 -0300 (BRT) Date: Mon, 18 Feb 2013 18:27:16 -0300 From: Eduardo Habkost To: Andreas =?iso-8859-1?Q?F=E4rber?= Message-ID: <20130218212716.GT8494@otherpad.lan.raisama.net> References: <1360600511-25133-1-git-send-email-imammedo@redhat.com> <1360600511-25133-2-git-send-email-imammedo@redhat.com> <20130218203049.GA16618@otherpad.lan.raisama.net> <512290CC.2000804@suse.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <512290CC.2000804@suse.de> X-Fnord: you can see the fnord User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-MIME-Autoconverted: from 8bit to quoted-printable by mx1.redhat.com id r1ILPejd015587 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Jiri Denemark , Igor Mammedov , Don@cloudswitch.com, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH 1/9] target-i386: cpu: convert existing dynamic properties into static properties X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Mon, Feb 18, 2013 at 09:36:28PM +0100, Andreas Färber wrote: > Am 18.02.2013 21:30, schrieb Eduardo Habkost: > > On Mon, Feb 11, 2013 at 05:35:03PM +0100, Igor Mammedov wrote: > >> Following properties are converted: > >> * vendor > >> * xlevel > >> * custom setter/getter replaced by qdev's DEFINE_PROP_UINT32 > >> * level > >> * custom setter/getter replaced by qdev's DEFINE_PROP_UINT32 > >> * tsc-frequency > >> * stepping > >> * model > >> * family > >> * model-id > >> * check "if (model_id == NULL)" looks unnecessary now, since all > >> builtin model-ids are not NULL and user shouldn't be able to set > >> it NULL (cpumodel string parsing code takes care of it, if feature > >> is specified as "model-id=" on command line, its parsing will > >> result in an empty string as value). > >> * use g_malloc0() instead of g_malloc() in x86_cpuid_get_model_id() > >> > >> Common changes to all properties: > >> * properties code are moved to the top of file, before properties array > >> definition > >> * s/error_set/error_setg/;s/QERR*/with similar message/ > > > > Why? > > Been having a similar question (sorry for not replying yet): > > Can't we leave my new-style getters and setters in place and invoke them > through some glue code from whatever old-style machinery qdev static > properties still use? I don't understand your question. What do you mean by "my new-style getters and setters"? Igor's patch keeps the existing getters/setters almost the same (I manually run "diff" to check that, see the end of this message), except for the error_set() calls, the small differences I pointed out in my message, and extra visit_type_str() calls required for the string getter/setters (that have a different signature from object_property_add_str() getter/setters). > > BTW I'm not yet clear on how we should proceed with subclasses and KVM. > Proposing we proceed with your properties refactoring after all, then > maybe it becomes more clear where the problems are. I believe Igor will send a new version soon. I completely agree with having KVM-specific and TCG-specific classes, because they _are_ different CPU models. The same reasons we have[1] to make "-cpu SandyBridge" and "-cpu Haswell" different classes, apply to making "-disable-kvm -cpu SandyBridge" and "-enable-kvm -cpu SandyBridge" different classes as well. About the default for the "vendor" property: right now it is not an issue because Igor's patches are not setting any externally-visible default value for the "vendor" property (so this is just an implementation detail), and now I am happy with either of the solutions we have been discussing (as my questions about QOM design have been answered). In either case, when we actually get to set a introspectable default for the property, I hope to have heard additional feedback from other people (especially from the libvirt folks). [1] The reasons I remember are: 1) Allowing CPU model definition probing, by simply querying the default values of properties for each class; * Rationale for separating TCG/KVM: the resulting CPU features are different when running KVM and when running TCG, so they are in practice different CPU models. For example: differences between "-enable-kvm -cpu SandyBridge" and "-disable-kvm -cpu SandyBridge" are huge when compared to the small differences between "-enable-kvm -cpu SandyBridge" and "-enable-kvm -cpu Haswell". 2) Allowing machine-type compatibility to be specified easily using global properties on the machine-type compat_props field. * Rationale for separating TCG/KVM: we may want to set a compatibility property only for TCG or only for KVM. For example, the n270 MOVBE fix we are going to include QEMU in 1.5 will need to disable MOVBE on pc-1.4, but only on the TCG CPU model, because KVM already had MOVBE working on pc-1.4. --- /tmp/oldsetters 2013-02-18 17:12:53.781477861 -0300 +++ /tmp/newsetters 2013-02-18 17:12:59.069477982 -0300 @@ -26,8 +26,9 @@ return; } if (value < min || value > max) { - error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, min, max); + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min" + "imum: %" PRId64 ", maximum: %" PRId64, + object_get_typename(obj), name, value, min, max); return; } @@ -39,6 +40,14 @@ } } +PropertyInfo qdev_prop_family = { + .name = "uint32", + .get = x86_cpuid_version_get_family, + .set = x86_cpuid_version_set_family, +}; +#define DEFINE_PROP_FAMILY(_n) \ + DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_family, uint32_t) + static void x86_cpuid_version_get_model(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) { @@ -65,8 +74,9 @@ return; } if (value < min || value > max) { - error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, min, max); + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min" + "imum: %" PRId64 ", maximum: %" PRId64, + object_get_typename(obj), name, value, min, max); return; } @@ -74,6 +84,14 @@ env->cpuid_version |= ((value & 0xf) << 4) | ((value >> 4) << 16); } +PropertyInfo qdev_prop_model = { + .name = "uint32", + .get = x86_cpuid_version_get_model, + .set = x86_cpuid_version_set_model, +}; +#define DEFINE_PROP_MODEL(_n) \ + DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_model, uint32_t) + static void x86_cpuid_version_get_stepping(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) @@ -101,8 +119,9 @@ return; } if (value < min || value > max) { - error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, min, max); + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min" + "imum: %" PRId64 ", maximum: %" PRId64, + object_get_typename(obj), name, value, min, max); return; } @@ -110,39 +129,16 @@ env->cpuid_version |= value & 0xf; } -static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaque, - const char *name, Error **errp) -{ - X86CPU *cpu = X86_CPU(obj); - - visit_type_uint32(v, &cpu->env.cpuid_level, name, errp); -} +PropertyInfo qdev_prop_stepping = { + .name = "uint32", + .get = x86_cpuid_version_get_stepping, + .set = x86_cpuid_version_set_stepping, +}; +#define DEFINE_PROP_STEPPING(_n) \ + DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_stepping, uint32_t) -static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaque, - const char *name, Error **errp) -{ - X86CPU *cpu = X86_CPU(obj); - - visit_type_uint32(v, &cpu->env.cpuid_level, name, errp); -} - -static void x86_cpuid_get_xlevel(Object *obj, Visitor *v, void *opaque, - const char *name, Error **errp) -{ - X86CPU *cpu = X86_CPU(obj); - - visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp); -} - -static void x86_cpuid_set_xlevel(Object *obj, Visitor *v, void *opaque, - const char *name, Error **errp) -{ - X86CPU *cpu = X86_CPU(obj); - - visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp); -} - -static char *x86_cpuid_get_vendor(Object *obj, Error **errp) +static void x86_cpuid_get_vendor(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) { X86CPU *cpu = X86_CPU(obj); CPUX86State *env = &cpu->env; @@ -151,19 +147,27 @@ value = (char *)g_malloc(CPUID_VENDOR_SZ + 1); x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2, env->cpuid_vendor3); - return value; + visit_type_str(v, &value, name, errp); + g_free(value); } -static void x86_cpuid_set_vendor(Object *obj, const char *value, - Error **errp) +static void x86_cpuid_set_vendor(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) { X86CPU *cpu = X86_CPU(obj); CPUX86State *env = &cpu->env; + char *value; int i; + visit_type_str(v, &value, name, errp); + if (error_is_set(errp)) { + return; + } + if (strlen(value) != CPUID_VENDOR_SZ) { - error_set(errp, QERR_PROPERTY_VALUE_BAD, "", - "vendor", value); + error_setg(errp, "Property '%s.%s' doesn't take value '%s'", + object_get_typename(obj), name, value); + g_free(value); return; } @@ -171,47 +175,73 @@ env->cpuid_vendor2 = 0; env->cpuid_vendor3 = 0; for (i = 0; i < 4; i++) { - env->cpuid_vendor1 |= ((uint8_t)value[i ]) << (8 * i); + env->cpuid_vendor1 |= ((uint8_t)value[i]) << (8 * i); env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i); env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i); } + g_free(value); +} + +PropertyInfo qdev_prop_vendor = { + .name = "string", + .get = x86_cpuid_get_vendor, + .set = x86_cpuid_set_vendor, +}; +#define DEFINE_PROP_VENDOR(_n) { \ + .name = _n, \ + .info = &qdev_prop_vendor \ } -static char *x86_cpuid_get_model_id(Object *obj, Error **errp) +static void x86_cpuid_get_model_id(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) { X86CPU *cpu = X86_CPU(obj); CPUX86State *env = &cpu->env; char *value; int i; - value = g_malloc(48 + 1); + value = g_malloc0(48 + 1); for (i = 0; i < 48; i++) { value[i] = env->cpuid_model[i >> 2] >> (8 * (i & 3)); } - value[48] = '\0'; - return value; + visit_type_str(v, &value, name, errp); + g_free(value); } -static void x86_cpuid_set_model_id(Object *obj, const char *model_id, - Error **errp) +static void x86_cpuid_set_model_id(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) { X86CPU *cpu = X86_CPU(obj); CPUX86State *env = &cpu->env; int c, len, i; + char *value; - if (model_id == NULL) { - model_id = ""; + visit_type_str(v, &value, name, errp); + if (error_is_set(errp)) { + return; } - len = strlen(model_id); + + len = strlen(value); memset(env->cpuid_model, 0, 48); for (i = 0; i < 48; i++) { if (i >= len) { c = '\0'; } else { - c = (uint8_t)model_id[i]; + c = (uint8_t)value[i]; } env->cpuid_model[i >> 2] |= c << (8 * (i & 3)); } + g_free(value); +} + +PropertyInfo qdev_prop_model_id = { + .name = "string", + .get = x86_cpuid_get_model_id, + .set = x86_cpuid_set_model_id, +}; +#define DEFINE_PROP_MODEL_ID(_n) { \ + .name = _n, \ + .info = &qdev_prop_model_id \ } static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, void *opaque, @@ -237,11 +267,20 @@ return; } if (value < min || value > max) { - error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, min, max); + error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min" + "imum: %" PRId64 ", maximum: %" PRId64, + object_get_typename(obj), name, value, min, max); return; } cpu->env.tsc_khz = value / 1000; } +PropertyInfo qdev_prop_tsc_freq = { + .name = "int64", + .get = x86_cpuid_get_tsc_freq, + .set = x86_cpuid_set_tsc_freq, +}; +#define DEFINE_PROP_TSC_FREQ(_n) \ + DEFINE_PROP(_n, X86CPU, env.tsc_khz, qdev_prop_tsc_freq, int32_t) +