* [Qemu-devel] [RFC] QEMU Object Model status/merge plan @ 2011-12-12 19:36 Anthony Liguori 2011-12-13 11:35 ` Stefan Hajnoczi 0 siblings, 1 reply; 7+ messages in thread From: Anthony Liguori @ 2011-12-12 19:36 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Markus Armbruster, Avi Kivity, Paolo Bonzini, Gerd Hoffmann Hi, I've spent the past week aggressively converting the tree to QEMU Object Model and was successful in doing a mostly complete conversion. There are a couple of outstanding issues regarding devices that are doing Bad Things but they aren't that hard to fix up. This is my development head: https://github.com/aliguori/qemu/tree/qom-next In this tree, you will see: - Full conversion of qdev devices to QOM - Relaxed bus constraints so that you can now have devices without busses - Refactored serial to take advantage of QOM I choose the serial device to showcase what we'll eventually be able to do. The three relevant files are: https://github.com/aliguori/qemu/blob/qom-next/hw/isa-serial.c https://github.com/aliguori/qemu/blob/qom-next/hw/mm-serial.c https://github.com/aliguori/qemu/blob/qom-next/hw/serial.c I chose to model mm-serial via inheritance although I think it should be through composition. I mainly did this to illustrate the possibility. The diffstat is epic as a lot of automated code conversion was used. I've included the python scripts in the tree for historic purposes. I've included the diffstat to give people an idea what to expect. In terms of merging, I've setup another tree that I've been rebasing in: https://github.com/aliguori/qemu/tree/qom-rebase.5 I feel pretty good about the structure of this tree. It basically has four parts that I plan to turn into four series: 1) Basic infrastructure/properties. No automated conversions 2) Modeling of qdev-base classes in QOM, eliminate sub classes of DeviceInfo. The later is a scripted conversion. 3) Various cleanups to make scripting possible, then scripted removal of per-bus type registration functions 4) Scripted conversion of DeviceInfo to TypeInfo followed changes to qdev to make all objects creatable by QOM directly. There's more details, but for the most part, QOM conversion is done at the end of this. Once we get this all merged, we can then start the big cleanups like I did above with the serial device. After series (2), there's enough QOM infrastructure to start using it in other places in the tree. I sent out a v3 of (1) and hope to merge that soon. I would like to get all four series merged ASAP as they are challenging to rebase because the sheer amount of change. Obviously, I'm looking for feedback about the best way to make this digestable. I think it's impossible to realistically review the automated changes so my current is to keep all automated changes in each series as one patch so that people can review a representative set of changes. Regards, Anthony Liguori b/Makefile.objs | 10 b/Makefile.target | 3 b/QMP/qmp.py | 6 b/QMP/qom-get | 26 + b/QMP/qom-list | 30 + b/QMP/qom-set | 21 b/VERSION | 2 b/hw/9pfs/virtio-9p-device.c | 46 + b/hw/a9mpcore.c | 29 - b/hw/ac97.c | 43 + b/hw/acpi_piix4.c | 70 +- b/hw/ads7846.c | 20 b/hw/alpha_dp264.c | 7 b/hw/alpha_typhoon.c | 21 b/hw/apb_pci.c | 82 ++- b/hw/apic.c | 37 - b/hw/applesmc.c | 34 - b/hw/arm11mpcore.c | 58 +- b/hw/arm_sysctl.c | 35 - b/hw/arm_timer.c | 32 + b/hw/armv7m.c | 29 - b/hw/armv7m_nvic.c | 16 b/hw/bitbang_i2c.c | 21 b/hw/bonito.c | 55 +- b/hw/ccid-card-emulated.c | 48 + b/hw/ccid-card-passthru.c | 41 + b/hw/ccid.h | 27 - b/hw/cirrus_vga.c | 45 + b/hw/container.c | 29 + b/hw/cs4231.c | 31 - b/hw/cs4231a.c | 36 - b/hw/debugcon.c | 32 - b/hw/dec_pci.c | 82 ++- b/hw/ds1225y.c | 33 - b/hw/ds1338.c | 34 - b/hw/e1000.c | 49 + b/hw/eccmemctl.c | 33 - b/hw/eepro100.c | 6 b/hw/empty_slot.c | 18 b/hw/es1370.c | 40 - b/hw/escc.c | 51 +- b/hw/esp.c | 31 - b/hw/etraxfs_eth.c | 37 - b/hw/etraxfs_pic.c | 29 - b/hw/etraxfs_ser.c | 21 b/hw/etraxfs_timer.c | 17 b/hw/fdc.c | 110 ++-- b/hw/fw_cfg.c | 37 - b/hw/g364fb.c | 37 - b/hw/grackle_pci.c | 45 + b/hw/grlib_apbuart.c | 29 - b/hw/grlib_gptimer.c | 35 - b/hw/grlib_irqmp.c | 33 - b/hw/gt64xxx.c | 43 + b/hw/gus.c | 38 - b/hw/hda-audio.c | 64 +- b/hw/hpet.c | 37 - b/hw/hw.h | 4 b/hw/i2c.c | 131 +++-- b/hw/i2c.h | 47 + b/hw/i440fx.c | 395 +++++++++++++++ b/hw/i8254.c | 36 - b/hw/i8259.c | 40 - b/hw/ide/cmd646.c | 43 - b/hw/ide/ich.c | 40 - b/hw/ide/internal.h | 20 b/hw/ide/isa.c | 36 - b/hw/ide/pci.c | 11 b/hw/ide/piix.c | 101 ++-- b/hw/ide/qdev.c | 153 ++++-- b/hw/ide/via.c | 31 - b/hw/integratorcp.c | 45 + b/hw/intel-hda.c | 124 +++- b/hw/intel-hda.h | 25 b/hw/ioapic.c | 25 b/hw/ioh3420.c | 63 +- b/hw/isa-bus.c | 63 +- b/hw/isa-serial.c | 157 ++++++ b/hw/isa-serial.h | 82 +++ b/hw/isa.h | 21 b/hw/ivshmem.c | 51 +- b/hw/kvmclock.c | 28 - b/hw/kvmclock.h | 5 b/hw/lan9118.c | 33 - b/hw/lance.c | 37 - b/hw/lm32_juart.c | 23 b/hw/lm32_pic.c | 23 b/hw/lm32_sys.c | 33 - b/hw/lm32_timer.c | 35 - b/hw/lm32_uart.c | 23 b/hw/lm832x.c | 37 - b/hw/lsi53c895a.c | 38 - b/hw/m48t59.c | 71 +- b/hw/macio.c | 5 b/hw/marvell_88w8618_audio.c | 39 - b/hw/max111x.c | 40 + b/hw/max7310.c | 39 - b/hw/mc146818rtc.c | 59 +- b/hw/milkymist-ac97.c | 23 b/hw/milkymist-hpdmc.c | 23 b/hw/milkymist-memcard.c | 23 b/hw/milkymist-minimac2.c | 41 + b/hw/milkymist-pfpu.c | 23 b/hw/milkymist-softusb.c | 47 - b/hw/milkymist-sysctl.c | 47 + b/hw/milkymist-tmu2.c | 23 b/hw/milkymist-uart.c | 23 b/hw/milkymist-vgafb.c | 35 - b/hw/mips_fulong2e.c | 5 b/hw/mips_jazz.c | 1 b/hw/mips_malta.c | 13 b/hw/mips_mipssim.c | 11 b/hw/mips_r4k.c | 6 b/hw/mipsnet.c | 37 - b/hw/mm-serial.c | 129 +++++ b/hw/mm-serial.h | 60 ++ b/hw/mpc8544_guts.c | 18 b/hw/mst_fpga.c | 23 b/hw/musicpal.c | 183 +++++-- b/hw/nand.c | 37 - b/hw/ne2000-isa.c | 34 - b/hw/ne2000.c | 41 + b/hw/object.c | 515 ++++++++++++++++++++ b/hw/object.h | 499 +++++++++++++++++++ b/hw/omap_gpio.c | 78 +-- b/hw/omap_intc.c | 70 +- b/hw/omap_uart.c | 3 b/hw/onenand.c | 39 - b/hw/opencores_eth.c | 35 - b/hw/openpic.c | 5 b/hw/parallel.c | 34 - b/hw/pc.c | 101 ++-- b/hw/pc.h | 70 -- b/hw/pc_piix.c | 64 +- b/hw/pci.c | 186 +++---- b/hw/pci.h | 80 +-- b/hw/pci_bridge.c | 2 b/hw/pcie.c | 2 b/hw/pckbd.c | 22 b/hw/pcnet-pci.c | 43 + b/hw/pcnet.c | 2 b/hw/petalogix_ml605_mmu.c | 1 b/hw/piix3.c | 198 +++++++ b/hw/piix3.h | 37 + b/hw/piix4.c | 37 - b/hw/pl011.c | 38 + b/hw/pl022.c | 16 b/hw/pl031.c | 23 b/hw/pl041.c | 38 - b/hw/pl050.c | 42 + b/hw/pl061.c | 42 + b/hw/pl080.c | 46 + b/hw/pl110.c | 69 +- b/hw/pl181.c | 16 b/hw/pl190.c | 25 b/hw/ppc405_uc.c | 1 b/hw/ppc440.c | 1 b/hw/ppc4xx_pci.c | 5 b/hw/ppc_prep.c | 9 b/hw/ppce500_mpc8544ds.c | 1 b/hw/ppce500_pci.c | 46 + b/hw/ppce500_spin.c | 18 b/hw/prep_pci.c | 5 b/hw/pxa2xx.c | 109 +++- b/hw/pxa2xx_dma.c | 33 - b/hw/pxa2xx_gpio.c | 33 - b/hw/pxa2xx_pic.c | 23 b/hw/pxa2xx_timer.c | 74 +- b/hw/qdev-monitor.c | 495 +++++++++++++++++++ b/hw/qdev-properties.c | 50 - b/hw/qdev.c | 1085 +++++++++++++++++++++++-------------------- b/hw/qdev.h | 370 +++++++++++++- b/hw/qxl.c | 70 +- b/hw/realview.c | 18 b/hw/realview_gic.c | 17 b/hw/rtl8139.c | 47 + b/hw/s390-virtio-bus.c | 25 b/hw/sb16.c | 40 - b/hw/sbi.c | 23 b/hw/scsi-bus.c | 125 +++- b/hw/scsi-disk.c | 177 ++++--- b/hw/scsi-generic.c | 41 + b/hw/scsi.h | 30 - b/hw/serial.c | 366 ++------------ b/hw/serial.h | 172 ++++++ b/hw/sga.c | 20 b/hw/sh_pci.c | 41 + b/hw/slavio_intctl.c | 23 b/hw/slavio_misc.c | 41 + b/hw/slavio_timer.c | 33 - b/hw/sm501.c | 1 b/hw/smbus.c | 85 ++- b/hw/smbus.h | 39 + b/hw/smbus_eeprom.c | 39 - b/hw/smc91c111.c | 35 - b/hw/spapr_llan.c | 45 + b/hw/spapr_pci.c | 37 + b/hw/spapr_vio.c | 100 ++- b/hw/spapr_vio.h | 39 - b/hw/spapr_vscsi.c | 39 - b/hw/spapr_vty.c | 41 + b/hw/sparc32_dma.c | 35 - b/hw/spitz.c | 110 ++-- b/hw/ssd0303.c | 37 - b/hw/ssd0323.c | 20 b/hw/ssi-sd.c | 20 b/hw/ssi.c | 39 + b/hw/ssi.h | 18 b/hw/stellaris.c | 71 ++ b/hw/stellaris_enet.c | 31 - b/hw/strongarm.c | 148 ++++- b/hw/sun4c_intctl.c | 23 b/hw/sun4m.c | 92 ++- b/hw/sun4m_iommu.c | 33 - b/hw/sun4u.c | 88 ++- b/hw/syborg_fb.c | 31 - b/hw/syborg_interrupt.c | 29 - b/hw/syborg_keyboard.c | 29 - b/hw/syborg_pointer.c | 31 - b/hw/syborg_rtc.c | 16 b/hw/syborg_serial.c | 31 - b/hw/syborg_timer.c | 29 - b/hw/syborg_virtio.c | 41 + b/hw/sysbus.c | 50 + b/hw/sysbus.h | 23 b/hw/tcx.c | 41 + b/hw/tmp105.c | 41 - b/hw/tosa.c | 54 +- b/hw/tusb6010.c | 21 b/hw/twl92230.c | 39 - b/hw/unin_pci.c | 180 +++++-- b/hw/usb-bt.c | 35 - b/hw/usb-bus.c | 203 ++++++-- b/hw/usb-ccid.c | 151 ++++- b/hw/usb-desc.c | 18 b/hw/usb-ehci.c | 67 +- b/hw/usb-hid.c | 116 ++-- b/hw/usb-hub.c | 37 - b/hw/usb-msd.c | 56 +- b/hw/usb-net.c | 52 +- b/hw/usb-ohci.c | 76 +-- b/hw/usb-serial.c | 96 ++- b/hw/usb-uhci.c | 205 +++++--- b/hw/usb-uhci.h | 2 b/hw/usb-wacom.c | 39 - b/hw/usb.c | 24 b/hw/usb.h | 97 ++- b/hw/versatile_pci.c | 58 +- b/hw/versatilepb.c | 23 b/hw/vga-isa.c | 22 b/hw/vga-pci.c | 36 - b/hw/virtex_ml507.c | 1 b/hw/virtio-console.c | 77 +-- b/hw/virtio-net.c | 2 b/hw/virtio-pci.c | 222 +++++--- b/hw/virtio-serial-bus.c | 98 ++- b/hw/virtio-serial.h | 84 +-- b/hw/vmmouse.c | 34 - b/hw/vmport.c | 20 b/hw/vmware_vga.c | 38 - b/hw/vmware_vga.h | 6 b/hw/vt82c686.c | 136 +++-- b/hw/wdt_i6300esb.c | 37 - b/hw/wdt_ib700.c | 22 b/hw/wm8750.c | 41 - b/hw/xen_platform.c | 38 - b/hw/xilinx_axidma.c | 31 - b/hw/xilinx_axienet.c | 39 - b/hw/xilinx_ethlite.c | 35 - b/hw/xilinx_intc.c | 29 - b/hw/xilinx_timer.c | 31 - b/hw/xilinx_uartlite.c | 17 b/hw/xio3130_downstream.c | 63 +- b/hw/xio3130_upstream.c | 57 +- b/hw/z2.c | 60 +- b/hw/zaurus.c | 31 - b/monitor.h | 4 b/qapi-schema.json | 107 ++++ b/qemu-common.h | 1 b/qerror.c | 4 b/qerror.h | 3 b/qmp-commands.hx | 18 b/qmp.c | 93 +++ b/scripts/patch-monkey.py | 130 +++++ b/scripts/pm2.py | 176 ++++++ b/scripts/qapi-commands.py | 1 b/scripts/qapi-types.py | 1 b/usb-bsd.c | 29 - b/usb-linux.c | 58 +- b/usb-redir.c | 33 - hw/piix_pci.c | 568 ---------------------- 291 files changed, 11875 insertions(+), 5338 deletions(-) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] QEMU Object Model status/merge plan 2011-12-12 19:36 [Qemu-devel] [RFC] QEMU Object Model status/merge plan Anthony Liguori @ 2011-12-13 11:35 ` Stefan Hajnoczi 2011-12-13 13:43 ` Anthony Liguori 0 siblings, 1 reply; 7+ messages in thread From: Stefan Hajnoczi @ 2011-12-13 11:35 UTC (permalink / raw) To: Anthony Liguori Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, qemu-devel, Markus Armbruster, Avi Kivity, Paolo Bonzini, Gerd Hoffmann On Mon, Dec 12, 2011 at 7:36 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > I choose the serial device to showcase what we'll eventually be able to do. > The three relevant files are: > > https://github.com/aliguori/qemu/blob/qom-next/hw/isa-serial.c > > https://github.com/aliguori/qemu/blob/qom-next/hw/mm-serial.c > > https://github.com/aliguori/qemu/blob/qom-next/hw/serial.c I'm not sure I understand how init functions are called for derived classes. On one hand mm-serial.c calls its superclass init function, on the other hand isa-bus.c:isa_qdev_init() calls an init function that its child class must provide. One is calling its parent, the other is calling its child. Is there a consistent way of doing this and what did I miss :)? Stefan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] QEMU Object Model status/merge plan 2011-12-13 11:35 ` Stefan Hajnoczi @ 2011-12-13 13:43 ` Anthony Liguori 2011-12-13 15:05 ` Kevin Wolf 0 siblings, 1 reply; 7+ messages in thread From: Anthony Liguori @ 2011-12-13 13:43 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Kevin Wolf, Peter Maydell, Stefan Hajnoczi, Markus Armbruster, qemu-devel, Avi Kivity, Paolo Bonzini, Gerd Hoffmann On 12/13/2011 05:35 AM, Stefan Hajnoczi wrote: > On Mon, Dec 12, 2011 at 7:36 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >> I choose the serial device to showcase what we'll eventually be able to do. >> The three relevant files are: >> >> https://github.com/aliguori/qemu/blob/qom-next/hw/isa-serial.c >> >> https://github.com/aliguori/qemu/blob/qom-next/hw/mm-serial.c >> >> https://github.com/aliguori/qemu/blob/qom-next/hw/serial.c > > I'm not sure I understand how init functions are called for derived > classes. There are three types of init functions: class_init ========== This lives in (TypeInit) and is called when a class is first created for a type. It is only ever called once. Within this function, you should override any methods in your base classes and set default implementations for any methods you implement. instance_init ============= This is the constructor for a type. It is called when an object is created and chained such that the base class constructors are called first to initialize the object. DeviceState::init ================= This is the qdev initialize function. It is called sometime after properties are set and before the guest starts running for the first time. Long term, I plan to change this to "DeviceState::realize" and remove explicit calls to qdev_init() in favor of a propagated realize signal. But for now, I'm trying to avoid churn in the tree. > On one hand mm-serial.c calls its superclass init function, > on the other hand isa-bus.c:isa_qdev_init() calls an init function > that its child class must provide. One is calling its parent, the > other is calling its child. Is there a consistent way of doing this > and what did I miss :)? Yes, this is all DeviceState::init. This is what is called when you invoke qdev_init(). Right now, you have to do it explicitly and in the case of composition, since the device is creating another device, it must be the one that calls qdev_init() for that device. In the case of inheritance, we're just calling the superclass's init function because DeviceState::init is just a normal method so we have to explicitly chain it if we want that behavior. Regards, Anthony Liguori > > Stefan > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] QEMU Object Model status/merge plan 2011-12-13 13:43 ` Anthony Liguori @ 2011-12-13 15:05 ` Kevin Wolf 2011-12-13 16:02 ` Anthony Liguori 0 siblings, 1 reply; 7+ messages in thread From: Kevin Wolf @ 2011-12-13 15:05 UTC (permalink / raw) To: Anthony Liguori Cc: Peter Maydell, Stefan Hajnoczi, Stefan Hajnoczi, qemu-devel, Markus Armbruster, Avi Kivity, Paolo Bonzini, Gerd Hoffmann Am 13.12.2011 14:43, schrieb Anthony Liguori: > On 12/13/2011 05:35 AM, Stefan Hajnoczi wrote: >> On Mon, Dec 12, 2011 at 7:36 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >>> I choose the serial device to showcase what we'll eventually be able to do. >>> The three relevant files are: >>> >>> https://github.com/aliguori/qemu/blob/qom-next/hw/isa-serial.c >>> >>> https://github.com/aliguori/qemu/blob/qom-next/hw/mm-serial.c >>> >>> https://github.com/aliguori/qemu/blob/qom-next/hw/serial.c >> >> I'm not sure I understand how init functions are called for derived >> classes. > > There are three types of init functions: > > class_init > ========== > > This lives in (TypeInit) and is called when a class is first created for a type. > It is only ever called once. Within this function, you should override any > methods in your base classes and set default implementations for any methods you > implement. I guess in most cases this could be replaced by a static table and the function could be made optional? (That is, there could be a default implementation for the NULL case) > instance_init > ============= > > This is the constructor for a type. It is called when an object is created and > chained such that the base class constructors are called first to initialize the > object. Same for this one, in your serial code it looks like this doesn't do anything interesting in the common case and could be made optional (it adds an UART child device, but this is static property and should be moved anyway) I think even in the future the really interesting work will be done in realize. Kevin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] QEMU Object Model status/merge plan 2011-12-13 15:05 ` Kevin Wolf @ 2011-12-13 16:02 ` Anthony Liguori 2011-12-14 10:33 ` Kevin Wolf 0 siblings, 1 reply; 7+ messages in thread From: Anthony Liguori @ 2011-12-13 16:02 UTC (permalink / raw) To: Kevin Wolf Cc: Peter Maydell, Stefan Hajnoczi, Stefan Hajnoczi, qemu-devel, Markus Armbruster, Avi Kivity, Paolo Bonzini, Gerd Hoffmann On 12/13/2011 09:05 AM, Kevin Wolf wrote: > Am 13.12.2011 14:43, schrieb Anthony Liguori: >> On 12/13/2011 05:35 AM, Stefan Hajnoczi wrote: >>> On Mon, Dec 12, 2011 at 7:36 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >>>> I choose the serial device to showcase what we'll eventually be able to do. >>>> The three relevant files are: >>>> >>>> https://github.com/aliguori/qemu/blob/qom-next/hw/isa-serial.c >>>> >>>> https://github.com/aliguori/qemu/blob/qom-next/hw/mm-serial.c >>>> >>>> https://github.com/aliguori/qemu/blob/qom-next/hw/serial.c >>> >>> I'm not sure I understand how init functions are called for derived >>> classes. >> >> There are three types of init functions: >> >> class_init >> ========== >> >> This lives in (TypeInit) and is called when a class is first created for a type. >> It is only ever called once. Within this function, you should override any >> methods in your base classes and set default implementations for any methods you >> implement. > > I guess in most cases this could be replaced by a static table and the > function could be made optional? (That is, there could be a default > implementation for the NULL case) As it turns out, you can pass an opaque to class_init so you could have something like: typedef struct PCIDeviceOps { void (*foo)(PCIDevice *dev, ...); ... }; And then you could write a generic: void pci_generic_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); PCIDeviceOps *ops = data; if (ops->foo) { k->foo = ops->foo; } } Which would then let you do: static PCIDeviceOps e1000_device_ops = { .foo = e1000_foo, ... }; static TypeInfo e1000_device_info = { .name = TYPE_E1000, .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(E1000State), .class_init = pci_generic_class_init, .class_data = &e1000_device_ops, }; I didn't really plan on this, but it looks like it would work pretty well. It might be reasonable to do for really common devices. I don't think it's all that nice to do for everything though because the *_generic_class_init() functions get really ugly and makes allowing subclassing pretty hard. > >> instance_init >> ============= >> >> This is the constructor for a type. It is called when an object is created and >> chained such that the base class constructors are called first to initialize the >> object. > > Same for this one, in your serial code it looks like this doesn't do > anything interesting in the common case and could be made optional (it > adds an UART child device, but this is static property and should be > moved anyway) It could potentially, yes. instance_init functions will often times not be needed. It's really meant to do things like initialize lists and stuff like that that property accessors may need to work with. > I think even in the future the really interesting work will be done in > realize. Yes. The various TypeInfo methods can all be omitted if they don't do any work. Regards, Anthony Liguori > > Kevin > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] QEMU Object Model status/merge plan 2011-12-13 16:02 ` Anthony Liguori @ 2011-12-14 10:33 ` Kevin Wolf 2011-12-14 13:46 ` Anthony Liguori 0 siblings, 1 reply; 7+ messages in thread From: Kevin Wolf @ 2011-12-14 10:33 UTC (permalink / raw) To: Anthony Liguori Cc: Peter Maydell, Stefan Hajnoczi, Stefan Hajnoczi, qemu-devel, Markus Armbruster, Avi Kivity, Paolo Bonzini, Gerd Hoffmann Am 13.12.2011 17:02, schrieb Anthony Liguori: > On 12/13/2011 09:05 AM, Kevin Wolf wrote: >> Am 13.12.2011 14:43, schrieb Anthony Liguori: >>> On 12/13/2011 05:35 AM, Stefan Hajnoczi wrote: >>>> On Mon, Dec 12, 2011 at 7:36 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >>>>> I choose the serial device to showcase what we'll eventually be able to do. >>>>> The three relevant files are: >>>>> >>>>> https://github.com/aliguori/qemu/blob/qom-next/hw/isa-serial.c >>>>> >>>>> https://github.com/aliguori/qemu/blob/qom-next/hw/mm-serial.c >>>>> >>>>> https://github.com/aliguori/qemu/blob/qom-next/hw/serial.c >>>> >>>> I'm not sure I understand how init functions are called for derived >>>> classes. >>> >>> There are three types of init functions: >>> >>> class_init >>> ========== >>> >>> This lives in (TypeInit) and is called when a class is first created for a type. >>> It is only ever called once. Within this function, you should override any >>> methods in your base classes and set default implementations for any methods you >>> implement. >> >> I guess in most cases this could be replaced by a static table and the >> function could be made optional? (That is, there could be a default >> implementation for the NULL case) > > As it turns out, you can pass an opaque to class_init so you could have > something like: > > typedef struct PCIDeviceOps { > void (*foo)(PCIDevice *dev, ...); > ... > }; > > And then you could write a generic: > > void pci_generic_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > PCIDeviceOps *ops = data; > > if (ops->foo) { > k->foo = ops->foo; > } > } > > Which would then let you do: > > static PCIDeviceOps e1000_device_ops = { > .foo = e1000_foo, > ... > }; > > static TypeInfo e1000_device_info = { > .name = TYPE_E1000, > .parent = TYPE_PCI_DEVICE, > .instance_size = sizeof(E1000State), > .class_init = pci_generic_class_init, > .class_data = &e1000_device_ops, > }; > > I didn't really plan on this, but it looks like it would work pretty well. It > might be reasonable to do for really common devices. I don't think it's all > that nice to do for everything though because the *_generic_class_init() > functions get really ugly and makes allowing subclassing pretty hard. It looks much nicer. What kind of ugliness do you see in *_generic_class_init? Just that it's a long chain of if (ops->field != NULL) k->field = ops->field; statements? This looks like something that could easily be generated in the long term. Also, what's the problem with subclassing? Doesn't this work: static TypeInfo not_quite_e1000_device_info = { .name = TYPE_NOT_QUITE_E1000, .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(NotQuiteE1000State), .class_init = e1000_generic_class_init, .class_data = ¬_quite_e1000_device_ops, }; static PCIDeviceOps not_quite_e1000_device_ops = { .bar = not_quite_e1000_bar, .parent_ops = { .foo = not_quite_e1000_foo, }, }; void e1000_generic_class_init(ObjectClass *klass, void *data) { E1000DeviceClass *k = E1000_DEVICE_CLASS(klass); E1000DeviceOps *ops = data; if (ops->bar) { k->bar = ops->bar; } pci_generic_class_init(klass, &ops->parent_ops); } (Btw, what's the point with klass vs. class? Do you expect that tools which can deal with C++ will fail if we spell it class?) >>> instance_init >>> ============= >>> >>> This is the constructor for a type. It is called when an object is created and >>> chained such that the base class constructors are called first to initialize the >>> object. >> >> Same for this one, in your serial code it looks like this doesn't do >> anything interesting in the common case and could be made optional (it >> adds an UART child device, but this is static property and should be >> moved anyway) > > It could potentially, yes. instance_init functions will often times not be > needed. It's really meant to do things like initialize lists and stuff like > that that property accessors may need to work with. Right, this is what I thought. >> I think even in the future the really interesting work will be done in >> realize. > > Yes. The various TypeInfo methods can all be omitted if they don't do any work. My concern is probably that currently they actually have to do some work. Not much, just two or three lines of code, but it's enough to make it impossible to omit them. I think that QOM allows to do a lot of complicated things (at least you have invested a lot of thought there, and even though I haven't seen the patches yet, I'll assume that you got most of that right). The part that I still see missing is that we should not only allow complicated things, but also make the common thing simple. Kevin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC] QEMU Object Model status/merge plan 2011-12-14 10:33 ` Kevin Wolf @ 2011-12-14 13:46 ` Anthony Liguori 0 siblings, 0 replies; 7+ messages in thread From: Anthony Liguori @ 2011-12-14 13:46 UTC (permalink / raw) To: Kevin Wolf Cc: Peter Maydell, Stefan Hajnoczi, Stefan Hajnoczi, qemu-devel, Markus Armbruster, Avi Kivity, Paolo Bonzini, Gerd Hoffmann On 12/14/2011 04:33 AM, Kevin Wolf wrote: > Am 13.12.2011 17:02, schrieb Anthony Liguori: >> static TypeInfo e1000_device_info = { >> .name = TYPE_E1000, >> .parent = TYPE_PCI_DEVICE, >> .instance_size = sizeof(E1000State), >> .class_init = pci_generic_class_init, >> .class_data =&e1000_device_ops, >> }; >> >> I didn't really plan on this, but it looks like it would work pretty well. It >> might be reasonable to do for really common devices. I don't think it's all >> that nice to do for everything though because the *_generic_class_init() >> functions get really ugly and makes allowing subclassing pretty hard. > > It looks much nicer. > > What kind of ugliness do you see in *_generic_class_init? Just that it's > a long chain of if (ops->field != NULL) k->field = ops->field; > statements? Yeah. The other thing that gets a bit weird about it is a common trick in order to still be able to call the superclass version of the function is to squirrel away the super class pointer in a class member. Something like: static void e1000_handle_event(PCIDevice *dev) { E1000State *s = E1000_DEVICE(dev); E1000Class *k = E1000_DEVICE_GET_CLASS(s); // Call base class version of the function k->super_handle_event(dev); // Implement subclass behavior for function } static void e1000_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *pk = PCI_DEVICE_CLASS(klass); E1000Class *ek = E1000_CLASS(klass); ek->super_handle_event = pk->handle_event; pk->handle_event = e1000_handle_event; } There is an object_get_super() that returns the superclass that would let you do the same thing but the above is the idiomatic form in GObject at least. > This looks like something that could easily be generated in > the long term. > > Also, what's the problem with subclassing? Doesn't this work: > > static TypeInfo not_quite_e1000_device_info = { > .name = TYPE_NOT_QUITE_E1000, > .parent = TYPE_PCI_DEVICE, > .instance_size = sizeof(NotQuiteE1000State), > .class_init = e1000_generic_class_init, > .class_data =¬_quite_e1000_device_ops, > }; > > static PCIDeviceOps not_quite_e1000_device_ops = { > .bar = not_quite_e1000_bar, > .parent_ops = { > .foo = not_quite_e1000_foo, > }, > }; > > void e1000_generic_class_init(ObjectClass *klass, void *data) > { > E1000DeviceClass *k = E1000_DEVICE_CLASS(klass); > E1000DeviceOps *ops = data; > > if (ops->bar) { > k->bar = ops->bar; > } > > pci_generic_class_init(klass,&ops->parent_ops); > } It definitely works, it just ends up adding boiler plate. One of my goals was to make it as easy as possible to specify a device with a little code as possible. I don't think there's much of a readability gain by using an ops structure. It's basically: .init = e1000_device_init, vs: k->init = e1000_device_init; The only advantage I see in using an ops structure is that it prevents you from doing anything other than overriding members. I'm not sure that that is a virtue. > (Btw, what's the point with klass vs. class? Do you expect that tools > which can deal with C++ will fail if we spell it class?) Since I was doing so much code conversion, I tried to err on the side of being conservative so that we wouldn't have to touch everything again. So I avoided any C++ keywords as much as possible. Perhaps one day we'll export a libqemu-block.so and even though it will be C, we'll want to make sure that a C++ program can include it. >>>> instance_init >>>> ============= >>>> >>>> This is the constructor for a type. It is called when an object is created and >>>> chained such that the base class constructors are called first to initialize the >>>> object. >>> >>> Same for this one, in your serial code it looks like this doesn't do >>> anything interesting in the common case and could be made optional (it >>> adds an UART child device, but this is static property and should be >>> moved anyway) >> >> It could potentially, yes. instance_init functions will often times not be >> needed. It's really meant to do things like initialize lists and stuff like >> that that property accessors may need to work with. > > Right, this is what I thought. > >>> I think even in the future the really interesting work will be done in >>> realize. >> >> Yes. The various TypeInfo methods can all be omitted if they don't do any work. > > My concern is probably that currently they actually have to do some > work. Not much, just two or three lines of code, but it's enough to make > it impossible to omit them. > > I think that QOM allows to do a lot of complicated things (at least you > have invested a lot of thought there, and even though I haven't seen the > patches yet, I'll assume that you got most of that right). The part that > I still see missing is that we should not only allow complicated things, > but also make the common thing simple. Yeah, I think it's pretty simple right now. If you want to add a method to a class, you: 1) Add it to the <DEVICE>Class structure (adding the structure if necessary) 2) Set the default value of that method in the class_init function for that type If you want to override a base class method, you: 1) Set a new value for the appropriate method in your type's class_init function Contrast this with qdev, where to add a method, you: 1) Need to make sure a <BUS>DeviceInfo structure exists 2) Potentially update the bus registration function if a default version needs to be implemented There's no assumption of infrastructure in QOM. All types are self contained. You actually can't override a base class method in qdev unless it's explicitly supported by the bus registration function (and most of them don't allow it). Maybe we can find ways to make common simple things simpler down the road, if so, I'm all for it. Regards, Anthony Liguori > > Kevin > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-12-14 13:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-12 19:36 [Qemu-devel] [RFC] QEMU Object Model status/merge plan Anthony Liguori 2011-12-13 11:35 ` Stefan Hajnoczi 2011-12-13 13:43 ` Anthony Liguori 2011-12-13 15:05 ` Kevin Wolf 2011-12-13 16:02 ` Anthony Liguori 2011-12-14 10:33 ` Kevin Wolf 2011-12-14 13:46 ` Anthony Liguori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).