Am 25.02.2013 21:11, schrieb Dmitry Fleytman: [snip] > +static int > +vmxnet3_msix_load(QEMUFile *f, void *opaque, int version_id) > +{ > + msix_load(&((VMXNET3State *)opaque)->dev, f);
Apart from doing too much in one line, you should not access the parent field ->dev directly but use PCI_DEVICE(opaque) cast macro, which makes the code much simpler. http://wiki.qemu.org/QOMConventions Please check the above rest of the file for more occurrences. Suggest to rename dev field to parent_obj as recommended above, then you will easily see where it is (mis)used outside of VMState code. > + return 0; > +} > + > +static int vmxnet3_pci_init(PCIDevice *dev) > +{ > + static const MemoryRegionOps b0_ops = { > + .read = vmxnet3_io_bar0_read, > + .write = vmxnet3_io_bar0_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > + }; > + > + static const MemoryRegionOps b1_ops = { > + .read = vmxnet3_io_bar1_read, > + .write = vmxnet3_io_bar1_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > + }; Any reason to place these inside the function? Makes it harder to see what the function is actually doing and may need to be relocated to instance_init. > + > + VMXNET3State *s = DO_UPCAST(VMXNET3State, dev, dev); Please don't use DO_UPCAST() for QOM types, introduce your own VMXNET3(obj) cast macro and use that instead. > + > + VMW_CBPRN("Starting init..."); > + > + memory_region_init_io(&s->bar0, &b0_ops, s, > + "vmxnet3-b0", VMXNET3_PT_REG_SIZE); > + pci_register_bar(&s->dev, VMXNET3_BAR0_IDX, Please don't access the parent field ->dev directly. Here you already have a PCIDevice *dev, which you would be advised to rename to PCIDevice *d or so, see below. > + PCI_BASE_ADDRESS_SPACE_MEMORY, &s->bar0); > + > + memory_region_init_io(&s->bar1, &b1_ops, s, > + "vmxnet3-b1", VMXNET3_VD_REG_SIZE); > + pci_register_bar(&s->dev, VMXNET3_BAR1_IDX, > + PCI_BASE_ADDRESS_SPACE_MEMORY, &s->bar1); > + > + memory_region_init(&s->msix_bar, "vmxnet3-msix-bar", > + VMXNET3_MSIX_BAR_SIZE); > + pci_register_bar(&s->dev, VMXNET3_MSIX_BAR_IDX, > + PCI_BASE_ADDRESS_SPACE_MEMORY, &s->msix_bar); > + > + vmxnet3_reset_interrupt_states(s); > + > + /* Interrupt pin A */ > + s->dev.config[PCI_INTERRUPT_PIN] = 0x01; dev->config[... > + > + if (!vmxnet3_init_msix(s)) { > + VMW_WRPRN("Failed to initialize MSI-X, configuration is > inconsistent."); > + } > + > + if (!vmxnet3_init_msi(s)) { > + VMW_WRPRN("Failed to initialize MSI, configuration is > inconsistent."); > + } When might these functions fail and why do they not return non-0 then? > + > + vmxnet3_net_init(s); > + > + register_savevm(&dev->qdev, "vmxnet3-msix", -1, 1, > + vmxnet3_msix_save, vmxnet3_msix_load, s); Why is this needed and not in the main VMStateDescription as a subsection? > + > + add_boot_device_path(s->conf.bootindex, &dev->qdev, "/ethernet-phy@0"); Not dev->qdev either, instead DEVICE(dev) or better DeviceState *dev to avoid repeated casts. > + > + return 0; > +} While PCI does not fully support QOM realize, you should prepare for this by splitting up this function into int vmxnet3_pci_init(PCIDevice *d) and an .instance_init void vmxnet3_init(Object *obj) The latter should contain any trivial initializations, such as the MemoryRegions, whereas dc->realize / pdc->init would contain anything dependent on user-settable properties or having global effects. The final init replacement looks like this: void vmxnet3_realize(DeviceState *dev, Error **errp) > + > + > +static void vmxnet3_pci_uninit(PCIDevice *dev) > +{ > + VMXNET3State *s = DO_UPCAST(VMXNET3State, dev, dev); Cast macro > + > + VMW_CBPRN("Starting uninit..."); > + > + unregister_savevm(&dev->qdev, "vmxnet3-msix", s); No ->qdev > + > + vmxnet3_net_uninit(s); > + > + vmxnet3_cleanup_msix(s); > + > + vmxnet3_cleanup_msi(s); > + > + memory_region_destroy(&s->bar0); > + memory_region_destroy(&s->bar1); > + memory_region_destroy(&s->msix_bar); > +} Similarly this should be split into uninit (unrealize to become) and .instance_finalize, corresponding to the initialization sequence. Whether you do that in this patch or as a follow-up, you should keep the final code structure in mind here for your choice of variables, especially the DeviceState *dev vs. PCIDevice *dev name conflict. > + > +static void vmxnet3_qdev_reset(DeviceState *dev) > +{ > + VMXNET3State *s = DO_UPCAST(VMXNET3State, dev.qdev, dev); Cast macro Stefan, please keep an eye on such QOM issues during your review! > + VMW_CBPRN("Starting QDEV reset..."); > + vmxnet3_reset(s); > +} [...] > +static void vmxnet3_class_init(ObjectClass *class, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(class); > + PCIDeviceClass *c = PCI_DEVICE_CLASS(class); > + > + c->init = vmxnet3_pci_init; > + c->exit = vmxnet3_pci_uninit; > + c->vendor_id = PCI_VENDOR_ID_VMWARE; > + c->device_id = PCI_DEVICE_ID_VMWARE_VMXNET3; > + c->revision = PCI_DEVICE_ID_VMWARE_VMXNET3_REVISION; > + c->class_id = PCI_CLASS_NETWORK_ETHERNET; > + c->subsystem_vendor_id = PCI_VENDOR_ID_VMWARE; > + c->subsystem_id = PCI_DEVICE_ID_VMWARE_VMXNET3; > + c->config_write = vmxnet3_write_config, > + dc->desc = "VMWare Paravirtualized Ethernet v3"; > + dc->reset = vmxnet3_qdev_reset; > + dc->vmsd = &vmstate_vmxnet3; > + dc->props = vmxnet3_properties; > +} > + > +static TypeInfo vmxnet3_info = { static const - this has been advertised on the list for quite a while and all in-tree devices have been brought in line by now. > + .name = "vmxnet3", > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(VMXNET3State), > + .class_init = vmxnet3_class_init, > +}; > + > +static void vmxnet3_register_types(void) > +{ > + VMW_CBPRN("vmxnet3_register_types called..."); > + type_register_static(&vmxnet3_info); > +} > + > +type_init(vmxnet3_register_types) > diff --git a/hw/vmxnet3.h b/hw/vmxnet3.h > new file mode 100644 > index 0000000..22787b1 > --- /dev/null > +++ b/hw/vmxnet3.h > @@ -0,0 +1,760 @@ > +/* > + * QEMU VMWARE VMXNET3 paravirtual NIC > + * > + * Copyright (c) 2012 Ravello Systems LTD (http://ravellosystems.com) > + * > + * Developed by Daynix Computing LTD (http://www.daynix.com) > + * > + * Authors: > + * Dmitry Fleytman <dmi...@daynix.com> > + * Tamir Shomer <tam...@daynix.com> > + * Yan Vugenfirer <y...@daynix.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ [...] > +/* > + * Following is an interface definition for > + * VMXNET3 device as provided by VMWARE > + * See original copyright from Linux kernel v3.2.8 > + * header file drivers/net/vmxnet3/vmxnet3_defs.h below. > + */ > + > +/* > + * Linux driver for VMware's vmxnet3 ethernet NIC. > + * > + * Copyright (C) 2008-2009, VMware, Inc. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; version 2 of the License and no later version. These two licenses clearly contradict each other... Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg