Re: [Qemu-devel] [PATCH v3 5/7] vmxnet3: The vmxnet3 device is a PCIE endpoint

[Shmulik Ladkani]

Thanks Jason,

On Mon, 14 Dec 2015 14:24:36 +0800, address@hidden wrote:
> > +static void vmxnet3_realize(DeviceState *qdev, Error **errp)
> > +{
> > +    VMXNET3Class *vc = VMXNET3_DEVICE_GET_CLASS(qdev);
> > +    PCIDevice *pci_dev = PCI_DEVICE(qdev);
> > +    VMXNET3State *s = VMXNET3(qdev);
> > +
> > +    if (!(s->compat_flags & VMXNET3_COMPAT_FLAG_DISABLE_PCIE)) {
> > +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > +    }
> 
> Looking at the other pci express device implementation (e.g nvme). Looks
> like we can re-use the "is_express" property of PCIDeviceClass by
> setting this to true in vmxnet3_class_init(). If this is ok, there's
> probably no need for hacking like this.

Yes, arming PCIDeviceClass.is_express in a 'class_init' method is the
classic way instructing the pci device to get the QEMU_PCI_CAP_EXPRESS
flag.
But this only works when 'is_express' is unconditionally true for all
instances of the class.

However in our case, we need to set it conditionally per instance,
according to the 'x-disable-pcie' device property.

My first attempt was setting QEMU_PCI_CAP_EXPRESS in
'vmxnet3_instance_init', which seems much cleaner (no need to introduce
the 'parent_dc_realize' hack).

This attempt failed, since at time of 'instance_init' invocation, the
device properties are NOT yet processed, so the 'compat_flags' isn't
set with the right value:

qdev_device_add()
   dev = DEVICE(object_new(driver));  # device creation
     object_new_with_type()
       object_initialize_with_type()
         object_init_with_type()
           ti->instance_init(obj);  # instance_init invoked at device creation
   ...
   qemu_opt_foreach(opts, set_property, dev, &err)  # sets properties
   ...

An alternative to introducing 'parent_dc_realize' was direct invocation
of 'pci_qdev_realize' within 'vmxnet3_realize', as I suggested in [1].
However this was rejected by Marcel Apfelbaum, as this might be error
prone since subclass (TYPE_VMXNET3) should have no direct knowledge
about its parent class's (TYPE_PCI_DEVICE) realize method, see [2].

Another attempt I've made is to indroduce a new type vmxnet3e (the
pcie variant of vmxnet3).
I dropped this approach since it was way too cumbersome, introducing
lots of boiler-plate code for the two (otherwise) identical types.

Since virtio-pci device needed the same fix (making it pcie without
breaking compat), and since the above approach was chosen
 (0560b0e virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its 
DeviceClass realize method)
I've repeated same approach for vmxnet3.

I'm open to other suggestions, if we can come up with something cleaner
that fits all requirements.

Regards,
Shmulik

[1]
https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00043.html

[2]
https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00114.html