Re: [Qemu-devel] [PATCH v11 5/5] VMXNET3 device implementation

[Andreas Färber]

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, "/address@hidden");

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 <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.
> + *
> + */
[...]
> +/*
> + * 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