Re: [Qemu-devel] [PATCH V3] pci: removed the is_express field since a uniform interface was inserted

[Yoni Bettan <ybettan@redhat.com>]

On 12/11/2017 04:19 PM, Eduardo Habkost wrote:
> On Mon, Dec 11, 2017 at 03:11:39PM +0200, Yoni Bettan wrote:
>>
>> On 12/07/2017 10:58 PM, Eduardo Habkost wrote:
>>> On Tue, Dec 05, 2017 at 07:17:06PM +0200, Yoni Bettan wrote:
>>>>           * according to Eduardo Habkost's commit
>>>>             fd3b02c8896d597dd8b9e053dec579cf0386aee1
>>>>
>>>>           * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
>>>>             don't need this field anymore
>>>>
>>>>           * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
>>>>             or
>>>>             devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE (is_express == 0)
>>>>             where not affected by the change
>>>>
>>>>             The only devices that were affected are those that are hybrid and also
>>>>             had (is_express == 1) - therefor only:
>>>>               - hw/vfio/pci.c
>>>>               - hw/usb/hcd-xhci.c
>>>>
>>>>             For both I made sure that QEMU_PCI_CAP_EXPRESS is on
>>>>
>>>> Signed-off-by: Yoni Bettan <ybettan@redhat.com>
>>>> ---
>>>>    docs/pcie_pci_bridge.txt           | 2 +-
>>>>    hw/block/nvme.c                    | 1 -
>>>>    hw/net/e1000e.c                    | 1 -
>>>>    hw/pci-bridge/pcie_pci_bridge.c    | 1 -
>>>>    hw/pci-bridge/pcie_root_port.c     | 1 -
>>>>    hw/pci-bridge/xio3130_downstream.c | 1 -
>>>>    hw/pci-bridge/xio3130_upstream.c   | 1 -
>>>>    hw/pci-host/xilinx-pcie.c          | 1 -
>>>>    hw/pci/pci.c                       | 4 +++-
>>>>    hw/scsi/megasas.c                  | 4 ----
>>>>    hw/usb/hcd-xhci.c                  | 7 ++++++-
>>>>    hw/vfio/pci.c                      | 3 ++-
>>>>    include/hw/pci/pci.h               | 3 ---
>>>>    13 files changed, 12 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt
>>>> index 5a4203f97c..ab35ebf3ca 100644
>>>> --- a/docs/pcie_pci_bridge.txt
>>>> +++ b/docs/pcie_pci_bridge.txt
>>>> @@ -110,5 +110,5 @@ To enable device hot-plug into the bridge on Linux there're 3 ways:
>>>>    Implementation
>>>>    ==============
>>>>    The PCIE-PCI bridge is based on PCI-PCI bridge, but also accumulates PCI Express
>>>> -features as a PCI Express device (is_express=1).
>>>> +features as a PCI Express device.
>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>> index 441e21ed1f..9325bc0911 100644
>>>> --- a/hw/block/nvme.c
>>>> +++ b/hw/block/nvme.c
>>>> @@ -1087,7 +1087,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
>>>>        pc->vendor_id = PCI_VENDOR_ID_INTEL;
>>>>        pc->device_id = 0x5845;
>>>>        pc->revision = 2;
>>>> -    pc->is_express = 1;
>>>>        set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>>>        dc->desc = "Non-Volatile Memory Express";
>>>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>>>> index f1af279e8d..c360f0d8c9 100644
>>>> --- a/hw/net/e1000e.c
>>>> +++ b/hw/net/e1000e.c
>>>> @@ -675,7 +675,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)
>>>>        c->revision = 0;
>>>>        c->romfile = "efi-e1000e.rom";
>>>>        c->class_id = PCI_CLASS_NETWORK_ETHERNET;
>>>> -    c->is_express = 1;
>>>>        dc->desc = "Intel 82574L GbE Controller";
>>>>        dc->reset = e1000e_qdev_reset;
>>>> diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
>>>> index a4d827c99d..b7d9ebbec2 100644
>>>> --- a/hw/pci-bridge/pcie_pci_bridge.c
>>>> +++ b/hw/pci-bridge/pcie_pci_bridge.c
>>>> @@ -169,7 +169,6 @@ static void pcie_pci_bridge_class_init(ObjectClass *klass, void *data)
>>>>        DeviceClass *dc = DEVICE_CLASS(klass);
>>>>        HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>>> -    k->is_express = 1;
>>>>        k->is_bridge = 1;
>>>>        k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>>        k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
>>>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>>>> index 9b6e4ce512..45f9e8cd4a 100644
>>>> --- a/hw/pci-bridge/pcie_root_port.c
>>>> +++ b/hw/pci-bridge/pcie_root_port.c
>>>> @@ -145,7 +145,6 @@ static void rp_class_init(ObjectClass *klass, void *data)
>>>>        DeviceClass *dc = DEVICE_CLASS(klass);
>>>>        PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>> -    k->is_express = 1;
>>>>        k->is_bridge = 1;
>>>>        k->config_write = rp_write_config;
>>>>        k->realize = rp_realize;
>>>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>>>> index 1e09d2afb7..613a0d6bb7 100644
>>>> --- a/hw/pci-bridge/xio3130_downstream.c
>>>> +++ b/hw/pci-bridge/xio3130_downstream.c
>>>> @@ -177,7 +177,6 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
>>>>        DeviceClass *dc = DEVICE_CLASS(klass);
>>>>        PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>> -    k->is_express = 1;
>>>>        k->is_bridge = 1;
>>>>        k->config_write = xio3130_downstream_write_config;
>>>>        k->realize = xio3130_downstream_realize;
>>>> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
>>>> index 227997ce46..d4645bddee 100644
>>>> --- a/hw/pci-bridge/xio3130_upstream.c
>>>> +++ b/hw/pci-bridge/xio3130_upstream.c
>>>> @@ -148,7 +148,6 @@ static void xio3130_upstream_class_init(ObjectClass *klass, void *data)
>>>>        DeviceClass *dc = DEVICE_CLASS(klass);
>>>>        PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>> -    k->is_express = 1;
>>>>        k->is_bridge = 1;
>>>>        k->config_write = xio3130_upstream_write_config;
>>>>        k->realize = xio3130_upstream_realize;
>>>> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
>>>> index 7659253090..a4ca3ba30f 100644
>>>> --- a/hw/pci-host/xilinx-pcie.c
>>>> +++ b/hw/pci-host/xilinx-pcie.c
>>>> @@ -298,7 +298,6 @@ static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
>>>>        k->device_id = 0x7021;
>>>>        k->revision = 0;
>>>>        k->class_id = PCI_CLASS_BRIDGE_HOST;
>>>> -    k->is_express = true;
>>>>        k->is_bridge = true;
>>>>        k->init = xilinx_pcie_root_init;
>>>>        k->exit = pci_bridge_exitfn;
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index b2d139bd9a..6b5676b0f4 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -2014,12 +2014,14 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>    {
>>>>        PCIDevice *pci_dev = (PCIDevice *)qdev;
>>>>        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>>>> +    ObjectClass *klass = OBJECT_CLASS(pc);
>>>>        Error *local_err = NULL;
>>>>        PCIBus *bus;
>>>>        bool is_default_rom;
>>>>        /* initialize cap_present for pci_is_express() and pci_config_size() */
>>>> -    if (pc->is_express) {
>>>> +    if (object_class_dynamic_cast(klass, INTERFACE_PCIE_DEVICE) &&
>>>> +       !object_class_dynamic_cast(klass, INTERFACE_CONVENTIONAL_PCI_DEVICE)) {
>>>>            pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> I suggest a comment here explaining that hybrid devices must
>>> manage QEMU_PCI_CAP_EXPRESS manually themselves.
>> It is a good idea, I will do it!
>>>>        }
>>>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>>>> index d5eae6239a..ee51feda59 100644
>>>> --- a/hw/scsi/megasas.c
>>>> +++ b/hw/scsi/megasas.c
>>>> @@ -2447,7 +2447,6 @@ typedef struct MegasasInfo {
>>>>        uint16_t subsystem_id;
>>>>        int ioport_bar;
>>>>        int mmio_bar;
>>>> -    bool is_express;
>>>>        int osts;
>>>>        const VMStateDescription *vmsd;
>>>>        Property *props;
>>>> @@ -2465,7 +2464,6 @@ static struct MegasasInfo megasas_devices[] = {
>>>>            .ioport_bar = 2,
>>>>            .mmio_bar = 0,
>>>>            .osts = MFI_1078_RM | 1,
>>>> -        .is_express = false,
>>>>            .vmsd = &vmstate_megasas_gen1,
>>>>            .props = megasas_properties_gen1,
>>>>            .interfaces = (InterfaceInfo[]) {
>>>> @@ -2482,7 +2480,6 @@ static struct MegasasInfo megasas_devices[] = {
>>>>            .ioport_bar = 0,
>>>>            .mmio_bar = 1,
>>>>            .osts = MFI_GEN2_RM,
>>>> -        .is_express = true,
>>>>            .vmsd = &vmstate_megasas_gen2,
>>>>            .props = megasas_properties_gen2,
>>>>            .interfaces = (InterfaceInfo[]) {
>>>> @@ -2506,7 +2503,6 @@ static void megasas_class_init(ObjectClass *oc, void *data)
>>>>        pc->subsystem_vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
>>>>        pc->subsystem_id = info->subsystem_id;
>>>>        pc->class_id = PCI_CLASS_STORAGE_RAID;
>>>> -    pc->is_express = info->is_express;
>>>>        e->mmio_bar = info->mmio_bar;
>>>>        e->ioport_bar = info->ioport_bar;
>>>>        e->osts = info->osts;
>>>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>>>> index af3a9d88de..2e4dd71248 100644
>>>> --- a/hw/usb/hcd-xhci.c
>>>> +++ b/hw/usb/hcd-xhci.c
>>>> @@ -3649,6 +3649,11 @@ static Property xhci_properties[] = {
>>>>        DEFINE_PROP_END_OF_LIST(),
>>>>    };
>>>> +static void xhci_instance_init(Object *obj)
>>>> +{
>>>> +    PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> I suggest adding a comment explaining why we need to set
>>> QEMU_PCI_CAP_EXPRESS manually here.
>>>
>>> I just noticed that every other device that sets/unsets
>>> QEMU_PCI_CAP_EXPRESS do it on realize:
>>>
>>> $ g grep -p QEMU_PCI_CAP_EXPRESS
>>> hw/net/vmxnet3.c=static void vmxnet3_realize(DeviceState *qdev, Error **errp)
>>> hw/net/vmxnet3.c:        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> hw/pci/pci.c=static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>> hw/pci/pci.c:        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> hw/scsi/vmw_pvscsi.c=static void pvscsi_realize(DeviceState *qdev, Error **errp)
>>> hw/scsi/vmw_pvscsi.c:        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> hw/vfio/pci.c=static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>>> hw/vfio/pci.c:        vdev->pdev.cap_present &= ~QEMU_PCI_CAP_EXPRESS;
>>> hw/virtio/virtio-pci.c=static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>>> hw/virtio/virtio-pci.c:        pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS;
>>> hw/virtio/virtio-pci.c=static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>>> hw/virtio/virtio-pci.c:        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> include/hw/pci/pci.h=enum {
>>> include/hw/pci/pci.h:    QEMU_PCI_CAP_EXPRESS = 0x4,
>>> include/hw/pci/pci.h=static inline int pci_is_express(const PCIDevice *d)
>>> include/hw/pci/pci.h:    return d->cap_present & QEMU_PCI_CAP_EXPRESS;
>> Those devices are initialized in instance_init rather than in realize
>> because unlike other
>> devices that are initialized in realize those devices are not a function of
>> the Qemu command
>> line so we don't need to wait to the realize function.
> Thanks, I think now I get it: vmxnet3, pvscsi and virtio-pci do
> set QEMU_PCI_CAP_EXPRESS conditionally, but vfio and xhci don't.
Exactly
>
> Now, my question is: why is this inconsistent?  Can't we make all
> devices use the same mechanisms to enable/disable
> QEMU_PCI_CAP_EXPRESS, including xhci and vfio?
Maybe there is a way but I don't see it.

The problem is that devices that needs the QEMU_PCI_CAP_EXPRESS on
*unconditionally*, needs it before **their** realize function because before
the patch, they where initializing in according to is_express flag that was
set on **their** class_init function.

On the other hand the devices that needs the QEMU_PCI_CAP_EXPRESS on
*conditionally*, are checking some fields that are set according to the
Qemu command line so the condition can't be checked on instance_init
function.

Do you see the problem?

An other option is to override the realize function for each hybrid 
device (that had is_express =1 )
so that the new function will turn on the flag and only then run the 
original realize function..

But it is ugly, much longer and not so clear (require new macros, new 
filed for parent_realize int the struct,
new class for one device - therefor also add fields to type info, new 
"parent_realize" function to run before
realize etc)
>
>
>> I added a comment about it.
>>> We probably should address this inconsistency, while being
>>> careful to not introduce compatibility bugs.  Probably
>>> pci_config_alloc() is with the QEMU_PCI_CAP_EXPRESS cleared is on
>>> vmxnet3, pvscsi, and virtio-pci?
>> I am not sure I understood what you meant about the pci_config_alloc()
>> function.
> I was confused because this gets called before
> PCIDeviceClass::realize:
>
>    pci_qdev_realize() -> do_pci_register_device() -> pci_config_alloc()
>      -> pci_config_size() -> pci_is_express()
>
> But my mistake was to assume that pvscsi_realize(),
> virtio_pci_dc_realize() and vmxnet3_realize() were
> PCIDeviceClass::realize.  They are not: they are
> DeviceClass::realize methods, and run before pci_qdev_realize().
>
> Anyway, I think this is confusing and requires too much
> boilerplate code on the hybrid devices.
I think the overhead is the same, instead of telling a new device
is_express = 1 now you turn QEMU_PCI_CAP_EXPRESS on instead
we would have been doing it even before the change with is_express
field - but it requires a realize function.
>   We could have a common
> mechanism for hybrid devices to enable/disable
> QEMU_PCI_CAP_EXPRESS, instead of requiring each device to
> reimplement DeviceClass::realize?
I guess we can try to figure it out
> Note that I'm just speculating about potential cleanups for the
> future.  Your patch is still a step in the right direction.
It is nice to here. But I agree with you we should try to automate
it.
>
>
>>>
>>>> +}
>>>> +
>>>>    static void xhci_class_init(ObjectClass *klass, void *data)
>>>>    {
>>>>        PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>> @@ -3661,7 +3666,6 @@ static void xhci_class_init(ObjectClass *klass, void *data)
>>>>        k->realize      = usb_xhci_realize;
>>>>        k->exit         = usb_xhci_exit;
>>>>        k->class_id     = PCI_CLASS_SERIAL_USB;
>>>> -    k->is_express   = 1;
>>>>    }
>>>>    static const TypeInfo xhci_info = {
>>>> @@ -3669,6 +3673,7 @@ static const TypeInfo xhci_info = {
>>>>        .parent        = TYPE_PCI_DEVICE,
>>>>        .instance_size = sizeof(XHCIState),
>>>>        .class_init    = xhci_class_init,
>>>> +    .instance_init = xhci_instance_init,
>>>>        .abstract      = true,
>>>>        .interfaces = (InterfaceInfo[]) {
>>>>            { INTERFACE_PCIE_DEVICE },
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index c977ee327f..afad0c002f 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -2972,6 +2972,8 @@ static void vfio_instance_init(Object *obj)
>>>>        vdev->host.function = ~0U;
>>>>        vdev->nv_gpudirect_clique = 0xFF;
>>>> +
>>>> +    pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>> Same as above: a comment explaining why this is needed would be
>>> useful.
>> same as above: I added a comment
>>>>    }
>>>>    static Property vfio_pci_dev_properties[] = {
>>>> @@ -3026,7 +3028,6 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
>>>>        pdc->exit = vfio_exitfn;
>>>>        pdc->config_read = vfio_pci_read_config;
>>>>        pdc->config_write = vfio_pci_write_config;
>>>> -    pdc->is_express = 1; /* We might be */
>>>>    }
>>>>    static const TypeInfo vfio_pci_dev_info = {
>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>>> index 8d02a0a383..a27be85111 100644
>>>> --- a/include/hw/pci/pci.h
>>>> +++ b/include/hw/pci/pci.h
>>>> @@ -236,9 +236,6 @@ typedef struct PCIDeviceClass {
>>>>         */
>>>>        int is_bridge;
>>>> -    /* pcie stuff */
>>>> -    int is_express;   /* is this device pci express? */
>>>> -
>>>>        /* rom bar */
>>>>        const char *romfile;
>>>>    } PCIDeviceClass;
>>>> -- 
>>>> 2.14.3
>>>>
>>>>