[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 11/16] introduce ICC bus/device/bridge
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 11/16] introduce ICC bus/device/bridge |
Date: |
Mon, 22 Apr 2013 17:08:42 +0200 |
On Mon, 22 Apr 2013 15:22:30 +0200
Andreas Färber <address@hidden> 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 <address@hidden>
> > ---
> > 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
>
- [Qemu-devel] [PATCH 09/16] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest, (continued)
[Qemu-devel] [PATCH 13/16] target-i386: replace MSI_SPACE_SIZE with APIC_SPACE_SIZE, Igor Mammedov, 2013/04/15
[Qemu-devel] [PATCH 14/16] target-i386: move APIC to ICC bus, Igor Mammedov, 2013/04/15