From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60699) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yi1gx-0007NR-JB for qemu-devel@nongnu.org; Tue, 14 Apr 2015 10:20:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yi1gr-0005OB-Km for qemu-devel@nongnu.org; Tue, 14 Apr 2015 10:19:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yi1gr-0005O0-Dj for qemu-devel@nongnu.org; Tue, 14 Apr 2015 10:19:49 -0400 Date: Tue, 14 Apr 2015 16:19:43 +0200 From: Igor Mammedov Message-ID: <20150414161943.355c36d1@nial.brq.redhat.com> In-Reply-To: <20150414140839.GA3466@thinpad.lan.raisama.net> References: <1428696149-12461-1-git-send-email-ehabkost@redhat.com> <20150414134919.4f084e2d@nial.brq.redhat.com> <20150414140839.GA3466@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] target-i386: Register QOM properties for feature flags List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Paolo Bonzini , Jiri Denemark , qemu-devel@nongnu.org, Andreas =?UTF-8?B?RsOkcmJlcg==?= On Tue, 14 Apr 2015 11:08:39 -0300 Eduardo Habkost wrote: [...] > > > +/* Register a boolean feature-bits property. > > > + * If mask has multiple bits, all must be set for the property to return true. > > > + * The same property name can be registered multiple times to make it affect > > > + * multiple bits in the same FeatureWord. > > > + */ > > > +static void x86_cpu_register_feature_prop(X86CPU *cpu, > > > + const char *prop_name, > > > + FeatureWord w, > > > + uint32_t mask) > > isn't used as mask by caller, s/mask/bit/ ??? > > There will be an use case for mask containing multiple bits, later. My > plan is to remove the duplicate "kvmclock" alias from kvm_feature_name, > and call this manually: > > x86_cpu_register_feature_prop(cpu, "kvmclock", FEAT_KVM, > (1 << KVM_FEATURE_CLOCKSOURCE) | > (1 << KVM_FEATURE_CLOCKSOURCE2)); make it mask when it starts to be used as such > > I didn't do that yet because I need the existing > x86_cpu_parse_featurestr() code to keep working until it is converted to > use object_property_set(). > [...] > > > + op = object_property_find(OBJECT(cpu), prop_name, NULL); > > > + if (op) { > > > + fp = op->opaque; > > > + assert(fp->word == w); > > > + fp->mask |= mask; > > ^^^ This is the block of code that will be removed once I add the manual > "kvmclock" registration call I mentioned above. > > > > + } else { > > > + fp = g_new0(FeatureProperty, 1); > > > + fp->word = w; > > > + fp->mask = mask; > > > + object_property_add(OBJECT(cpu), prop_name, "bool", > > > + x86_cpu_get_feature_prop, > > > + x86_cpu_set_feature_prop, > > > + x86_cpu_release_feature_prop, fp, &error_abort); > > > + } > > > +} > > > + > > > +static void x86_cpu_register_feature_bit_props(X86CPU *cpu, > > this adds 1 property and possibly aliases, _props() is confusing here. > > Alias properties are still properties like any other, aren't they? The > function is still responsible for registering multiple properties. Is > the "_props()" suffix really that confusing? technically aliases are properties but from user pov it's the same property just with another name. > > > > I'd rename it to x86_cpu_add_feature_bit_prop() and inline > > above x86_cpu_register_feature_prop() since it's not going to be reused > > I prefer to keep the single-property function separated, as it may > become a generic bitmap property registration function inside generic > QOM code later. With your feature_word_ptr suggestion, it would be even > more generic and non-x86-specific. > > (To be honest, I would prefer to keep the single-property function > registration code clearly separated even if it was never going to be > reused anywhere. 20-line functions are already too long for my taste.) ok [...]