qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Dmitry Fleytman
Subject: Re: [Qemu-devel] [PATCH v11 5/5] VMXNET3 device implementation
Date: Sat, 2 Mar 2013 14:42:34 +0200

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


[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 <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



--
Dmitry Fleytman
Technology Expert and Consultant,

Daynix Computing Ltd.

Cell: +972-54-2819481

Skype: dmitry.fleytman

reply via email to

[Prev in Thread] Current Thread [Next in Thread]