From patchwork Tue Apr 3 13:59:49 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 150453 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 A0FD2B6FE4 for ; Wed, 4 Apr 2012 00:25:40 +1000 (EST) Received: from localhost ([::1]:49571 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SF4HU-0007gb-1W for incoming@patchwork.ozlabs.org; Tue, 03 Apr 2012 10:00:20 -0400 Received: from eggs.gnu.org ([208.118.235.92]:36687) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SF4HH-0007ZK-KH for qemu-devel@nongnu.org; Tue, 03 Apr 2012 10:00:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SF4H6-0001D6-PK for qemu-devel@nongnu.org; Tue, 03 Apr 2012 10:00:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19206) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SF4H6-0001Cf-Gj for qemu-devel@nongnu.org; Tue, 03 Apr 2012 09:59:56 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q33DxoP7028729 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 3 Apr 2012 09:59:50 -0400 Received: from [10.3.113.138] (ovpn-113-138.phx2.redhat.com [10.3.113.138]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q33DxnNJ025616; Tue, 3 Apr 2012 09:59:49 -0400 Message-ID: <1333461589.3799.183.camel@bling.home> From: Alex Williamson To: "Michael S. Tsirkin" Date: Tue, 03 Apr 2012 07:59:49 -0600 In-Reply-To: <20120403100515.GA9697@redhat.com> References: <1333393194.3799.175.camel@bling.home> <20120403100515.GA9697@redhat.com> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.11 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Anthony Liguori , gleb@redhat.com, qemu-devel@nongnu.org, Isaku Yamahata , Gerd Hoffmann , Paolo Bonzini , Igor Mammedov , Aurelien Jarno Subject: Re: [Qemu-devel] [PATCHv3] piix: fix up/down races 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 Tue, 2012-04-03 at 13:05 +0300, Michael S. Tsirkin wrote: > On Tue, Apr 03, 2012 at 02:40:41AM -0400, Igor Mammedov wrote: > > > > + */ > > > > + s->pci0_status.down &= ~(1U << slot); > > > > > > Should we clear "up" here too? > > Is it possible to create pci dev for not yet freed pci slot? > > It's not possible ATM. > > > If not then we should not care, otherwise we should fix that. > > > > @@ -567,8 +595,6 @@ 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); > > > > } else { > > > > > > So if we have an old bios and do an add, followed by a remove, guest > > > ACPI finds both "up" and "down" set for the slot and we rely on the > > > ordering of the AML checking up before down to keep the duct tape and > > > bailing wire from exploding? > > In fact UP triggers a rescan - there is no injection event in ACPI. > So it seems you can check the events in any order. So, we're not concerned with extra device checks and we can clear "down" on eject. What's then the practical advantage of your patch over: Signed-off-by: Alex Williamson --- ie. Up statically returns all hotplug slots, check them all. Down is cleared on eject and reset. Then there's no bios compatibility issue, no bios update even. Clearing down from PCNT doesn't really buy us anything. Thanks, Alex diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 0c77730..050061c 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -79,6 +79,7 @@ typedef struct PIIX4PMState { } PIIX4PMState; static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s); +static void pciej_write(void *opaque, uint32_t addr, uint32_t val); #define ACPI_ENABLE 0xf1 #define ACPI_DISABLE 0xf0 @@ -291,6 +292,10 @@ static void piix4_update_hotplug(PIIX4PMState *s) s->pci0_hotplug_enable &= ~(1 << slot); } } + + while (s->pci0_status.down) { + pciej_write(s, PCI_BASE + 4, s->pci0_status.down); + } } static void piix4_reset(void *opaque) @@ -470,38 +475,15 @@ static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val) static uint32_t pcihotplug_read(void *opaque, uint32_t addr) { - uint32_t val = 0; + uint32_t val; struct pci_status *g = opaque; - switch (addr) { - case PCI_BASE: - val = g->up; - break; - case PCI_BASE + 4: - val = g->down; - break; - default: - break; - } + + val = g->down; PIIX4_DPRINTF("pcihotplug read %x == %x\n", addr, val); return val; } -static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val) -{ - struct pci_status *g = opaque; - switch (addr) { - case PCI_BASE: - g->up = val; - break; - case PCI_BASE + 4: - g->down = val; - break; - } - - PIIX4_DPRINTF("pcihotplug write %x <== %d\n", addr, val); -} - static uint32_t pciej_read(void *opaque, uint32_t addr) { PIIX4_DPRINTF("pciej read %x\n", addr); @@ -514,6 +496,8 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val) DeviceState *qdev, *next; int slot = ffs(val) - 1; + 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); @@ -560,8 +544,8 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) register_ioport_write(PROC_BASE, 32, 1, gpe_writeb, s); register_ioport_read(PROC_BASE, 32, 1, gpe_readb, s); - register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status); - register_ioport_read(PCI_BASE, 8, 4, pcihotplug_read, pci0_status); + register_ioport_read(PCI_BASE, 4, 4, pcirmv_read, s); + register_ioport_read(PCI_BASE + 4, 4, 4, pcihotplug_read, pci0_status); register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus); register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus); @@ -640,8 +624,6 @@ 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); } else {