Re: [Qemu-devel] [PATCH 3/4] hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property

[Michael S. Tsirkin]

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" <address@hidden> 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 <address@hidden>
> > 
> > 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