From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a42Lk-0001g5-9X for qemu-devel@nongnu.org; Wed, 02 Dec 2015 03:01:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a42Lh-0001Ca-3L for qemu-devel@nongnu.org; Wed, 02 Dec 2015 03:01:16 -0500 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:38865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a42Lg-0001CT-OY for qemu-devel@nongnu.org; Wed, 02 Dec 2015 03:01:13 -0500 Received: by wmec201 with SMTP id c201so46326312wme.1 for ; Wed, 02 Dec 2015 00:01:12 -0800 (PST) Date: Wed, 2 Dec 2015 10:01:09 +0200 From: Shmulik Ladkani Message-ID: <20151202100109.72be507a@pixies> In-Reply-To: <565E0729.9060603@redhat.com> References: <1448987005-28335-1-git-send-email-shmulik.ladkani@ravellosystems.com> <565DCC97.5020504@redhat.com> <20151201213007.2b41e810@halley> <565E0729.9060603@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Marcel Apfelbaum Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" 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