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

[Marcel Apfelbaum]

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 <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);
 
     /* 
-- 
1.8.3.1


I hope I helped,
Marcel

> 
> Signed-off-by: Etienne Martineau <address@hidden>
> ---
>  hw/pci/pcie.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 02cde6f..4cf8f4c 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -261,17 +261,31 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>      pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
>  }
>  
> +/* Upon hot unplugged the device object must be stay until guest OS finish
> + * device removal ( at least for virtio-pci ). On non PCI Express there is no
> + * issue because the clean up is part of acpi_pcihp_eject_slot().
> + */
> +static void pcie_cap_slot_hot_unplug_cleanup(void *opaque)
> +{
> +    DeviceState *dev = opaque;
> +    object_unparent(OBJECT(dev));
> +}
> +
>  void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState 
> *dev,
>                                   Error **errp)
>  {
>      uint8_t *exp_cap;
> +    QEMUTimer *timer;
>  
>      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);
> +
> +    timer = timer_new_ms(QEMU_CLOCK_REALTIME, 
> +                         pcie_cap_slot_hot_unplug_cleanup, dev);
> +    timer_mod(timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 1000 );
>  }
>  
>  /* pci express slot for pci express root/downstream port