[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" prope
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property |
Date: |
Wed, 17 Jun 2015 16:15:12 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
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 <address@hidden>
>>>> Cc: Michael S. Tsirkin <address@hidden>
>>>> Cc: Marcel Apfelbaum <address@hidden>
>>>> Signed-off-by: Laszlo Ersek <address@hidden>
>>>
>>>
>>> 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.
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
>>>>
[Qemu-devel] [PATCH v6 6/7] hw/core: explicit OFW unit address callback for SysBusDeviceClass, Laszlo Ersek, 2015/06/17
[Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB, Laszlo Ersek, 2015/06/17
[Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB, Laszlo Ersek, 2015/06/17