Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/17/2015 05:15 PM, Laszlo Ersek wrote:
> On 06/17/15 16:02, Michael S. Tsirkin wrote:
>> On Wed, Jun 17, 2015 at 03:55:32PM +0200, Laszlo Ersek wrote:
>>> On 06/17/15 15:42, Michael S. Tsirkin wrote:
>>>> On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
>>>>> In the PCI expander bridge, we will want to disable those features of
>>>>> pci-bridge that relate to SHPC (standard hotplug controller):
>>>>>
>>>>> - SHPC bar and underlying MemoryRegion
>>>>> - interrupt (INTx or MSI)
>>>>> - effective hotplug callbacks
>>>>> - other SHPC hooks (initialization, cleanup, migration etc)
>>>>>
>>>>> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
>>>>> the above if the bit is explicitly cleared.
>>>>>
>>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>>
>>>> I think I would prefer naming the property "shpc".
>>>> In particular ACPI hotplug still works fine.
>>>
>>> Will do.
>>>
>>> Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
>>> what would be your preference?
>>
>> PCI_BRIDGE_DEV_F_SHPC_REQ
>>
>> Also add macro for "shpc" property string to avoid duplication.
>> Also clear msi explicitly - no need for that to depend on shpc.
>>
>>> Should I also rename the helper function hotplug_enabled() to
>>> shpc_enabled()?
>>
>> It should be tested only once - before shpc is initialized.
>> All other users should test QEMU_PCI_CAP_SHPC - if you like,
>> using a helper shpc_present.
>
> I'll make one attempt (in v7) to get this right. If I miss it, I'll ask
> Marcel to please take over the series.
No problem here,
I'll take it from V8 if necessary, but I think it will be just fine. :)

Thanks,
Marcel

>
> Thanks
> Laszlo
>
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>>>
>>>>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>      v6:
>>>>>      - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
>>>>>        hotplug-less bridge for PXB" from v5) [Michael]
>>>>>      - I tested migration-to-file and migration-from-file, with a single PXB
>>>>>        behind which a virtio-scsi HBA lived
>>>>>      - No idea how to test the hotplug callbacks. I tried the device_add and
>>>>>        device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
>>>>>        monitor reports no error at all; the callbacks don't seem to be
>>>>>        reached.
>>>>>      - Retested with windows 2012; it works
>>>>>
>>>>>   hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
>>>>>   1 file changed, 78 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>>>> index d697c7b..a0a1ce9 100644
>>>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>>>> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
>>>>>       MemoryRegion bar;
>>>>>       uint8_t chassis_nr;
>>>>>   #define PCI_BRIDGE_DEV_F_MSI_REQ 0
>>>>> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
>>>>>       uint32_t flags;
>>>>>   };
>>>>>   typedef struct PCIBridgeDev PCIBridgeDev;
>>>>>
>>>>> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
>>>>> +{
>>>>> +    return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
>>>>> +}
>>>>> +
>>>>>   static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>   {
>>>>>       PCIBridge *br = PCI_BRIDGE(dev);
>>>>> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>       if (err) {
>>>>>           goto bridge_error;
>>>>>       }
>>>>> -    dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>>>> -    memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev));
>>>>> -    err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>>>> -    if (err) {
>>>>> -        goto shpc_error;
>>>>> +
>>>>> +    if (hotplug_enabled(bridge_dev)) {
>>>>> +        dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>>>> +        memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>>>>> +                           shpc_bar_size(dev));
>>>>> +        err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>>>> +        if (err) {
>>>>> +            goto shpc_error;
>>>>> +        }
>>>>>       }
>>>>> +
>>>>>       err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>>>>>       if (err) {
>>>>>           goto slotid_error;
>>>>>       }
>>>>> +
>>>>> +    if (!hotplug_enabled(bridge_dev)) {
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>>       if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>>>>>           msi_supported) {
>>>>>           err = msi_init(dev, 0, 1, true, true);
>>>>> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>   msi_error:
>>>>>       slotid_cap_cleanup(dev);
>>>>>   slotid_error:
>>>>> -    shpc_cleanup(dev, &bridge_dev->bar);
>>>>> +    if (hotplug_enabled(bridge_dev)) {
>>>>> +        shpc_cleanup(dev, &bridge_dev->bar);
>>>>> +    }
>>>>>   shpc_error:
>>>>>       pci_bridge_exitfn(dev);
>>>>>   bridge_error:
>>>>> @@ -89,53 +107,101 @@ bridge_error:
>>>>>   static void pci_bridge_dev_exitfn(PCIDevice *dev)
>>>>>   {
>>>>>       PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>> +
>>>>>       if (msi_present(dev)) {
>>>>>           msi_uninit(dev);
>>>>>       }
>>>>>       slotid_cap_cleanup(dev);
>>>>> -    shpc_cleanup(dev, &bridge_dev->bar);
>>>>> +    if (hotplug_enabled(bridge_dev)) {
>>>>> +        shpc_cleanup(dev, &bridge_dev->bar);
>>>>> +    }
>>>>>       pci_bridge_exitfn(dev);
>>>>>   }
>>>>>
>>>>>   static void pci_bridge_dev_instance_finalize(Object *obj)
>>>>>   {
>>>>> +    /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
>>>>>       shpc_free(PCI_DEVICE(obj));
>>>>>   }
>>>>>
>>>>>   static void pci_bridge_dev_write_config(PCIDevice *d,
>>>>>                                           uint32_t address, uint32_t val, int len)
>>>>>   {
>>>>> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
>>>>> +
>>>>>       pci_bridge_write_config(d, address, val, len);
>>>>>       if (msi_present(d)) {
>>>>>           msi_write_config(d, address, val, len);
>>>>>       }
>>>>> -    shpc_cap_write_config(d, address, val, len);
>>>>> +    if (hotplug_enabled(bridge_dev)) {
>>>>> +        shpc_cap_write_config(d, address, val, len);
>>>>> +    }
>>>>>   }
>>>>>
>>>>>   static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>>>>>   {
>>>>>       PCIDevice *dev = PCI_DEVICE(qdev);
>>>>> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>>
>>>>>       pci_bridge_reset(qdev);
>>>>> -    shpc_reset(dev);
>>>>> +    if (hotplug_enabled(bridge_dev)) {
>>>>> +        shpc_reset(dev);
>>>>> +    }
>>>>>   }
>>>>>
>>>>>   static Property pci_bridge_dev_properties[] = {
>>>>>                       /* Note: 0 is not a legal chassis number. */
>>>>>       DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
>>>>>       DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true),
>>>>> +    DEFINE_PROP_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
>>>>> +                    true),
>>>>>       DEFINE_PROP_END_OF_LIST(),
>>>>>   };
>>>>>
>>>>> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
>>>>> +{
>>>>> +    PCIDevice *dev = opaque;
>>>>> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>> +
>>>>> +    return hotplug_enabled(bridge_dev);
>>>>> +}
>>>>> +
>>>>>   static const VMStateDescription pci_bridge_dev_vmstate = {
>>>>>       .name = "pci_bridge",
>>>>>       .fields = (VMStateField[]) {
>>>>>           VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
>>>>> -        SHPC_VMSTATE(shpc, PCIDevice, NULL),
>>>>> +        SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
>>>>>           VMSTATE_END_OF_LIST()
>>>>>       }
>>>>>   };
>>>>>
>>>>> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
>>>>> +                                      DeviceState *dev, Error **errp)
>>>>> +{
>>>>> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>>> +
>>>>> +    if (!hotplug_enabled(bridge_dev)) {
>>>>> +        error_setg(errp, "standard hotplug controller has been disabled for "
>>>>> +                   "this %s", TYPE_PCI_BRIDGE_DEV);
>>>>> +        return;
>>>>> +    }
>>>>> +    shpc_device_hotplug_cb(hotplug_dev, dev, errp);
>>>>> +}
>>>>> +
>>>>> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>>> +                                                 DeviceState *dev,
>>>>> +                                                 Error **errp)
>>>>> +{
>>>>> +    PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>>> +
>>>>> +    if (!hotplug_enabled(bridge_dev)) {
>>>>> +        error_setg(errp, "standard hotplug controller has been disabled for "
>>>>> +                   "this %s", TYPE_PCI_BRIDGE_DEV);
>>>>> +        return;
>>>>> +    }
>>>>> +    shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
>>>>> +}
>>>>> +
>>>>>   static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>>   {
>>>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>>> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>>       dc->props = pci_bridge_dev_properties;
>>>>>       dc->vmsd = &pci_bridge_dev_vmstate;
>>>>>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>>> -    hc->plug = shpc_device_hotplug_cb;
>>>>> -    hc->unplug_request = shpc_device_hot_unplug_request_cb;
>>>>> +    hc->plug = pci_bridge_dev_hotplug_cb;
>>>>> +    hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
>>>>>   }
>>>>>
>>>>>   static const TypeInfo pci_bridge_dev_info = {
>>>>> --
>>>>> 1.8.3.1
>>>>>
>