From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34143) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8aO4-0004na-QF for qemu-devel@nongnu.org; Mon, 14 Dec 2015 16:10:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8aO1-0001py-6X for qemu-devel@nongnu.org; Mon, 14 Dec 2015 16:10:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43209) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8aO0-0001ps-V1 for qemu-devel@nongnu.org; Mon, 14 Dec 2015 16:10:25 -0500 Date: Mon, 14 Dec 2015 23:10:20 +0200 From: "Michael S. Tsirkin" Message-ID: <20151214230450-mutt-send-email-mst@redhat.com> References: <1449994112-7054-1-git-send-email-shmulik.ladkani@ravellosystems.com> <1449994112-7054-6-git-send-email-shmulik.ladkani@ravellosystems.com> <566EF8FD.3080206@redhat.com> <20151214173729.GA14061@redhat.com> <20151214230105.6890c641@halley> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151214230105.6890c641@halley> Subject: Re: [Qemu-devel] [PATCH v2 5/6] vmw_pvscsi: The pvscsi device is a PCIE endpoint List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shmulik Ladkani Cc: Jason Wang , idan.brown@ravellosystems.com, qemu-devel@nongnu.org, Marcel Apfelbaum , Dmitry Fleytman , Paolo Bonzini On Mon, Dec 14, 2015 at 11:01:05PM +0200, Shmulik Ladkani wrote: > Hi Michael, > > On Mon, 14 Dec 2015 19:37:29 +0200 "Michael S. Tsirkin" wrote: > > On Mon, Dec 14, 2015 at 06:14:37PM +0100, Paolo Bonzini wrote: > > > > > > On 13/12/2015 09:08, Shmulik Ladkani wrote: > > > > + pvs_k->parent_dc_realize = dc->realize; > > > > > > Marcel, Michael, > > > > > > this creates a really nasty dependency on the contents of pci_qdev_realize. > > > > > > Can you instead change PCIDeviceClass's pc->is_express to a function > > > pointer, and provide a sample implementation pci_is_express_true for the > > > devices that set is_express to true? > > > > > > > I'm not very familiar with vmw code, and I dislike overusing callbacks. > > What exactly would this callback do? > > Not specific to vmw. > > Recently we've made virtio-pci a pcie: > 1811e64c hw/virtio: Add PCIe capability to virtio devices > > For migration, 'x-pcie-disable' proprety was introduced. > Thus, instances of virtio-pci may be either pci or pcie. > > We'd like to do the same for vmxnet3 and vmw_pvscsi. > > This exposes a design limitation. > > PCIDeviceClass.is_express is a propery of the class. > All pci_dev instances get the QEMU_PCI_CAP_EXPRESS bit assigned into > their 'cap_present' according to their class 'pc->is_express' value, > early in 'pci_qdev_realize', quote: > > static void pci_qdev_realize(DeviceState *qdev, Error **errp) > ... > /* initialize cap_present for pci_is_express() and pci_config_size() */ > if (pc->is_express) { > pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > } > > > However, we'd like to set QEMU_PCI_CAP_EXPRESS conditionally per > instance. > > In order to set QEMU_PCI_CAP_EXPRESS conditionally per instance > (for example, according to the given x-pcie-disable property), the > 'parent_dc_realize' trick was suggested. > > Reasoning is documented in: > 0560b0e9 virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method > > Alas, the same trick needs to be repeated for all classes whose > instances may be either pci or pcie. Actually, I think you can just move the code to pci core. There won't be a need for a callback then. > What Paolo suggest, is having a callback which can return whether the > device *instance* needs the QEMU_PCI_CAP_EXPRESS bit. > > So in 'pci_qdev_realize', instead of: > > if (pc->is_express) > pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > we'll have something like: > > if (pc->is_express(qdev)) > pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > WDYT? > > Regards, > Shmulik