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 <m...@redhat.com>
>> Cc: Michael S. Tsirkin <m...@redhat.com>
>> Cc: Marcel Apfelbaum <mar...@redhat.com>
>> Signed-off-by: Laszlo Ersek <ler...@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?
Should I also rename the helper function hotplug_enabled() to
shpc_enabled()?
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
>>
