Re: [Qemu-devel] [PATCH 11/16] introduce ICC bus/device/bridge

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 22 Apr 2013 15:22:30 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 16.04.2013 00:12, schrieb Igor Mammedov:
> > ... to provide hotplug-able bus.
> > 
> > * icc-bridge will serve as a parent for icc-bus and provide
> >   mmio mapping services to child icc-devices.
> > * icc-device will replace SysBusDevice as a parent of APIC
> >   and IOAPIC devices.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   * Rebase on top the last HW reorganization series.
> >   * Move hw/icc_bus.c into hw/cpu/ and hw/icc_bus.h include/hw/i386/
> 
> http://wiki.qemu.org/QOMConventions
> 
> Comments below...
> 
[...]
> > +
> > +static void icc_device_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *k = DEVICE_CLASS(klass);
> > +
> > +    k->init = icc_device_init;
> 
> Please use k->realize for any new base device type or type derived from
> SysBusDevice.
> 
> If there's nothing to do at this level, you can just provide a dummy
> realize function here to override Device's and override it (without
> saving the parent's dummy function) in IOAPIC or wherever you need it.
>
I'll try it, and if changes isn't big, I'll merge into this patch.

> > +    k->bus_type = TYPE_ICC_BUS;
> > +}
> > +
> > +static const TypeInfo icc_device_info = {
> > +    .name = TYPE_ICC_DEVICE,
> > +    .parent = TYPE_DEVICE,
> > +    .abstract = true,
> > +    .instance_size = sizeof(ICCDevice),
> > +    .class_size = sizeof(ICCDeviceClass),
> > +    .class_init = icc_device_class_init,
> > +};
> > +
> > +typedef struct ICCBridgeState {
> > +    SysBusDevice busdev;
> 
> parent_obj please - makes it clear that it is a QOM struct and flags
> accidental uses of obsolete SysBus macros.
done

> 
> > +} ICCBridgeState;
> > +#define ICC_BRIGDE(obj) OBJECT_CHECK(ICCBridgeState, (obj),
> > TYPE_ICC_BRIDGE) +
> > +
> > +static void icc_bridge_initfn(Object *obj)
> > +{
> > +    qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus");
> 
> qbus_create_inplace()?
done
 
[...]

> 
> BTW Anthony recently suggested to drop the "fn" where there's no init
> vs. initfn name conflicts; I don't mind either way.
changed to init
 
[...]
> 
> /*< private >*/
> 
> > +    BusState qbus;
> 
> parent_obj
done

> 
> /*< public >*/
> 
> ...since it is in a header.
> 
> > +} ICCBus;
> > +#define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
> > +
> > +typedef struct ICCDevice {
> > +    DeviceState qdev;
> 
> dito
s/qdev/parent_obj/ will affect APIC and IOAPIC patches make-ing them even
bigger and more difficult to review distracting from real changes.

it would be better to send follow-up patch, that does trivial QOM conversions
afterwards.

I'm not sure that making ICCDevice.qdev private would be good performance wise
since it would require using DEVICE() dynamic cast in apic_check_pic().

I'd like to keep this as is for now and put cleanup patch on TODO list for
1.6 to get rid of DO_UPCAST() macro in apic/ioapic files.

> 
> Who becomes the MAINTAINER of ICC?
Perhaps me??

> 
> Regards,
> Andreas
>