|
From: | Alexey Kardashevskiy |
Subject: | Re: [Qemu-ppc] [RFC 3/9] spapr: DEFINE_SPAPR_MACHINE |
Date: | Tue, 1 Dec 2015 15:12:25 +1100 |
User-agent: | Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 12/01/2015 02:10 PM, David Gibson wrote:
On Tue, Dec 01, 2015 at 01:38:20PM +1100, Alexey Kardashevskiy wrote:On 11/30/2015 07:51 PM, David Gibson wrote:At the moment all the class_init functions and TypeInfo structures for the various versioned pseries machine types are open-coded. As more versions are created this is getting increasingly clumsy. This patch borrows the approach used in PC, using a DEFINE_SPAPR_MACHINE() macro to construct most of the boilerplate from simpler 'class_compat' and 'instance_compat' functions. This patch makes a small semantic change - the versioned machine types are now registered through machine_init() instead of type_init(). Since the new way is how PC already did it, I'm assuming that's correct. Signed-off-by: David Gibson <address@hidden> --- hw/ppc/spapr.c | 114 ++++++++++++++++++++++----------------------------------- 1 file changed, 44 insertions(+), 70 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index c126e10..ca62343 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2301,13 +2301,40 @@ static const TypeInfo spapr_machine_info = { }, }; +#define DEFINE_SPAPR_MACHINE(suffix, verstr, instance_compat) \instance_compat is passed directly (I like it) but spapr_machine_##suffix##_class_compat is not (I do not like it - cannot grep for it), would be nice to have these similar.Hrm. I was trying to avoid having lots of macro arguments that all have the same form.
PC passes 2 functions though. Both or neither will do.
Also, this could be: #define DEFINE_SPAPR_MACHINE(major, minor, instance_compat) and then use spapr_machine__##major##_##minor##_class_init() "pseries-"#major"."#minor ?I suppose, but I'd prefer to keep it similar to PC.
If it was DEFINE_PPC_MACHINE(), then I would not say a word against but in this case it would be just self-documenting that pseries machines are per QEMU versions.
+ static void spapr_machine_##suffix##_class_init(ObjectClass *oc, \ + void *data) \ + { \ + MachineClass *mc = MACHINE_CLASS(oc); \ + spapr_machine_##suffix##_class_compat(mc); \ + } \ + static void spapr_machine_##suffix##_instance_init(Object *obj) \ + { \ + void (*compat)(MachineState *m) = (instance_compat); \ + MachineState *machine = MACHINE(obj); \ + if (compat) { \ + compat(machine); \ + } \ + spapr_machine_initfn(obj); \I'd swap compat() and spapr_machine_initfn() and let a new machine overwrite some of default stuff.I was thinking about that, but I don't want to do it in this patch - I want to separate rearrangements from semantic changes to make review easier.+ } \ + static const TypeInfo spapr_machine_##suffix##_info = { \ + .name = MACHINE_TYPE_NAME("pseries-" verstr), \ + .parent = TYPE_SPAPR_MACHINE, \ + .class_init = spapr_machine_##suffix##_class_init, \ + .instance_init = spapr_machine_##suffix##_instance_init, \ + }; \ + static void spapr_machine_register_##suffix(void) \ + { \ + type_register(&spapr_machine_##suffix##_info); \ + } \ + machine_init(spapr_machine_register_##suffix) + /* * pseries-2.5 */ -static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data) +static void spapr_machine_2_5_class_compat(MachineClass *mc) { - MachineClass *mc = MACHINE_CLASS(oc); - sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc); mc->desc = "pSeries Logical Partition (PAPR compliant) v2.5"; mc->alias = "pseries"; @@ -2315,11 +2342,7 @@ static void spapr_machine_2_5_class_init(ObjectClass *oc, void *data) smc->dr_lmb_enabled = true; } -static const TypeInfo spapr_machine_2_5_info = { - .name = MACHINE_TYPE_NAME("pseries-2.5"), - .parent = TYPE_SPAPR_MACHINE, - .class_init = spapr_machine_2_5_class_init, -}; +DEFINE_SPAPR_MACHINE(2_5, "2.5", NULL); /* * pseries-2.4 @@ -2327,23 +2350,18 @@ static const TypeInfo spapr_machine_2_5_info = { #define SPAPR_COMPAT_2_4 \ HW_COMPAT_2_4 -static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data) +static void spapr_machine_2_4_class_compat(MachineClass *mc) { static GlobalProperty compat_props[] = { SPAPR_COMPAT_2_4 { /* end of list */ } }; - MachineClass *mc = MACHINE_CLASS(oc); mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4"; mc->compat_props = compat_props;xxx_init() is still a better name than xxx_compat(). Same about xxx_instance_compat().It's really not. I deliberately avoided calling things 'init' or 'initfn', because that's used in so many places and it's not clear whether I'm talking about class_init, instance_init, mc->init or something else.
These were "class_init" and "instance_init", not "something_init" so it was clear what is what. Now with "compat" I read it as they only do compatibility stuff (compatibilize? is there such a word?) and the rest is hidden somewhere else but the whole idea of machine versions is to support versions compatibility so using "compat" in the names is redundant. Or I am missing the point here...
-- Alexey
[Prev in Thread] | Current Thread | [Next in Thread] |