|
From: | Dmitry Fleytman |
Subject: | Re: [Qemu-devel] [PATCH v11 5/5] VMXNET3 device implementation |
Date: | Sat, 2 Mar 2013 14:42:34 +0200 |
Am 25.02.2013 21:11, schrieb Dmitry Fleytman:
[snip]
> +static intApart from doing too much in one line, you should not access the parent
> +vmxnet3_msix_load(QEMUFile *f, void *opaque, int version_id)
> +{
> + msix_load(&((VMXNET3State *)opaque)->dev, f);
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;Any reason to place these inside the function?
> +}
> +
> +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,
> + },
> + };
Makes it harder to see what the function is actually doing and may need
to be relocated to instance_init.
> +Please don't use DO_UPCAST() for QOM types, introduce your own
> + VMXNET3State *s = DO_UPCAST(VMXNET3State, dev, dev);
VMXNET3(obj) cast macro and use that instead.
> +Please don't access the parent field ->dev directly. Here you already
> + 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,
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);dev->config[...
> +
> + 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;
When might these functions fail and why do they not return non-0 then?
> +
> + 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.");
> + }
> +Why is this needed and not in the main VMStateDescription as a subsection?
> + vmxnet3_net_init(s);
> +
> + register_savevm(&dev->qdev, "vmxnet3-msix", -1, 1,
> + vmxnet3_msix_save, vmxnet3_msix_load, s);
> +Not dev->qdev either, instead DEVICE(dev) or better DeviceState *dev to
> + add_boot_device_path(s->conf.bootindex, &dev->qdev, "/address@hidden");
avoid repeated casts.
While PCI does not fully support QOM realize, you should prepare for
> +
> + return 0;
> +}
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)
Cast macro
> +
> +
> +static void vmxnet3_pci_uninit(PCIDevice *dev)
> +{
> + VMXNET3State *s = DO_UPCAST(VMXNET3State, dev, dev);
No ->qdev
> +
> + VMW_CBPRN("Starting uninit...");
> +
> + unregister_savevm(&dev->qdev, "vmxnet3-msix", s);
Similarly this should be split into uninit (unrealize to become) and
> +
> + 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);
> +}
.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.
> +Cast macro
> +static void vmxnet3_qdev_reset(DeviceState *dev)
> +{
> + VMXNET3State *s = DO_UPCAST(VMXNET3State, dev.qdev, dev);
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)static const - this has been advertised on the list for quite a while
> +{
> + 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 = {
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 <address@hidden>
> + * Tamir Shomer <address@hidden>
> + * Yan Vugenfirer <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +/*These two licenses clearly contradict each other...
> + * 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.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Daynix Computing Ltd.
Cell: +972-54-2819481
Skype: dmitry.fleytman[Prev in Thread] | Current Thread | [Next in Thread] |