q35: Fix memory_region_del_eventfd: Assertion `i != mr->ioeventfd_nb' failed.

On Tue, 2014-05-27 at 12:28 -0400, Etienne Martineau wrote:
> Hi,
> 
> When using virtio disk plug/unplug with q35 machine I see two problems. Note
> that when using the same sequence with default 440FX I see no issues.
Hi Etienne, thank you for reporting the problem.

> 
> A) 'pcie.0' does not support hotplugging'
Well, pcie.0 represents the Root Complex and devices attached to it
are Root Complex Integrated Endpoint and as the name suggests, should
not be able to be hot plugged/unplugged.
From PCI EXPRESS spec:
    A Root Complex Integrated Endpoint may not be hot-plugged independent of the Root
    Complex as a whole.

That being said, this should not crash QEMU! We should not allow hot plug/unplug
for pcie.0.

> 
> I can workaround this problem if I manually specify 
> "-readconfig /usr/share/qemu/Q35-chipset.cfg" on the QEMU command line and
> use the following monitor command to plug the disk:
> 
> (qemu) device_add virtio-blk-pci,id=device1,drive=drive1,scsi=on,bus=ich9-pcie-port-1
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 

> 
> The content of Q35-chipset.cfg is:
> [device "ich9-pcie-port-1"]
>   driver = "ioh3420"
>   multifunction = "on"
>   bus = "pcie.0"
> 
> B) Upon disk unplug QEMU is crashing. This is with recent qemu.git. The following patch
> solve the issue.
As I said before, QEMU it should not crash. 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.

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 have an "in progress" implementation that works for Linux only and should solve your issue.

Subject: [Qemu-devel] [PATCH] pcie: hotplug/unplug for linux

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci/pcie.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

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 <marcel.a@redhat.com>
> ---
>  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