[Qemu-devel] [PATCH v3 0/3] hw/pcie: better hotplug/hotunplug support

[Qemu-devel] [PATCH v3 0/3] hw/pcie: better hotplug/hotunplug support

[Marcel Apfelbaum]

v2 -> v3:
 - Rebased the series on PCI branch

v1 -> v2:
 - Addressed Michael S. Tsirkin's comments:
   - patch 2/3:
     - Added "power controller present" to compat_props.
   - patch 3/3:
     - Unplug all bus functions on hot-unplug, not only function 0.
   - Minor code refactoring

The current code is broken: it does surprise removal which crashes guests.

Reimplemented the steps:
 - Hotplug triggers both 'present detect change' and
   'attention button pressed'.

 - Hotunplug starts by triggering 'attention button pressed',
   then waits for the OS to power off the device and only
   then detaches it.

Fixes CVE-2014-3471.

patch 1/3: trivial debug message fix
patch 2/3: enable 'power controller' to receive power events from guests
patch 3/3: the actual hotplug/hotunplug implementation.

Tested with Linux and Windows guests and with an e1000 with "PCIe" capability.
Notes:
    Windows requires devices to be pci express in order to enable
    hotplug functionality, so we need to think about converting
    virtio devices to pci express.
    Linux outputs a "Surprise Removal/Addition" info message because
    we trigger 2 events in the same time, however this warning
    can be disregarded or a kernel patch submitted for our scenario.

Marcel Apfelbaum (3):
  hw/pcie: corrected a debug message
  hw/pcie: implement power controller functionality
  hw/pcie: better hotplug/hotunplug support

 hw/pci-bridge/ioh3420.c            |  7 +++++
 hw/pci-bridge/xio3130_downstream.c |  7 +++++
 hw/pci/pcie.c                      | 64 +++++++++++++++++++++++++++++++++-----
 include/hw/i386/pc.h               | 10 +++++-
 include/hw/pci/pci.h               |  3 ++
 include/hw/pci/pcie.h              |  2 ++
 include/hw/pci/pcie_regs.h         |  2 ++
 7 files changed, 87 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/3] hw/pcie: corrected a debug message

[Marcel Apfelbaum]

Trivial issue, discovered while debugging.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci/pcie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 02cde6f..ae92f00 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -224,7 +224,7 @@ static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
     *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
     uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
 
-    PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: %d\n", state);
+    PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: 0x%x\n", sltsta);
     if (sltsta & PCI_EXP_SLTSTA_EIS) {
         /* the slot is electromechanically locked.
          * This error is propagated up to qdev and then to HMP/QMP.
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/3] hw/pcie: implement power controller functionality

[Marcel Apfelbaum]

It is needed by hot-unplug in order to get an indication
from the OS when the device can be physically detached.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci-bridge/ioh3420.c            |  7 +++++++
 hw/pci-bridge/xio3130_downstream.c |  7 +++++++
 hw/pci/pcie.c                      | 33 ++++++++++++++++++++++++++++++++-
 include/hw/i386/pc.h               | 10 +++++++++-
 include/hw/pci/pci.h               |  3 +++
 include/hw/pci/pcie.h              |  2 ++
 include/hw/pci/pcie_regs.h         |  2 ++
 7 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index f4e17ac..7cd87fc 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -180,6 +180,12 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,
     return PCIE_SLOT(d);
 }
 
+static Property ioh3420_props[] = {
+    DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
+                    QEMU_PCIE_SLTCAP_PCP_BITNR, true),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static const VMStateDescription vmstate_ioh3420 = {
     .name = "ioh-3240-express-root-port",
     .version_id = 1,
@@ -210,6 +216,7 @@ static void ioh3420_class_init(ObjectClass *klass, void *data)
     dc->desc = "Intel IOH device id 3420 PCIE Root Port";
     dc->reset = ioh3420_reset;
     dc->vmsd = &vmstate_ioh3420;
+    dc->props = ioh3420_props;
 }
 
 static const TypeInfo ioh3420_info = {
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 8f22f93..51f20d7 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -147,6 +147,12 @@ PCIESlot *xio3130_downstream_init(PCIBus *bus, int devfn, bool multifunction,
     return PCIE_SLOT(d);
 }
 
+static Property xio3130_downstream_props[] = {
+    DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
+                    QEMU_PCIE_SLTCAP_PCP_BITNR, true),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static const VMStateDescription vmstate_xio3130_downstream = {
     .name = "xio3130-express-downstream-port",
     .version_id = 1,
@@ -177,6 +183,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
     dc->desc = "TI X3130 Downstream Port of PCI Express Switch";
     dc->reset = xio3130_downstream_reset;
     dc->vmsd = &vmstate_xio3130_downstream;
+    dc->props = xio3130_downstream_props;
 }
 
 static const TypeInfo xio3130_downstream_info = {
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ae92f00..d6d9eb8 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -294,6 +294,15 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
                                PCI_EXP_SLTCAP_AIP |
                                PCI_EXP_SLTCAP_ABP);
 
+    if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
+        pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCAP,
+                                   PCI_EXP_SLTCAP_PCP);
+        pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL,
+                                     PCI_EXP_SLTCTL_PCC);
+        pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL,
+                                   PCI_EXP_SLTCTL_PCC);
+    }
+
     pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL,
                                  PCI_EXP_SLTCTL_PIC |
                                  PCI_EXP_SLTCTL_AIC);
@@ -327,6 +336,10 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
 void pcie_cap_slot_reset(PCIDevice *dev)
 {
     uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
+    uint8_t port_type = pcie_cap_get_type(dev);
+
+    assert(port_type == PCI_EXP_TYPE_DOWNSTREAM ||
+           port_type == PCI_EXP_TYPE_ROOT_PORT);
 
     PCIE_DEV_PRINTF(dev, "reset\n");
 
@@ -339,9 +352,27 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                  PCI_EXP_SLTCTL_PDCE |
                                  PCI_EXP_SLTCTL_ABPE);
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
-                               PCI_EXP_SLTCTL_PIC_OFF |
                                PCI_EXP_SLTCTL_AIC_OFF);
 
+    if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
+        bool populated;
+        uint16_t pic;
+
+        /* Downstream ports enforce device number 0. */
+        populated = (pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0] != NULL);
+
+        if (populated) {
+            pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL,
+                                         PCI_EXP_SLTCTL_PCC);
+        } else {
+            pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
+                                       PCI_EXP_SLTCTL_PCC);
+        }
+
+        pic = populated ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF;
+        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, pic);
+     }
+
     pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
                                  PCI_EXP_SLTSTA_EIS |/* on reset,
                                                         the lock is released */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 76d4c6e..4a9a550 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -297,8 +297,16 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
             .driver   = "ICH9-LPC",\
             .property = "memory-hotplug-support",\
             .value    = "off",\
+        },{\
+            .driver   = "xio3130-downstream",\
+            .property = COMPAT_PROP_PCP,\
+            .value    = "off",\
+        },{\
+            .driver   = "ioh3420",\
+            .property = COMPAT_PROP_PCP,\
+            .value    = "off",\
         }
-
+ 
 #define PC_Q35_COMPAT_1_7 \
         PC_COMPAT_1_7, \
         PC_Q35_COMPAT_2_0, \
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8c25ae5..c352c7b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -158,6 +158,9 @@ enum {
     QEMU_PCI_CAP_SHPC = (1 << QEMU_PCI_SHPC_BITNR),
 #define QEMU_PCI_SLOTID_BITNR 6
     QEMU_PCI_CAP_SLOTID = (1 << QEMU_PCI_SLOTID_BITNR),
+    /* PCI Express capability - Power Controller Present */
+#define QEMU_PCIE_SLTCAP_PCP_BITNR 7
+    QEMU_PCIE_SLTCAP_PCP = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
 };
 
 #define TYPE_PCI_DEVICE "pci-device"
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index b0bf7e3..7fe81f3 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -76,6 +76,8 @@ struct PCIExpressDevice {
     PCIEAERLog aer_log;
 };
 
+#define COMPAT_PROP_PCP "power_controller_present"
+
 /* PCI express capability helper functions */
 int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
 int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 4d123d9..652d9fc 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -57,6 +57,8 @@
 #define PCI_EXP_SLTCTL_PIC_SHIFT        (ffs(PCI_EXP_SLTCTL_PIC) - 1)
 #define PCI_EXP_SLTCTL_PIC_OFF                          \
     (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT)
+#define PCI_EXP_SLTCTL_PIC_ON                          \
+    (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT)
 
 #define PCI_EXP_SLTCTL_SUPPORTED        \
             (PCI_EXP_SLTCTL_ABPE |      \
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/3] hw/pcie: better hotplug/hotunplug support

[Marcel Apfelbaum]

The current code is broken: it does surprise removal which crashes guests.

Reimplemented the steps:
 - Hotplug triggers both 'present detect change' and
   'attention button pressed'.

 - Hotunplug starts by triggering 'attention button pressed',
   then waits for the OS to power off the device and only
   then detaches it.

Fixes CVE-2014-3471.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci/pcie.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index d6d9eb8..da32589 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                PCI_EXP_SLTSTA_PDS);
-    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
+    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
+                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
 }
 
 void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
 
-    object_unparent(OBJECT(dev));
-    pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
-                                 PCI_EXP_SLTSTA_PDS);
-    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
+    pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
 }
 
 /* pci express slot for pci express root/downstream port
@@ -383,6 +381,11 @@ void pcie_cap_slot_reset(PCIDevice *dev)
     hotplug_event_update_event_status(dev);
 }
 
+static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    object_unparent(OBJECT(dev));
+}
+
 void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint32_t addr, uint32_t val, int len)
 {
@@ -407,6 +410,22 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                         sltsta);
     }
 
+    /*
+     * If the slot is polulated, power indicator is off and power
+     * controller is off, it is safe to detach the devices.
+     */
+    if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
+        ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) {
+            PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
+            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
+                                pcie_unplug_device, NULL);
+
+            pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                         PCI_EXP_SLTSTA_PDS);
+            pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
+                                       PCI_EXP_SLTSTA_PDC);
+    }
+
     hotplug_event_notify(dev);
 
     /* 
-- 
1.8.3.1