On 12/01/2015 09:30 PM, Shmulik Ladkani wrote:
Hi,
On Tue, 1 Dec 2015 18:36:39 +0200 Marcel Apfelbaum <mar...@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?
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.
