Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint

[Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>]

Hi Michael,

On Mon, 14 Dec 2015 19:37:29 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Dec 14, 2015 at 06:14:37PM +0100, Paolo Bonzini wrote:
> > 
> > On 13/12/2015 09:08, Shmulik Ladkani wrote:
> > > +    pvs_k->parent_dc_realize = dc->realize;
> > 
> > Marcel, Michael,
> > 
> > this creates a really nasty dependency on the contents of pci_qdev_realize.
> > 
> > Can you instead change PCIDeviceClass's pc->is_express to a function
> > pointer, and provide a sample implementation pci_is_express_true for the
> > devices that set is_express to true?
> > 
> 
> I'm not very familiar with vmw code, and I dislike overusing callbacks.
> What exactly would this callback do?

Not specific to vmw.

Recently we've made virtio-pci a pcie:
  1811e64c hw/virtio: Add PCIe capability to virtio devices

For migration, 'x-pcie-disable' proprety was introduced.
Thus, instances of virtio-pci may be either pci or pcie.

We'd like to do the same for vmxnet3 and vmw_pvscsi.

This exposes a design limitation.

PCIDeviceClass.is_express is a propery of the class.
All pci_dev instances get the QEMU_PCI_CAP_EXPRESS bit assigned into
their 'cap_present' according to their class 'pc->is_express' value,
early in 'pci_qdev_realize', quote:

static void pci_qdev_realize(DeviceState *qdev, Error **errp)
    ...
    /* initialize cap_present for pci_is_express() and pci_config_size() */
    if (pc->is_express) {
        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
    }


However, we'd like to set QEMU_PCI_CAP_EXPRESS conditionally per
instance.

In order to set QEMU_PCI_CAP_EXPRESS conditionally per instance
(for example, according to the given x-pcie-disable property), the
'parent_dc_realize' trick was suggested.

Reasoning is documented in:
  0560b0e9 virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method

Alas, the same trick needs to be repeated for all classes whose
instances may be either pci or pcie.

What Paolo suggest, is having a callback which can return whether the
device *instance* needs the QEMU_PCI_CAP_EXPRESS bit.

So in 'pci_qdev_realize', instead of:

    if (pc->is_express)
        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

we'll have something like:

    if (pc->is_express(qdev))
        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

WDYT?

Regards,
Shmulik