From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47294) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3rot-0003Kj-Ma for qemu-devel@nongnu.org; Tue, 01 Dec 2015 15:46:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3roq-0002f7-Ut for qemu-devel@nongnu.org; Tue, 01 Dec 2015 15:46:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34939) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3roq-0002ew-O9 for qemu-devel@nongnu.org; Tue, 01 Dec 2015 15:46:36 -0500 References: <1448987005-28335-1-git-send-email-shmulik.ladkani@ravellosystems.com> <565DCC97.5020504@redhat.com> <20151201213007.2b41e810@halley> From: Marcel Apfelbaum Message-ID: <565E0729.9060603@redhat.com> Date: Tue, 1 Dec 2015 22:46:33 +0200 MIME-Version: 1.0 In-Reply-To: <20151201213007.2b41e810@halley> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shmulik Ladkani Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On 12/01/2015 09:30 PM, Shmulik Ladkani wrote: > Hi, > > On Tue, 1 Dec 2015 18:36:39 +0200 Marcel Apfelbaum 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. >