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 <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.
> > >
> > > 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
> > > >
> > > >
> >
> >
