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

[Marcel Apfelbaum]

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 <address@hidden>
> > ---
> >  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