Re: [Xen-devel] [PATCH v5 19/24] hw: acpi: Retrieve the PCI bus from AcpiPciHpState

[Igor Mammedov <imammedo@xxxxxxxxxx>]

On Mon, 19 Nov 2018 18:02:53 +0000
"Boeuf, Sebastien" <sebastien.boeuf@xxxxxxxxx> wrote:

> On Mon, 2018-11-19 at 16:37 +0100, Igor Mammedov wrote:
> > On Fri, 16 Nov 2018 19:42:08 +0000
> > "Boeuf, Sebastien" <sebastien.boeuf@xxxxxxxxx> wrote:
> >   
> > > 
> > > Hi Igor,
> > > 
> > > On Fri, 2018-11-16 at 10:39 +0100, Igor Mammedov wrote:  
> > > > 
> > > > On Mon,  5 Nov 2018 02:40:42 +0100
> > > > Samuel Ortiz <sameo@xxxxxxxxxxxxxxx> wrote:
> > > >     
> > > > > 
> > > > > 
> > > > > From: Sebastien Boeuf <sebastien.boeuf@xxxxxxxxx>
> > > > > 
> > > > > Instead of using the machine type specific method find_i440fx()
> > > > > to
> > > > > retrieve the PCI bus, this commit aims to rely on the fact that
> > > > > the
> > > > > PCI bus is known by the structure AcpiPciHpState.
> > > > > 
> > > > > When the structure is initialized through acpi_pcihp_init()
> > > > > call,
> > > > > it saves the PCI bus, which means there is no need to invoke a
> > > > > special function later on.
> > > > > 
> > > > > Based on the fact that find_i440fx() was only used there, this
> > > > > patch also removes the function find_i440fx() itself from the
> > > > > entire codebase.
> > > > > 
> > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> > > > > Tested-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> > > > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@xxxxxxxxx>
> > > > > Signed-off-by: Jing Liu <jing2.liu@xxxxxxxxxxxxxxx>    
> > > > Thanks for cleaning it up
> > > > 
> > > > minor nit:
> > > > Taking in account that you're removing '/* TODO: Q35 support */'
> > > > comment along with find_i440fx(), it might be worth to mention
> > > > in this commit message. Something along lines that ACPI PCIHP
> > > > exist to support guests without SHPC support on PCI
> > > > based PC machine. Considering that Q35 provides native
> > > > PCI-E hotplug, there is no need to add ACPI hotplug there.    
> > > Oh yes sure we can update the commit message :). But just wanted to
> > > mention that 'pc' machine type uses ACPI PCIHP and does support
> > > SHPC, so it's not mutually exclusive.  
> > it supports both but is it relevant to this patch?
> > 
> > Point was that one shouldn't remove something silently without
> > any justification/explanation. So that readers that come later
> > wouldn't wonder about the reasons why the code was removed.
> >   
> 
> I understand the point but I think the comment was wrong in the first
> place since q35 never tried to support ACPI PCIHP, as they support PCIe
> native hotplug as you mentioned.
ok.

when you have something ready, feel free to ping me
(I don't mind to review on github if that helps to speed up process)

> 
> >    
> > >   
> > > > 
> > > > 
> > > > with commit message fixed
> > > > 
> > > > Reviewed-by: Igor Mammedov <imammedo@xxxxxxxxxx>
> > > >     
> > > > > 
> > > > > 
> > > > > ---
> > > > >  include/hw/i386/pc.h  |  1 -
> > > > >  hw/acpi/pcihp.c       | 10 ++++------
> > > > >  hw/pci-host/piix.c    |  8 --------
> > > > >  stubs/pci-host-piix.c |  6 ------
> > > > >  stubs/Makefile.objs   |  1 -
> > > > >  5 files changed, 4 insertions(+), 22 deletions(-)
> > > > >  delete mode 100644 stubs/pci-host-piix.c
> > > > > 
> > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > index 44cb6bf3f3..8e5f1464eb 100644
> > > > > --- a/include/hw/i386/pc.h
> > > > > +++ b/include/hw/i386/pc.h
> > > > > @@ -255,7 +255,6 @@ PCIBus *i440fx_init(const char *host_type,
> > > > > const char *pci_type,
> > > > >                      MemoryRegion *pci_memory,
> > > > >                      MemoryRegion *ram_memory);
> > > > >  
> > > > > -PCIBus *find_i440fx(void);
> > > > >  /* piix4.c */
> > > > >  extern PCIDevice *piix4_dev;
> > > > >  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > index 80d42e12ff..254b2e50ab 100644
> > > > > --- a/hw/acpi/pcihp.c
> > > > > +++ b/hw/acpi/pcihp.c
> > > > > @@ -93,10 +93,9 @@ static void *acpi_set_bsel(PCIBus *bus, void
> > > > > *opaque)
> > > > >      return bsel_alloc;
> > > > >  }
> > > > >  
> > > > > -static void acpi_set_pci_info(void)
> > > > > +static void acpi_set_pci_info(AcpiPciHpState *s)
> > > > >  {
> > > > >      static bool bsel_is_set;
> > > > > -    PCIBus *bus;
> > > > >      unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> > > > >  
> > > > >      if (bsel_is_set) {
> > > > > @@ -104,10 +103,9 @@ static void acpi_set_pci_info(void)
> > > > >      }
> > > > >      bsel_is_set = true;
> > > > >  
> > > > > -    bus = find_i440fx(); /* TODO: Q35 support */
> > > > > -    if (bus) {
> > > > > +    if (s->root) {
> > > > >          /* Scan all PCI buses. Set property to enable acpi
> > > > > based
> > > > > hotplug. */
> > > > > -        pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> > > > > &bsel_alloc);
> > > > > +        pci_for_each_bus_depth_first(s->root, acpi_set_bsel,
> > > > > NULL,
> > > > > &bsel_alloc);
> > > > >      }
> > > > >  }
> > > > >  
> > > > > @@ -213,7 +211,7 @@ static void
> > > > > acpi_pcihp_update(AcpiPciHpState
> > > > > *s)
> > > > >  
> > > > >  void acpi_pcihp_reset(AcpiPciHpState *s)
> > > > >  {
> > > > > -    acpi_set_pci_info();
> > > > > +    acpi_set_pci_info(s);
> > > > >      acpi_pcihp_update(s);
> > > > >  }
> > > > >  
> > > > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > > > index 47293a3915..658460264b 100644
> > > > > --- a/hw/pci-host/piix.c
> > > > > +++ b/hw/pci-host/piix.c
> > > > > @@ -445,14 +445,6 @@ PCIBus *i440fx_init(const char *host_type,
> > > > > const char *pci_type,
> > > > >      return b;
> > > > >  }
> > > > >  
> > > > > -PCIBus *find_i440fx(void)
> > > > > -{
> > > > > -    PCIHostState *s = OBJECT_CHECK(PCIHostState,
> > > > > -                                   object_resolve_path("/machi
> > > > > ne/i
> > > > > 440fx", NULL),
> > > > > -                                   TYPE_PCI_HOST_BRIDGE);
> > > > > -    return s ? s->bus : NULL;
> > > > > -}
> > > > > -
> > > > >  /* PIIX3 PCI to ISA bridge */
> > > > >  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> > > > >  {
> > > > > diff --git a/stubs/pci-host-piix.c b/stubs/pci-host-piix.c
> > > > > deleted file mode 100644
> > > > > index 6ed81b1f21..0000000000
> > > > > --- a/stubs/pci-host-piix.c
> > > > > +++ /dev/null
> > > > > @@ -1,6 +0,0 @@
> > > > > -#include "qemu/osdep.h"
> > > > > -#include "hw/i386/pc.h"
> > > > > -PCIBus *find_i440fx(void)
> > > > > -{
> > > > > -    return NULL;
> > > > > -}
> > > > > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > > > > index 5dd0aeeec6..725f78bedc 100644
> > > > > --- a/stubs/Makefile.objs
> > > > > +++ b/stubs/Makefile.objs
> > > > > @@ -41,6 +41,5 @@ stub-obj-y += pc_madt_cpu_entry.o
> > > > >  stub-obj-y += vmgenid.o
> > > > >  stub-obj-y += xen-common.o
> > > > >  stub-obj-y += xen-hvm.o
> > > > > -stub-obj-y += pci-host-piix.o
> > > > >  stub-obj-y += ram-block.o
> > > > >  stub-obj-y += ramfb.o    
> > > Thanks,
> > > Sebastie  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel