| Message ID | 1403185941-19561-4-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <marcel.a@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
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 <marcel.a@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
On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > 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 <marcel.a@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. Sure, put this in the comment. > > > > > + 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... Exactly. Up to 256 functions. > > 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. So why not clear PDS unconditionally? > > > > > + } > > > + } > > > + > > > hotplug_event_notify(dev); > > > > > > /* > > > -- > > > 1.8.3.1 > >
On Sun, 2014-06-22 at 14:03 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > > 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 <marcel.a@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. > > Sure, put this in the comment. > > > > > > > > + 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... > > Exactly. Up to 256 functions. > > > > 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. > > So why not clear PDS unconditionally? Because we are 'allowed' to remove the device only when the OS tells us the power is off. Before that, it can result in data loss, since the device driver is not yet unloaded. > > > > > > > + } > > > > + } > > > > + > > > > hotplug_event_notify(dev); > > > > > > > > /* > > > > -- > > > > 1.8.3.1 > > > >
On Sun, Jun 22, 2014 at 02:11:05PM +0300, Marcel Apfelbaum wrote: > On Sun, 2014-06-22 at 14:03 +0300, Michael S. Tsirkin wrote: > > On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > > > 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 <marcel.a@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. > > > > Sure, put this in the comment. > > > > > > > > > > > + 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... > > > > Exactly. Up to 256 functions. > > > > > > 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. > > > > So why not clear PDS unconditionally? > Because we are 'allowed' to remove the device only when the OS tells us > the power is off. Before that, it can result in data loss, > since the device driver is not yet unloaded. Yes. But if (slot_dev) seems not to be needed: if slot is empty we can clear PDC. > > > > > > > > > > + } > > > > > + } > > > > > + > > > > > hotplug_event_notify(dev); > > > > > > > > > > /* > > > > > -- > > > > > 1.8.3.1 > > > > > > > >
On Sun, 2014-06-22 at 14:12 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 02:11:05PM +0300, Marcel Apfelbaum wrote: > > On Sun, 2014-06-22 at 14:03 +0300, Michael S. Tsirkin wrote: > > > On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > > > > 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 <marcel.a@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. > > > > > > Sure, put this in the comment. > > > > > > > > > > > > > > + 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... > > > > > > Exactly. Up to 256 functions. > > > > > > > > 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. > > > > > > So why not clear PDS unconditionally? > > Because we are 'allowed' to remove the device only when the OS tells us > > the power is off. Before that, it can result in data loss, > > since the device driver is not yet unloaded. > > Yes. But if (slot_dev) seems not to be needed: > if slot is empty we can clear PDC. You mean PDS, PDC is the event we send to the OS. The main reason I added this "if clause" is to protect against multiple OS events of 'power controller off' and 'power indicator off', but now I see that indeed I don't need that because I check "sltsta & PCI_EXP_SLTSTA_PDS". I will remove that if. Thanks, Marcel > > > > > > > > > > > > > > > > + } > > > > > > + } > > > > > > + > > > > > > hotplug_event_notify(dev); > > > > > > > > > > > > /* > > > > > > -- > > > > > > 1.8.3.1 > > > > > > > > > > > >
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); } + 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) { + 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); + } + } + hotplug_event_notify(dev); /*
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. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- hw/pci/pcie.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)