[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [RFC 3/9] spapr: DEFINE_SPAPR_MACHINE
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [RFC 3/9] spapr: DEFINE_SPAPR_MACHINE |
Date: |
Tue, 1 Dec 2015 14:10:20 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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.
> 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.
>
> >+ 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.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[Qemu-ppc] [RFC 8/9] spapr: Improve setting of default machine version, David Gibson, 2015/11/30
[Qemu-ppc] [RFC 2/9] spapr: Rearrange versioned machine type code, David Gibson, 2015/11/30
[Qemu-ppc] [RFC 4/9] Move SET_MACHINE_COMPAT macro to boards.h, David Gibson, 2015/11/30