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

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

Hi,

On Tue, 1 Dec 2015 18:36:39 +0200 Marcel Apfelbaum <marcel@redhat.com> 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?

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

Sure.