From patchwork Thu Apr 12 10:28:42 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 152008 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id EC841B70C2 for ; Thu, 12 Apr 2012 20:29:05 +1000 (EST) Received: from localhost ([::1]:54831 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIHGx-00046y-8U for incoming@patchwork.ozlabs.org; Thu, 12 Apr 2012 06:29:03 -0400 Received: from eggs.gnu.org ([208.118.235.92]:43027) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIHGl-00046p-UW for qemu-devel@nongnu.org; Thu, 12 Apr 2012 06:28:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SIHGf-0004R9-0z for qemu-devel@nongnu.org; Thu, 12 Apr 2012 06:28:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60407) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SIHGe-0004R2-P5 for qemu-devel@nongnu.org; Thu, 12 Apr 2012 06:28:44 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3CASVA4014241 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 12 Apr 2012 06:28:32 -0400 Received: from redhat.com (reserved-201-250.tlv.redhat.com [10.35.201.250] (may be forged)) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id q3CASQQK024370; Thu, 12 Apr 2012 06:28:27 -0400 Date: Thu, 12 Apr 2012 13:28:42 +0300 From: "Michael S. Tsirkin" To: Alex Williamson Message-ID: <20120412102840.GA14321@redhat.com> References: <20120405170331.14105.52652.stgit@bling.home> <20120405170715.14105.7389.stgit@bling.home> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20120405170715.14105.7389.stgit@bling.home> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: aliguori@us.ibm.com, gleb@redhat.com, jbaron@redhat.com, qemu-devel@nongnu.org, yamahata@valinux.co.jp, kraxel@redhat.com, pbonzini@redhat.com, imammedo@redhat.com, aurelien@aurel32.net Subject: Re: [Qemu-devel] [PATCH v2 2/5] acpi_piix4: Fix PCI hotplug race X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Thu, Apr 05, 2012 at 11:07:15AM -0600, Alex Williamson wrote: > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 5e8b261..0e7af51 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -48,7 +48,7 @@ > #define PIIX4_PCI_HOTPLUG_STATUS 2 > > struct pci_status { > - uint32_t up; > + uint32_t up; /* deprecated, maintained for migration compatibility */ > uint32_t down; > }; > > @@ -70,6 +70,7 @@ typedef struct PIIX4PMState { > /* for pci hotplug */ > struct pci_status pci0_status; > uint32_t pci0_hotplug_enable; > + uint32_t pci0_slot_device_present; > } PIIX4PMState; > > static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s); > @@ -205,6 +206,17 @@ static void pm_write_config(PCIDevice *d, > pm_io_space_update((PIIX4PMState *)d); > } > > +static void vmstate_pci_status_pre_save(void *opaque) > +{ > + struct pci_status *pci0_status = opaque; > + PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status); > + > + /* We no longer track up, so build a safe value for migrating > + * to a version that still does... of course these might get lost > + * by an old buggy implementation, but we try. */ > + pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable; > +} > + > static int vmstate_acpi_post_load(void *opaque, int version_id) > { > PIIX4PMState *s = opaque; > @@ -241,6 +253,7 @@ static const VMStateDescription vmstate_pci_status = { > .version_id = 1, > .minimum_version_id = 1, > .minimum_version_id_old = 1, > + .pre_save = vmstate_pci_status_pre_save, > .fields = (VMStateField []) { > VMSTATE_UINT32(up, struct pci_status), > VMSTATE_UINT32(down, struct pci_status), > @@ -269,13 +282,38 @@ static const VMStateDescription vmstate_acpi = { > } > }; > > +static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) > +{ > + DeviceState *qdev, *next; > + BusState *bus = qdev_get_parent_bus(&s->dev.qdev); > + int slot = ffs(slots) - 1; > + > + /* Mark request as complete */ > + s->pci0_status.down &= ~(1U << slot); > + > + QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) { > + PCIDevice *dev = PCI_DEVICE(qdev); > + PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > + if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) { > + s->pci0_slot_device_present &= ~(1U << slot); > + qdev_free(qdev); > + } > + } > +} > + One small thing that bothers me is how slot bit is cleared apparently for each device. Hotplug and non hotplug devices should not mix normally, and we only set the bit when we add a device so it should all work out, but still, I think a bit better and more robust if the bit will get cleared unless a device is left in the slot. So I'm thinking of applying this on top: any objections? Warning: untested. Signed-off-by: Michael S. Tsirkin Acked-by: Alex Williamson diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 11c1f85..585da4e 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -287,6 +287,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) DeviceState *qdev, *next; BusState *bus = qdev_get_parent_bus(&s->dev.qdev); int slot = ffs(slots) - 1; + bool slot_free = true; /* Mark request as complete */ s->pci0_status.down &= ~(1U << slot); @@ -294,11 +295,17 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) { PCIDevice *dev = PCI_DEVICE(qdev); PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); - if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) { - s->pci0_slot_device_present &= ~(1U << slot); - qdev_free(qdev); + if (PCI_SLOT(dev->devfn) == slot) { + if (pc->no_hotplug) { + slot_free = false; + } else { + qdev_free(qdev); + } } } + if (slot_free) { + s->pci0_slot_device_present &= ~(1U << slot); + } } static void piix4_update_hotplug(PIIX4PMState *s)