Re: [Qemu-devel] [PATCH] virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method

[Marcel Apfelbaum]

On 12/01/2015 09:30 PM, Shmulik Ladkani wrote:
> Hi,
>
> On Tue, 1 Dec 2015 18:36:39 +0200 Marcel Apfelbaum <address@hidden> wrote:
> > > +    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus) &&
> > > +        !pci_bus_is_root(pci_dev->bus)) {
> > >            int pos;
> >
> > Here you should check only for 'pci_is_express(pci_dev)' .
>
> [snip]
>
> > > +static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> > > +{
> > > +    VirtioPCIClass *vpciklass = VIRTIO_PCI_GET_CLASS(qdev);
> > > +    VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> > > +    PCIDevice *pci_dev = &proxy->pci_dev;
> > > +
> > > +    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> > > +        !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) {
> > > +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> >
> > And here you should also check:
> >        pci_bus_is_express(pci_dev->bus) && !pci_bus_is_root(pci_dev->bus))
> >
> > The reason is the device becomes express only if *all* the conditions
> > are met.
>
> I'm ok with either approaches.
>
> However it seems common practice to set QEMU_PCI_CAP_EXPRESS
> unconditionally for PCIE devices.
>
> The few existing PCIE devices do so by assigning their
> PCIDeviceClass.is_express to 1 within their 'class_init', regardless the
> properties of the bus their on.
> (e.g. xhci_class_init, megasas_class_init, vfio_pci_dev_class_init,
>   nvme_class_init, and more)
>
> Some devices later call pcie_endpoint_cap_init conditionally.
> (e.g. usb_xhci_realize).
>
> Can you please examine this and let me know the preferred approach?

Yes, I saw that..., as always not a walk in the park.

- So we have "is_express = true" <=> QEMU_PCI_CAP_EXPRESS on <=> "config size = 
PCIe"
- Not related to the above (!!), if (some condition) => add PCIe express 
capability
  (megasas is the exception)

Let's take "usb_xhci":
 - If we put it under a PCI bus it will not be an express device, but
   it will have a "big" config space. Also pci_is_express(dev) will still 
return true!
 - This is probably a bug. (or I am missing something)
NVME:
 - simple, always PCIe
Now let's see vfio-pci:
 - is_express = true (with the comment: we might be) => PCIe config
 - vfio_populate_device => checks actual register (I think),
   if not PCIe, rewinds it :
        vdev->config_size = reg_info.size;
        if (vdev->config_size == PCI_CONFIG_SPACE_SIZE) {
                vdev->pdev.cap_present &= ~QEMU_PCI_CAP_EXPRESS;
        }
 - better (we still "loose" the space, but at least pci_is_express will return 
false)

Now virtio case:
 - If we split the conditions into 2 parts we would have usb_xhci issues:
   - PCIe config space for a PCI device if *some* conditions are not met.
   - pci_is_express will return true when we don't want that.
If you see a reason to split, please do, I only see problems :)

Our solution to make it "clean" is to not mark the class as "is_express",
but hijack realize method and add our "conditions" before calling it.

A more elegant solution would be to make is_express a method and let the 
subclasses
implement it:
 - vfio will look for the actual device config space
 - NVME will return true
 - usb_xhci will condition this on the bus type
 - virtio will have its own conditions.
But this is not 2.5 material.

I hope I helped,
Thanks for getting involved.
Marcel

> > > +    DeviceRealize saved_dc_realize;
> >
> > I would change the name to parent_realize :)
>
> Sure.