Re: [Qemu-devel] [PATCH] virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method

[Marcel Apfelbaum <marcel@redhat.com>]

On 12/01/2015 09:30 PM, Shmulik Ladkani wrote:
> 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?

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.
>