Re: [Qemu-devel] [PATCH] q35: Fix memory_region_del_eventfd: Assertion `i != mr->ioeventfd_nb' failed.

[Etienne Martineau]

Hi Marcel, thanks for your reply.

On 14-05-28 05:10 AM, Marcel Apfelbaum wrote:
> This is correct since now you are using a Root Complex Port.
> If you don't want to use a config file use:
> 
> <qemu-bin> [...] -device ioh3420,bus=pcie.0,id=ich9-pcie-port-1,slot=3 
You're right I could have done it on the command line directly.

> As I said before, QEMU it should not crash.
Well without the fix QEMU assert in memory_region_del_eventfd(). I can provide 
the bt if
needed?

> However, the reason that it does not work
> is because the current implementation uses "suprise"removal and the OS does 
> not have
> the chance to unload the driver and power down the device.
Yes exactly

> Your timer solves this by letting the OS to acknowledge "PRESENT DETECT" 
> change and
> unload the device, this however is not the optimal approach since you have a 
> race condition.
I must agree with you that if the disk is removed and plugged back within the 
timer period then it
will race.

> 
> I have an "in progress" implementation that works for Linux only and should 
> solve your issue.
This patch works fine for me. Thanks!

I was looking for a mechanism to initiate the object_unparent() based on some 
feedback from the OS
inside the pcie_cap_slot_write_config() but could not find anything useful with 
the current 
notification scheme.

The 'attention button' definitely provide a nice handshake to QEMU for clean-up.

> 
> Subject: [Qemu-devel] [PATCH] pcie: hotplug/unplug for linux
> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
>  hw/pci/pcie.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 02cde6f..8f06713 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -224,7 +224,7 @@ static void pcie_cap_slot_hotplug_common(PCIDevice 
> *hotplug_dev,
>      *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
>      uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
>  
> -    PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: %d\n", state);
> +    PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: 0x%x\n", sltsta);
>      if (sltsta & PCI_EXP_SLTSTA_EIS) {
>          /* the slot is electromechanically locked.
>           * This error is propagated up to qdev and then to HMP/QMP.
> @@ -258,7 +258,7 @@ 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_push_attention_button(PCI_DEVICE(hotplug_dev));
>  }
>  
>  void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState 
> *dev,
> @@ -268,10 +268,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
> @@ -292,6 +289,7 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
>                                 PCI_EXP_SLTCAP_HPC |
>                                 PCI_EXP_SLTCAP_PIP |
>                                 PCI_EXP_SLTCAP_AIP |
> +                               PCI_EXP_SLTCAP_PCP |
>                                 PCI_EXP_SLTCAP_ABP);
>  
>      pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL,
> @@ -302,6 +300,7 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
>                                 PCI_EXP_SLTCTL_AIC_OFF);
>      pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL,
>                                 PCI_EXP_SLTCTL_PIC |
> +                               PCI_EXP_SLTCTL_PCC |
>                                 PCI_EXP_SLTCTL_AIC |
>                                 PCI_EXP_SLTCTL_HPIE |
>                                 PCI_EXP_SLTCTL_CCIE |
> @@ -376,6 +375,16 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>                          sltsta);
>      }
>  
> +    if ((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) {
> +            object_unparent(OBJECT(slot_dev));
> +            pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                         PCI_EXP_SLTSTA_PDS);
> +        }
> +    }
> +
>      hotplug_event_notify(dev);
>  
>      /* 
> 

thanks,
Etienne