From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57715) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W7oBa-0003TI-0S for qemu-devel@nongnu.org; Mon, 27 Jan 2014 10:33:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W7oBS-0005fM-4S for qemu-devel@nongnu.org; Mon, 27 Jan 2014 10:33:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56077) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W7oBR-0005fB-P1 for qemu-devel@nongnu.org; Mon, 27 Jan 2014 10:33:09 -0500 Date: Mon, 27 Jan 2014 17:38:00 +0200 From: "Michael S. Tsirkin" Message-ID: <20140127153800.GC12689@redhat.com> References: <1390315206-20903-1-git-send-email-imammedo@redhat.com> <1390315206-20903-4-git-send-email-imammedo@redhat.com> <20140126100223.GA8709@redhat.com> <20140127150112.7bce6d66@nial.usersys.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140127150112.7bce6d66@nial.usersys.redhat.com> Subject: Re: [Qemu-devel] [PATCH 3/4] hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, aliguori@amazon.com On Mon, Jan 27, 2014 at 03:01:12PM +0100, Igor Mammedov wrote: > On Sun, 26 Jan 2014 12:02:23 +0200 > "Michael S. Tsirkin" wrote: > > > On Tue, Jan 21, 2014 at 03:40:05PM +0100, Igor Mammedov wrote: > > > when running with machine types older than 1.7 (i.e. without ACPI > > > builtin tables), PCI bus won't have ACPI_PCIHP_PROP_BSEL property > > > set. > > > Taking in account that acpi hotplug handler in 1.7 and older > > > machines is called only for root PCI bus, to make pcihp code > > > compatible with legacy machine types assume that bus without > > > ACPI_PCIHP_PROP_BSEL property has it equal to 0 and bail out > > > if it's not root bus. > > > > > > Signed-off-by: Igor Mammedov > > > > I think that's not the best way to do this. > > If bsel 0 *is* set on some bus, it should select it. > > Fallback to bus 0 only if bsel value 0 is not set anywhere. > > See e.g. how acpi_pcihp_find_hotplug_bus does it: > > > > if (!bsel && !find.bus) { > > find.bus = s->root; > > } > > > > otherwise we introduce dependency on the logic that sets > > bsel, this makes code fragile. > Or to avoid touching PCIHP code, as an alternative > we can add BSEL property to root bus and set it to 0 when > running in compatibility mode (!use_acpi_pci_hotplug). I didn't think too deeply about it, but on the surface this seems fine too. > > > > > --- > > > hw/acpi/pcihp.c | 10 +++------- > > > 1 files changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > index 6d34fe9..76dce8d 100644 > > > --- a/hw/acpi/pcihp.c > > > +++ b/hw/acpi/pcihp.c > > > @@ -63,14 +63,10 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) > > > { > > > QObject *o = object_property_get_qobject(OBJECT(bus), > > > ACPI_PCIHP_PROP_BSEL, NULL); > > > - int64_t bsel = -1; > > > if (o) { > > > - bsel = qint_get_int(qobject_to_qint(o)); > > > + return qint_get_int(qobject_to_qint(o)); > > > } > > > - if (bsel < 0) { > > > - return -1; > > > - } > > > - return bsel; > > > + return 0; > > > } > > > > > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) > > > @@ -190,7 +186,7 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev, > > > { > > > int slot = PCI_SLOT(dev->devfn); > > > int bsel = acpi_pcihp_get_bsel(dev->bus); > > > - if (bsel < 0) { > > > + if ((bsel == 0) && (dev->bus != s->root)) { > > > return -1; > > > } > > > > > > -- > > > 1.7.1