Andreas, thanks for your review.
The issue is we were developing this device using other devices code
(mainly e1000 and virtio-net) as a reference, and unfortunately they don't
follow current QOM rules.
Also these patches are pretty long-living (around a year already), I'm not
sure what was the proper convention that days.
Anyway, I've fixed almost everything you've pointed out. See below my
comments on specific issues.
On Tue, Feb 26, 2013 at 12:05 AM, Andreas Färber <afaer...@suse.de> wrote:
> 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.
>
> [DF] Done.
> > + 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.
[DF] Fixed.
> > +
> > + 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.
>
>
[DF] Done.
> > +
> > + 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.
[DF] Fixed everywhere.
> > + 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?
[DF] These functions may fail in case corresponding QEMU function fails.
[DF] They are bool, not int, return value follows boolean convention.
> > +
> > + 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?
[DF] The only two places in QEMU doing the same use this technique,
[DF] we wrote this by example.
> > +
> > + 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.
>
>
[DF] Currently it is not 100% clear for me how to split this function, so
I'd do this in follow-up patches when reference code of some other device
will be available.
[DF] Name conflict resolved in next submission.
> > +
> > +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.
>
>
[DF] Fixed
> > + .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
>
--
Dmitry Fleytman
Technology Expert and Consultant,
Daynix Computing Ltd.
Cell: +972-54-2819481
Skype: dmitry.fleytman
