|
From: | Marcel Apfelbaum |
Subject: | Re: [Qemu-devel] [PATCH] virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method |
Date: | Tue, 1 Dec 2015 22:46:33 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
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.
[Prev in Thread] | Current Thread | [Next in Thread] |