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

[Michael S. Tsirkin]

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