From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6xmf-0006Ur-Ei for qemu-devel@nongnu.org; Wed, 07 Aug 2013 03:03:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6xmY-0005DV-0K for qemu-devel@nongnu.org; Wed, 07 Aug 2013 03:03:49 -0400 Message-ID: <5201F146.5060400@suse.de> Date: Wed, 07 Aug 2013 09:03:34 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1375777673-20274-1-git-send-email-aik@ozlabs.ru> <1375777673-20274-6-git-send-email-aik@ozlabs.ru> <5200C789.5030802@suse.de> <5201E3F4.8070409@ozlabs.ru> In-Reply-To: <5201E3F4.8070409@ozlabs.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 5/6] xics: split to xics and xics-common List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Anthony Liguori , Alexander Graf , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Paul Mackerras , David Gibson Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy: > On 08/06/2013 07:53 PM, Andreas F=C3=A4rber wrote: >> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy: >>> + } >>> +} >>> + >>> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v, >>> + void *opaque, const char *name, struct Er= ror **errp) >> >> You should be able to drop both "struct"s. >=20 > Yeah, fixed. This is just how the ObjectPropertyAccessor type is define= d. Yeah, reason is some circular header dependencies. ;) >>> +static void xics_common_initfn(Object *obj) >>> +{ >>> + object_property_add(obj, "nr_irqs", "int", >>> + NULL, xics_prop_set_nr_irqs, NULL, NULL, NUL= L); >>> + object_property_add(obj, "nr_servers", "int", >>> + NULL, xics_prop_set_nr_servers, NULL, NULL, = NULL); >> >> Since this property is visible in the QOM tree, it would be nice to >> implement trivial getters returns the value from the state fields. >=20 > Added. How do I test it? ./QMP/qom-list to find the path, if not fixed in code yet, and ./QMP/qom-get path.nr_servers to retrieve the value, ./QMP/qom-set path.nr_servers to verify it doesn't kill the process. -qmp unix:./qmp-sock,server,nowait is what I use on the QEMU side. > "info qtree" prints only > DEVICE_CLASS(class)->props which is null in this case. True, info qtree is from qdev times and deprecated. I recently attached a "qom-tree" script doing the equivalent that you could try. I'll try to address -device xics,? showing them for 1.7. (Anthony indicated on a KVM call that the expected way to discover properties was to instantiate the type rather than looking at its class. Requires that types are instantiatable without asserting KVM accelerator, which gets initialized later.) >>> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRE= nvironment *spapr, >>> /* >>> * XICS >>> */ >>> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs) >>> +{ >>> + icp->ics =3D ICS(object_new(TYPE_ICS)); >>> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), = NULL); >> >> Why create this single object in the setter? Can't you just create thi= s >> in the common type's instance_init similar to before but using >> object_initialize() instead of object_new() and >> object_property_set_bool() in the realizefn? >=20 > I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_K= VM > (oops, KVM is at the end, will fix it) types. >=20 > And I would really want not to create it in instance_init() as I want t= o > have the object created and all its properties initialized in one place= . Doing it in instance_init facilitates improving the setter to allow multiple uses as a follow-up patch, since it must only be created once. If you don't, then the child will not be present in the composition tree before setting this seemingly unrelated property. That seems worse to me than accessing a field in two places - instance_init will be run before any property setter. BTW these dynamic setters do not check automatically whether the object has already been realized. If you want the setter to return an Error in that case, you will need to check for DeviceState *dev =3D DEVICE(icp); dev->realized yourself. Not sure if that's the semantics you desire, but I guess so? Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg