From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53220) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3jPx-0007Pk-PZ for qemu-devel@nongnu.org; Tue, 01 Dec 2015 06:48:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3jPu-0005eH-EH for qemu-devel@nongnu.org; Tue, 01 Dec 2015 06:48:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51956) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3jPu-0005dj-9T for qemu-devel@nongnu.org; Tue, 01 Dec 2015 06:48:18 -0500 References: <1447155689-26230-1-git-send-email-marcel@redhat.com> <20151201122900.530ef562@pixies> From: Marcel Apfelbaum Message-ID: <565D88FD.7000709@redhat.com> Date: Tue, 1 Dec 2015 13:48:13 +0200 MIME-Version: 1.0 In-Reply-To: <20151201122900.530ef562@pixies> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shmulik Ladkani , mst@redhat.com Cc: qemu-devel@nongnu.org, ehabkost@redhat.com, kraxel@redhat.com On 12/01/2015 12:29 PM, Shmulik Ladkani wrote: > Hi, > > On Tue, 10 Nov 2015 13:41:29 +0200, marcel@redhat.com wrote: >> The virtio devices are converted to PCI-Express >> if they are plugged into a PCI-Express bus and >> the 'modern' protocol is enabled. >> >> @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) >> >> + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) >> + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) >> + && pci_bus_is_express(pci_dev->bus) >> + && !pci_bus_is_root(pci_dev->bus)) { >> + int pos; >> + >> + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > Setting QEMU_PCI_CAP_EXPRESS here in 'virtio_pci_realize' is too late. > > This is since 'pci_qdev_realize' (DeviceClass.realize of TYPE_PCI_DEVICE) > is invoked prior the PCIDeviceClass's specific 'realize' method > (virtio_pci_realize in this case). > > During 'pci_qdev_realize' (specifically, within do_pci_register_device), > the QEMU_PCI_CAP_EXPRESS gets tested indirectly, when pci_is_express > and pci_config_size helpers are called. > > For example: 'pci_config_alloc' uses 'pci_config_size' which relies on > QEMU_PCI_CAP_EXPRESS property. > > Since virtio_pci sets QEMU_PCI_CAP_EXPRESS *after* pci_qdev_realize > has finished, we end up having an insufficient pci config space > allocated for the virtio "pcie" device. Hi, Thanks for catching it. It is a good time for a fix since we don't have yet an actual PCIe express capability. > > May I suggest the following: > - Expose 'pci_qdev_realize' > - Have 'virtio_pci_class_init' arm it's own dc->realize, > which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed, > and then call 'pci_qdev_realize' > - Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of > directly checking the proxy->flags > > If this sounds ok, I'll submit a fix. Give it a try, sure, but you don't need to expose pci_qdev_realize, you can 'hijack' parent's realize method before replacing it. Thanks, Marcel > > Regards, > Shmulik >