[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] `qdev_free` when unplug a pci device
From: |
Isaku Yamahata |
Subject: |
Re: [Qemu-devel] [PATCH] `qdev_free` when unplug a pci device |
Date: |
Tue, 1 Mar 2011 13:11:57 +0900 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
Hi.
I don't suppose that just introducing pending bits solve the issue.
Your test only use single hot plug/unplug. How about mixing of multiple
hot plug/unplug with different slots.
Zeroing up/down on piix4_device_hotplug() is the culprit.
State machine of (up, down) would be needed.
(up, down) of each slots should be changed following
something like
- on power-on (0, 0)
- on hot plug (0, 0) -> (1, 0)
if other combination -> error
- on hot unpug (1, 0) or (0, 0) -> (0, 1)
(0, 0) is for cold plugged devices.
(1, 0) is for hot plugged devices.
if other combination -> error
- on pciej_write(write on PCI_EJ_BASE)
(0, 1) -> (0, 0) and qdev_free()
if other combination -> ignore
When enabling sci, those bits are checked and raise sci if necessary.
Any comments on this?
Anyway the current pci hotplug-related commands are somewhat broken,
and needs clean up with multifunction hotplug support which is
on my todo list.
thanks
On Mon, Feb 28, 2011 at 10:52:40AM +0800, Wen Congyang wrote:
> Hi Markus Armbruster
>
> At 02/23/2011 04:30 PM, Markus Armbruster Write:
> > Isaku Yamahata <address@hidden> writes:
> >
>
> <snip>
>
> >
> > I don't think this patch is correct. Let me explain.
> >
> > Device hot unplug is *not* guaranteed to succeed.
> >
> > For some buses, such as USB, it always succeeds immediately, i.e. when
> > the device_del monitor command finishes, the device is gone. Live is
> > good.
> >
> > But for PCI, device_del merely initiates the ACPI unplug rain dance. It
> > doesn't wait for the dance to complete. Why? The dance can take an
> > unpredictable amount of time, including forever.
> >
> > Problem: Subsequent device_add can fail if it reuses the qdev ID or PCI
> > slot, and the unplug has not yet completed (race condition), or it
> > failed. Yes, Virginia, PCI hotplug *can* fail.
> >
> > When unplug succeeds, the qdev is automatically destroyed.
> > pciej_write() does that for PIIX4. Looks like pcie_cap_slot_event()
> > does it for PCIE.
>
> I got a similar problem. When I unplug a pci device by hand, it works
> as expected, and I can hotplug it again. But when I use a srcipt to
> do the same thing, sometimes it failed. I think I may find another bug.
>
> Steps to reproduce this bug:
> 1. cat ./test-e1000.sh # RHEL6RC is domain name
> #! /bin/bash
>
> while true; do
> virsh attach-interface RHEL6RC network default --mac
> 52:54:00:1f:db:c7 --model e1000
> if [[ $? -ne 0 ]]; then
> break
> fi
> virsh detach-interface RHEL6RC network --mac 52:54:00:1f:db:c7
> if [[ $? -ne 0 ]]; then
> break
> fi
> sleep 5
> done
>
> 2. ./test-e1000.sh
> Interface attached successfully
>
> Interface detached successfully
>
> Interface attached successfully
>
> Interface detached successfully
>
> error: Failed to attach interface
> error: operation failed: adding
> e1000,netdev=hostnet1,id=net1,mac=52:54:00:1f:db:c7,bus=pci.0,addr=0x8 device
> failed: Duplicate ID 'net1' for device
>
>
> I use ftrace to trace whether sci interrupt is passed to guest OS, here is
> log:
> # cat trace | grep 'irq=9'
> <idle>-0 [000] 118342.634772: irq_handler_entry: irq=9 name=acpi
> <idle>-0 [000] 118342.634831: irq_handler_exit: irq=9
> ret=handled
> <idle>-0 [000] 118342.693216: irq_handler_entry: irq=9 name=acpi
> <idle>-0 [000] 118342.693254: irq_handler_exit: irq=9
> ret=handled
> <idle>-0 [000] 118347.737689: irq_handler_entry: irq=9 name=acpi
> <idle>-0 [000] 118347.737738: irq_handler_exit: irq=9
> ret=handled
> According to the log, we only receive 3 sci interrupt, and one is lost.
>
> I enable piix4's debug and 1 line printf into piix4_device_hotplug:
> printf("slot: %d, up: %d, down: %d, state: %d\n", slot,
> s->pci0_status.up, s->pci0_status.down, state);
> here is the log:
> ========
> PM: mapping to 0xb000
> PM readw port=0x0004 val=0x0000
> ...
> gpe write afe2 <== 0
> gpe write afe0 <== 255
> gpe write afe3 <== 0
> gpe write afe1 <== 255
> PM readw port=0x0000 val=0x0001
> PM readw port=0x0002 val=0x0000
> gpe read afe0 == 0
> gpe read afe2 == 0
> gpe read afe1 == 0
> gpe read afe3 == 0
> PM writew port=0x0000 val=0x0020
> PM readw port=0x0002 val=0x0000
> PM writew port=0x0002 val=0x0020
> PM readw port=0x0002 val=0x0020
> gpe write afe2 <== 255
> gpe write afe3 <== 255
> ...
> slot: 6, up: 0, down: 0, state: 1 <==== we attach interface the first time
> PM readw port=0x0000 val=0x0001
> PM readw port=0x0002 val=0x0120
> gpe read afe0 == 2
> gpe read afe2 == ff
> gpe read afe2 == ff
> gpe write afe2 <== 253
> gpe read afe1 == 0
> gpe read afe3 == ff
> pcihotplug read ae00 == 40
> pcihotplug read ae04 == 0
> ...
> gpe write afe0 <== 2
> gpe write afe2 <== 255
> slot: 6, up: 64, down: 0, state: 0 <===== we detach interface the first time
> PM readw port=0x0000 val=0x0001
> PM readw port=0x0002 val=0x0120
> gpe read afe0 == 2
> gpe read afe2 == ff
> gpe read afe2 == ff
> gpe write afe2 <== 253
> gpe read afe1 == 0
> gpe read afe3 == ff
> pcihotplug read ae00 == 0
> pcihotplug read ae04 == 40
> ...
> pciej write ae08 <== 64 <=== we will call qdev_free()
> pciej write ae08 <== 64
> gpe write afe0 <== 2
> gpe write afe2 <== 255
> slot: 7, up: 0, down: 64, state: 1 <=== we attach interface the second time.
> PM readw port=0x0000 val=0x0001
> PM readw port=0x0002 val=0x0120
> gpe read afe0 == 2
> gpe read afe2 == ff
> gpe read afe2 == ff
> gpe write afe2 <== 253
> gpe read afe1 == 0
> gpe read afe3 == ff
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> pcihotplug read ae00 == 80
> pcihotplug read ae04 == 0
> slot: 7, up: 128, down: 0, state: 0 <=== we detach interface the second time
> gpe write afe0 <== 2 <=== write 2 to afe0 will cause sci interupt to
> be lost
> gpe write afe2 <== 255
> ===========
>
> Here is short log, and show the different between the first time and second
> time:
> ===========
> gpe write afe0 <== 2
> gpe write afe2 <== 255
> slot: 6, up: 64, down: 0, state: 0 <===== we detach interface the first time
> ....
> slot: 7, up: 128, down: 0, state: 0 <=== we detach interface the second time
> gpe write afe0 <== 2 <=== write 2 to afe0 will cause sci interupt to
> be lost
> gpe write afe2 <== 255
>
> ===========
>
> Does this behavior is a normal behavior?
>
> The following patch can fix this problem.
>
> From 3c149575b52261b8da9a73d5c4ebdfedce9866c1 Mon Sep 17 00:00:00 2001
> From: Wen Congyang <address@hidden>
> Date: Mon, 28 Feb 2011 10:33:45 +0800
> Subject: [PATCH] pend hotplug event until last event is handled by guest OS
>
> ---
> hw/acpi_piix4.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
> 1 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index b5a2762..4ff3475 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -74,6 +74,7 @@ typedef struct PIIX4PMState {
> /* for pci hotplug */
> struct gpe_regs gpe;
> struct pci_status pci0_status;
> + struct pci_status pci0_status_pending;
> uint32_t pci0_hotplug_enable;
> } PIIX4PMState;
>
> @@ -518,6 +519,19 @@ static void gpe_reset_val(uint16_t *cur, int addr,
> uint32_t val)
> *cur = (*cur & (0xff << (8 - shift))) | (x1 << shift);
> }
>
> +static void raise_pending_hotplug(PIIX4PMState *s)
> +{
> + if (s->pci0_status_pending.up || s->pci0_status_pending.down) {
> + s->gpe.sts |= PIIX4_PCI_HOTPLUG_STATUS;
> + s->pci0_status.up = s->pci0_status_pending.up;
> + s->pci0_status.down = s->pci0_status_pending.down;
> + s->pci0_status_pending.up = 0;
> + s->pci0_status_pending.down = 0;
> +
> + pm_update_sci(s);
> + }
> +}
> +
> static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
> {
> PIIX4PMState *s = opaque;
> @@ -539,6 +553,11 @@ static void gpe_writeb(void *opaque, uint32_t addr,
> uint32_t val)
> pm_update_sci(s);
>
> PIIX4_DPRINTF("gpe write %x <== %d\n", addr, val);
> +
> + if (!(g->sts & PIIX4_PCI_HOTPLUG_STATUS) && g->en &
> PIIX4_PCI_HOTPLUG_STATUS) {
> + /* check and reraise the pending hotplug event */
> + raise_pending_hotplug(s);
> + }
> }
>
> static uint32_t pcihotplug_read(void *opaque, uint32_t addr)
> @@ -639,12 +658,22 @@ static void enable_device(PIIX4PMState *s, int slot)
> s->pci0_status.up |= (1 << slot);
> }
>
> +static void pend_enable_device(PIIX4PMState *s, int slot)
> +{
> + s->pci0_status_pending.up |= (1 << slot);
> +}
> +
> static void disable_device(PIIX4PMState *s, int slot)
> {
> s->gpe.sts |= PIIX4_PCI_HOTPLUG_STATUS;
> s->pci0_status.down |= (1 << slot);
> }
>
> +static void pend_disable_device(PIIX4PMState *s, int slot)
> +{
> + s->pci0_status_pending.down |= (1 << slot);
> +}
> +
> static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> PCIHotplugState state)
> {
> @@ -659,15 +688,23 @@ static int piix4_device_hotplug(DeviceState *qdev,
> PCIDevice *dev,
> return 0;
> }
>
> - s->pci0_status.up = 0;
> - s->pci0_status.down = 0;
> - if (state == PCI_HOTPLUG_ENABLED) {
> - enable_device(s, slot);
> + if (s->gpe.sts & PIIX4_PCI_HOTPLUG_STATUS) {
> + if (state == PCI_HOTPLUG_ENABLED) {
> + pend_enable_device(s, slot);
> + } else {
> + pend_disable_device(s, slot);
> + }
> } else {
> - disable_device(s, slot);
> - }
> + s->pci0_status.up = 0;
> + s->pci0_status.down = 0;
> + if (state == PCI_HOTPLUG_ENABLED) {
> + enable_device(s, slot);
> + } else {
> + disable_device(s, slot);
> + }
>
> - pm_update_sci(s);
> + pm_update_sci(s);
> + }
>
> return 0;
> }
> --
> 1.7.1
>
> >
> > Your patch adds a *second* qdev_free(). No good.
> >
> >
>
>
--
yamahata