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>]

Thanks Marcel,

On Tue, 1 Dec 2015 22:46:33 +0200, marcel@redhat.com wrote:
> >> 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)

I actually assumed this is the right behavior.

A device class reports whether its instances *could* be pcie by arming
its PCIDeviceClass.is_express.
As such, the "big" config space is allocated for the instance. This is
harmless.

Such a device may (or may not) be connected to a pcie bus, and only if
so, we report it is a pcie endpoint.

Also, pcie_add_capability is allowed on that device, in order to setup
whatever capabilities on its pcie config space (even if finally not on a
pcie bus).

Moreover, VMSTATE_PCIE_DEVICE (which uses vmstate_pcie_device rather
than vmstate_pci_device) can be used for that device's
VMStateDescription fields *without* worrying whether the actual config
space is "big" or "small".
Otherwise one should examine whether vmstate_pcie_device or
vmstate_pci_device need to be used. Seems tedious.

This is the reasoning I can think of, why assigning QEMU_PCI_CAP_EXPRESS
and reporting pcie_endpoint_cap_init are not tightly coupled.

Indeed, no strict solution here, both approaches seem reasoanble (and
both are used!).

WDYT? Is my above interpretation makes sense?

Regards,
Shmulik