| Message ID | 1390315206-20903-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <imammedo@redhat.com> 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. > --- > 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
On Sun, 26 Jan 2014 12:02:23 +0200 "Michael S. Tsirkin" <mst@redhat.com> 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 <imammedo@redhat.com> > > 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). > > > --- > > 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
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" <mst@redhat.com> 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 <imammedo@redhat.com> > > > > 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
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; }
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 <imammedo@redhat.com> --- hw/acpi/pcihp.c | 10 +++------- 1 files changed, 3 insertions(+), 7 deletions(-)