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

[Marcel Apfelbaum <marcel.a@redhat.com>]

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