From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:54029)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <shmulik.ladkani@ravellosystems.com>)
	id 1a3qcz-0004zn-QA
	for qemu-devel@nongnu.org; Tue, 01 Dec 2015 14:30:18 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <shmulik.ladkani@ravellosystems.com>)
	id 1a3qcv-0005sb-Ma
	for qemu-devel@nongnu.org; Tue, 01 Dec 2015 14:30:17 -0500
Received: from mail-wm0-x22a.google.com ([2a00:1450:400c:c09::22a]:33966)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <shmulik.ladkani@ravellosystems.com>)
	id 1a3qcv-0005sQ-Fn
	for qemu-devel@nongnu.org; Tue, 01 Dec 2015 14:30:13 -0500
Received: by wmvv187 with SMTP id v187so222903206wmv.1
	for <qemu-devel@nongnu.org>; Tue, 01 Dec 2015 11:30:12 -0800 (PST)
Date: Tue, 1 Dec 2015 21:30:07 +0200
From: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
Message-ID: <20151201213007.2b41e810@halley>
In-Reply-To: <565DCC97.5020504@redhat.com>
References: <1448987005-28335-1-git-send-email-shmulik.ladkani@ravellosystems.com>
	<565DCC97.5020504@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: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Marcel Apfelbaum <marcel@redhat.com>
Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>

Hi,

On Tue, 1 Dec 2015 18:36:39 +0200 Marcel Apfelbaum <marcel@redhat.com> 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?

> > +    DeviceRealize saved_dc_realize;
> 
> I would change the name to parent_realize :)

Sure.