On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote:
> 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.
>
Pls not here that current code is broken: it does surprise removal which
crashes guests.
> Signed-off-by: Marcel Apfelbaum <marce...@redhat.com>
> ---
> hw/pci/pcie.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index f8bf515..9cfd93d 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
> @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
> sltsta);
> }
>
pls add code comments explaining the logic here.
> + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
> + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) {
> + PCIDevice *slot_dev =
> pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0];
> + if (slot_dev) {
Here you want to remove all devices behind the bridge?
You need to do this for all functions, not just function 0.
> + object_unparent(OBJECT(slot_dev));
> + 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);
These bits need to be cleared in any case?
> + }
> + }
> +
> hotplug_event_notify(dev);
>
> /*
> --
> 1.8.3.1
