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

[Michael S. Tsirkin]

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

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