[Qemu-devel] Re: [PATCH] pci: allow hotplug removal of cold-plugged devices

[Michael S. Tsirkin]

On Mon, Nov 15, 2010 at 11:23:44AM +0900, Isaku Yamahata wrote:
> Thank you for catching pcie part.
> The following fix is necessary.
> 
> diff --git a/hw/pcie.c b/hw/pcie.c
> index 4df48b8..f461c1c 100644
> --- a/hw/pcie.c
> +++ b/hw/pcie.c
> @@ -221,7 +221,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
>       */
>      assert(PCI_FUNC(pci_dev->devfn) == 0);
>  
> -    if (state) {
> +    if (state == PCI_HOTPLUG_ENABLED) {
>          pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>                                     PCI_EXP_SLTSTA_PDS);
>          pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
> 

Okay. Although note that the enum values were carefully
selected to make the old code work as well.

> On Sun, Nov 14, 2010 at 04:18:04PM +0200, Michael S. Tsirkin wrote:
> > This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
> > which broke hotplug removal of cold plugged devices:
> > 
> > - pass addition/removal state to hotplug callbacks
> > - use that in piix and pcie
> > 
> > This also fixes an assert on hotplug removal of coldplugged
> > express devices.
> > 
> > Reported-by: by Cam Macdonell <address@hidden>.
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> > 
> > So I think the below would be the cleanest way
> > to fix the bug as we keep the hot/cold plug logic
> > in a central palce. Untested. Comments? Cam?
> > 
> > 
> > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> > index 66c7885..f549089 100644
> > --- a/hw/acpi_piix4.c
> > +++ b/hw/acpi_piix4.c
> > @@ -585,7 +585,8 @@ static void pciej_write(void *opaque, uint32_t addr, 
> > uint32_t val)
> >      PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
> >  }
> >  
> > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int 
> > state);
> > +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> > +                                PCIHotplugState state);
> >  
> >  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
> >  {
> > @@ -615,18 +616,23 @@ static void disable_device(PIIX4PMState *s, int slot)
> >      s->pci0_status.down |= (1 << slot);
> >  }
> >  
> > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int 
> > state)
> > +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> > +                           PCIHotplugState state)
> >  {
> >      int slot = PCI_SLOT(dev->devfn);
> >      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
> >                                  DO_UPCAST(PCIDevice, qdev, qdev));
> >  
> > -    if (!dev->qdev.hotplugged)
> > +    /* Don't send event when device is enabled during qemu machine 
> > creation:
> > +     * it is present on boot, no hotplug event is necessary. We do send an
> > +     * event when the device is disabled later. */
> > +    if (state == PCI_COLDPLUG_ENABLED) {
> >          return 0;
> > +    }
> >  
> >      s->pci0_status.up = 0;
> >      s->pci0_status.down = 0;
> > -    if (state) {
> > +    if (state == PCI_HOTPLUG_ENABLED) {
> >          enable_device(s, slot);
> >      } else {
> >          disable_device(s, slot);
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 30e1603..316b24f 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1566,8 +1566,11 @@ static int pci_qdev_init(DeviceState *qdev, 
> > DeviceInfo *base)
> >      pci_add_option_rom(pci_dev);
> >  
> >      if (bus->hotplug) {
> > -        /* lower layer must check qdev->hotplugged */
> > -        rc = bus->hotplug(bus->hotplug_qdev, pci_dev, 1);
> > +        /* Let buses differentiate between hotplug and when device is
> > +         * enabled during qemu machine creation. */
> > +        rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
> > +                          qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
> > +                          PCI_COLDPLUG_ENABLED);
> >          if (rc != 0) {
> >              int r = pci_unregister_device(&pci_dev->qdev);
> >              assert(!r);
> > @@ -1581,7 +1584,8 @@ static int pci_unplug_device(DeviceState *qdev)
> >  {
> >      PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
> >  
> > -    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, 0);
> > +    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> > +                             PCI_HOTPLUG_DISABLED);
> >  }
> >  
> >  void pci_qdev_register(PCIDeviceInfo *info)
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 7100804..09b3e4c 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -214,7 +214,15 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
> >  
> >  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> >  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, int 
> > state);
> > +
> > +typedef enum {
> > +    PCI_HOTPLUG_DISABLED,
> > +    PCI_HOTPLUG_ENABLED,
> > +    PCI_COLDPLUG_ENABLED,
> > +} PCIHotplugState;
> > +
> > +typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> > +                              PCIHotplugState state);
> >  void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
> >                           const char *name, int devfn_min);
> >  PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
> > diff --git a/hw/pcie.c b/hw/pcie.c
> > index 35918f7..4df48b8 100644
> > --- a/hw/pcie.c
> > +++ b/hw/pcie.c
> > @@ -192,14 +192,16 @@ static void pcie_cap_slot_event(PCIDevice *dev, 
> > PCIExpressHotPlugEvent event)
> >  }
> >  
> >  static int pcie_cap_slot_hotplug(DeviceState *qdev,
> > -                                 PCIDevice *pci_dev, int state)
> > +                                 PCIDevice *pci_dev, PCIHotplugState state)
> >  {
> >      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
> >      uint8_t *exp_cap = d->config + d->exp.exp_cap;
> >      uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
> >  
> > -    if (!pci_dev->qdev.hotplugged) {
> > -        assert(state); /* this case only happens at machine creation. */
> > +    /* Don't send event when device is enabled during qemu machine 
> > creation:
> > +     * it is present on boot, no hotplug event is necessary. We do send an
> > +     * event when the device is disabled later. */
> > +    if (state == PCI_COLDPLUG_ENABLED) {
> >          pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >                                     PCI_EXP_SLTSTA_PDS);
> >          return 0;
> > 
> 
> -- 
> yamahata