On Mon, Dec 14, 2015 at 11:01:05PM +0200, Shmulik Ladkani wrote:
> Hi Michael,
>
> On Mon, 14 Dec 2015 19:37:29 +0200 "Michael S. Tsirkin" <m...@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.
Actually, I think you can just move the code to pci core.
There won't be a need for a callback then.
> 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
