| Message ID | 20181105014047.26447-20-sameo@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI reorganization for hardware-reduced API addition | expand |
On Mon, 5 Nov 2018 02:40:42 +0100 Samuel Ortiz <sameo@linux.intel.com> wrote: > From: Sebastien Boeuf <sebastien.boeuf@intel.com> > > 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@redhat.com> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> 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. with commit message fixed Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > 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("/machine/i440fx", 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
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@linux.intel.com> wrote: > > > > > From: Sebastien Boeuf <sebastien.boeuf@intel.com> > > > > 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@redhat.com> > > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > > Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> > 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. > > > with commit message fixed > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > > --- > > 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("/machine/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, Sebastien
On Fri, 16 Nov 2018 19:42:08 +0000 "Boeuf, Sebastien" <sebastien.boeuf@intel.com> 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@linux.intel.com> wrote: > > > > > > > > From: Sebastien Boeuf <sebastien.boeuf@intel.com> > > > > > > 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@redhat.com> > > > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > > > Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> > > 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. > > > > with commit message fixed > > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > --- > > > 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("/machine/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, > Sebastien
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@intel.com> 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@linux.intel.com> wrote: > > > > > > > > > > > > > > > From: Sebastien Boeuf <sebastien.boeuf@intel.com> > > > > > > > > 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@redhat.com> > > > > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > > > > Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> > > > 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. > > > > > > > > > > > > with commit message fixed > > > > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > > > > > > --- > > > > 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, > > Sebastien
On Mon, 19 Nov 2018 18:02:53 +0000 "Boeuf, Sebastien" <sebastien.boeuf@intel.com> 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@intel.com> 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@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > From: Sebastien Boeuf <sebastien.boeuf@intel.com> > > > > > > > > > > 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@redhat.com> > > > > > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > > > > > Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> > > > > 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@redhat.com> > > > > > > > > > > > > > > > > > > > --- > > > > > 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
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("/machine/i440fx", 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