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

[Jason Wang <jasowang@redhat.com>]

On 12/14/2015 03:32 PM, Shmulik Ladkani wrote:
> Thanks Jason,
>
> On Mon, 14 Dec 2015 14:24:36 +0800, jasowang@redhat.com 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.

I see.

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

Yes, that's another solution (as I replied for patch 6). A question
here. If vmware differs pci-e version of vmxnet3 from pci version,
probably we need do the same (and you don't even need to care for
compatibility in the case). At a quick glance, no much duplicated codes.
(if you mean the msi offsets, you can let vmxnet3e use the new offset
unconditionally).

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