On Thu, 2014-06-19 at 17:43 +0300, Michael S. Tsirkin wrote: > 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. I'll add a note, sure.
> > > 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. I thought is clear :( Basically: If the device is present, power indicator off and power controller off, it is safe to detach the device. > > > + 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? Yes, but since it is PCIe we only have one device, but we may have a multi-function device... > You need to do this for all functions, not just function 0. So bus devices are actually functions... OK, I'll run a loop here. > > > + 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? No, "PDS on" means the devices is present, so we clear it only after the OS powers it off. > > > + } > > + } > > + > > hotplug_event_notify(dev); > > > > /* > > -- > > 1.8.3.1