* [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support
@ 2016-05-31 17:48 Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 1/7] hw/acpi: remove dead acpi code Marcel Apfelbaum
` (6 more replies)
0 siblings, 7 replies; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
The series adds hotplug support to legacy PCI buses for Q35 machines.
The ACPI hotplug code is emitted if at least one legacy pci-bridge is present.
It is still RFC because:
- the compat code is for 2.6 machines
- it works only for pci-bridge, there are still some issues with i82801b11-bridge
- needs more testing
The current implementation does not include PCIe Root Port support.
Patches 1-4: various acpi pre-requisites fixes
Patch 5: adds pcihp initialization code to Q35
Patch 6: makes acpi pci hotplug generic by handling PCIe cases
Patch 7: Adds pchihp support to ich9.
Thanks,
Marcel
Marcel Apfelbaum (7):
hw/acpi: remove dead acpi code
hw/acpi: simplify dsdt building code
hw/acpi: fix a DSDT table issue when a pxb is present.
hw/apci: fix pcihp io initialization
hw/acpi: prepare pci hotplug IO for ich9
hw/acpi: extend acpi pci hotplug support for pci express
hw/ich9: enable pci acpi hotplug
hw/acpi/ich9.c | 31 +++++++++++++
hw/i386/acpi-build.c | 119 +++++++++++++++++++++++++------------------------
hw/isa/lpc_ich9.c | 12 +++++
include/hw/acpi/ich9.h | 4 ++
include/hw/i386/pc.h | 7 ++-
5 files changed, 114 insertions(+), 59 deletions(-)
--
2.4.3
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH RFC 1/7] hw/acpi: remove dead acpi code
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
@ 2016-05-31 17:48 ` Marcel Apfelbaum
2016-06-17 7:57 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 2/7] hw/acpi: simplify dsdt building code Marcel Apfelbaum
` (5 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
PCST and PCSB operation regions are not used by the Q35 machine.
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/i386/acpi-build.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 279f0d7..9aef25d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2016,16 +2016,6 @@ build_dsdt(GArray *table_data, GArray *linker,
build_piix4_pci0_int(dsdt);
} else {
sb_scope = aml_scope("_SB");
- aml_append(sb_scope,
- aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x0c));
- aml_append(sb_scope,
- aml_operation_region("PCSB", AML_SYSTEM_IO, aml_int(0xae0c), 0x01));
- field = aml_field("PCSB", AML_ANY_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
- aml_append(field, aml_named_field("PCIB", 8));
- aml_append(sb_scope, field);
- aml_append(dsdt, sb_scope);
-
- sb_scope = aml_scope("_SB");
dev = aml_device("PCI0");
aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
--
2.4.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH RFC 2/7] hw/acpi: simplify dsdt building code
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 1/7] hw/acpi: remove dead acpi code Marcel Apfelbaum
@ 2016-05-31 17:48 ` Marcel Apfelbaum
2016-06-17 8:39 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 3/7] hw/acpi: fix a DSDT table issue when a pxb is present Marcel Apfelbaum
` (4 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
Get the pci root-bus once and pass it around.
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/i386/acpi-build.c | 58 +++++++++++++++++++++++-----------------------------
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9aef25d..c6f4afe 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1990,7 +1990,7 @@ build_dsdt(GArray *table_data, GArray *linker,
PCMachineState *pcms = PC_MACHINE(machine);
uint32_t nr_mem = machine->ram_slots;
int root_bus_limit = 0xFF;
- PCIBus *bus = NULL;
+ PCIBus *root_bus = NULL;
int i;
dsdt = init_aml_allocator();
@@ -2033,6 +2033,9 @@ build_dsdt(GArray *table_data, GArray *linker,
build_q35_pci0_int(dsdt);
}
+ root_bus = PC_MACHINE(machine)->bus;
+ assert(root_bus);
+
build_cpu_hotplug_aml(dsdt);
build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
pm->mem_hp_io_len);
@@ -2077,8 +2080,8 @@ build_dsdt(GArray *table_data, GArray *linker,
}
aml_append(dsdt, scope);
- bus = PC_MACHINE(machine)->bus;
- if (bus) {
+ {
+ PCIBus *bus = root_bus;
QLIST_FOREACH(bus, &bus->child, sibling) {
uint8_t bus_num = pci_bus_num(bus);
uint8_t numa_node = pci_bus_numa_node(bus);
@@ -2318,38 +2321,29 @@ build_dsdt(GArray *table_data, GArray *linker,
pm->mem_hp_io_len);
{
- Object *pci_host;
- PCIBus *bus = NULL;
+ Aml *scope = aml_scope("PCI0");
- pci_host = acpi_get_i386_pci_host();
- if (pci_host) {
- bus = PCI_HOST_BRIDGE(pci_host)->bus;
- }
-
- if (bus) {
- Aml *scope = aml_scope("PCI0");
- /* Scan all PCI buses. Generate tables to support hotplug. */
- build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
-
- if (misc->tpm_version != TPM_VERSION_UNSPEC) {
- dev = aml_device("ISA.TPM");
- aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
- aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
- crs = aml_resource_template();
- aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
- TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
- /*
- FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
- Rewrite to take IRQ from TPM device model and
- fix default IRQ value there to use some unused IRQ
- */
- /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
- aml_append(dev, aml_name_decl("_CRS", crs));
- aml_append(scope, dev);
- }
+ /* Scan all PCI buses. Generate tables to support hotplug. */
+ build_append_pci_bus_devices(scope, root_bus,
+ pm->pcihp_bridge_en);
- aml_append(sb_scope, scope);
+ if (misc->tpm_version != TPM_VERSION_UNSPEC) {
+ dev = aml_device("ISA.TPM");
+ aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
+ aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+ crs = aml_resource_template();
+ aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
+ TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
+ /*
+ FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
+ Rewrite to take IRQ from TPM device model and
+ fix default IRQ value there to use some unused IRQ
+ */
+ /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
+ aml_append(dev, aml_name_decl("_CRS", crs));
+ aml_append(scope, dev);
}
+ aml_append(sb_scope, scope);
}
aml_append(dsdt, sb_scope);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH RFC 3/7] hw/acpi: fix a DSDT table issue when a pxb is present.
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 1/7] hw/acpi: remove dead acpi code Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 2/7] hw/acpi: simplify dsdt building code Marcel Apfelbaum
@ 2016-05-31 17:48 ` Marcel Apfelbaum
2016-06-17 8:57 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization Marcel Apfelbaum
` (3 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
PXBs do not support hotplug so they don't have a PCNT function.
Since the PXB's PCI root-bus is a child bus of bus 0, the
build_dsdt code will add a call to the corresponding PCNT function.
Fix this by skipping the PCNT call for the above case.
While at it skip also PCIe child buses.
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/i386/acpi-build.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index c6f4afe..0c329fb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -589,6 +589,10 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
QLIST_FOREACH(sec, &bus->child, sibling) {
int32_t devfn = sec->parent_dev->devfn;
+ if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
+ continue;
+ }
+
aml_append(method, aml_name("^S%.02X.PCNT", devfn));
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
` (2 preceding siblings ...)
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 3/7] hw/acpi: fix a DSDT table issue when a pxb is present Marcel Apfelbaum
@ 2016-05-31 17:48 ` Marcel Apfelbaum
2016-06-17 9:04 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 5/7] hw/acpi: prepare pci hotplug IO for ich9 Marcel Apfelbaum
` (2 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
The pm initialization code assigns the pcihp IO base and length to -1 on error,
but the later code will assume 0 as invalid value.
Fix it initializing the above value to 0 as expected.
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/i386/acpi-build.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0c329fb..2097c4c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -124,17 +124,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
Object *lpc = ich9_lpc_find();
Object *obj = NULL;
QObject *o;
+ int pcihp_io_len, pcihp_io_base;
pm->cpu_hp_io_base = 0;
- pm->pcihp_io_base = 0;
- pm->pcihp_io_len = 0;
if (piix) {
obj = piix;
pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
- pm->pcihp_io_base =
+ pcihp_io_base =
object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
- pm->pcihp_io_len =
+ pcihp_io_len =
object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
+
+ pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
+ pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
}
if (lpc) {
obj = lpc;
--
2.4.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH RFC 5/7] hw/acpi: prepare pci hotplug IO for ich9
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
` (3 preceding siblings ...)
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization Marcel Apfelbaum
@ 2016-05-31 17:48 ` Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug Marcel Apfelbaum
6 siblings, 0 replies; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
Does not have any effect yet since the pcihp is not enabled for ICH9.
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/i386/acpi-build.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2097c4c..8ae3e53 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -130,13 +130,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
if (piix) {
obj = piix;
pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
- pcihp_io_base =
- object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
- pcihp_io_len =
- object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
-
- pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
- pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
}
if (lpc) {
obj = lpc;
@@ -144,6 +137,11 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
}
assert(obj);
+ pcihp_io_base = object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
+ pcihp_io_len = object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
+
+ pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
+ pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
pm->cpu_hp_io_len = ACPI_GPE_PROC_LEN;
pm->mem_hp_io_base = ACPI_MEMORY_HOTPLUG_BASE;
pm->mem_hp_io_len = ACPI_MEMORY_HOTPLUG_IO_LEN;
--
2.4.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
` (4 preceding siblings ...)
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 5/7] hw/acpi: prepare pci hotplug IO for ich9 Marcel Apfelbaum
@ 2016-05-31 17:48 ` Marcel Apfelbaum
2016-06-20 14:11 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug Marcel Apfelbaum
6 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
Emit acpi code for pci hotplug on all PC machines:
- if legacy pci hotpug is enabled (pcihp_bridge_en)
- if there is at least one legacy pci bridge in current system
- for PCI buses only (skip PCIe buses)
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/i386/acpi-build.c | 49 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 8ae3e53..cc51f9e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo *info)
NULL);
}
+static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
+{
+ *(bool *)opaque |= !pci_bus_is_express(bus);
+}
+
#define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is APM_CNT_IOPORT */
static void acpi_align_size(GArray *blob, unsigned align)
@@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
unsigned *bsel_alloc = opaque;
unsigned *bus_bsel;
- if (qbus_is_hotpluggable(BUS(bus))) {
+ if (qbus_is_hotpluggable(BUS(bus)) && !pci_bus_is_express(bus)) {
bus_bsel = g_malloc(sizeof *bus_bsel);
*bus_bsel = (*bsel_alloc)++;
@@ -433,15 +438,12 @@ 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(PCIBus *bus)
{
- PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
unsigned bsel_alloc = 0;
- if (bus) {
- /* Scan all PCI buses. Set property to enable acpi based hotplug. */
- pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
- }
+ /* Scan all PCI buses. Set property to enable acpi based hotplug. */
+ pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
}
static void build_append_pcihp_notify_entry(Aml *method, int slot)
@@ -503,7 +505,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
* In this case they aren't themselves hot-pluggable.
* Hotplugged bridges *are* hot-pluggable.
*/
- bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
+ bridge_in_acpi = pc->is_bridge && !pc->is_express && pcihp_bridge_en &&
!DEVICE(pdev)->hotplugged;
hotplug_enabled_dev = bsel && dc->hotpluggable && !bridge_in_acpi;
@@ -1892,7 +1894,7 @@ static void build_piix4_isa_bridge(Aml *table)
aml_append(table, scope);
}
-static void build_piix4_pci_hotplug(Aml *table)
+static void build_acpi_pci_hotplug(Aml *table)
{
Aml *scope;
Aml *field;
@@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray *linker,
uint32_t nr_mem = machine->ram_slots;
int root_bus_limit = 0xFF;
PCIBus *root_bus = NULL;
+ bool has_pci_bus = false;
int i;
dsdt = init_aml_allocator();
@@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray *linker,
build_piix4_pm(dsdt);
build_piix4_isa_bridge(dsdt);
build_isa_devices_aml(dsdt);
- build_piix4_pci_hotplug(dsdt);
build_piix4_pci0_int(dsdt);
} else {
sb_scope = aml_scope("_SB");
@@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray *linker,
root_bus = PC_MACHINE(machine)->bus;
assert(root_bus);
+ pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
+ if (pm->pcihp_bridge_en && has_pci_bus) {
+ build_acpi_pci_hotplug(dsdt);
+ }
+
build_cpu_hotplug_aml(dsdt);
build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
pm->mem_hp_io_len);
@@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray *linker,
aml_append(scope, aml_method("_L00", 0, AML_NOTSERIALIZED));
- if (misc->is_piix4) {
+ if (pm->pcihp_bridge_en && has_pci_bus) {
method = aml_method("_E01", 0, AML_NOTSERIALIZED);
aml_append(method,
aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
@@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data, GArray *linker,
g_ptr_array_free(mem_ranges, true);
/* reserve PCIHP resources */
- if (pm->pcihp_io_len) {
+ if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
dev = aml_device("PHPR");
aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
aml_append(dev,
@@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray *linker,
{
Aml *scope = aml_scope("PCI0");
+ bool scope_used = false;
- /* Scan all PCI buses. Generate tables to support hotplug. */
- build_append_pci_bus_devices(scope, root_bus,
- pm->pcihp_bridge_en);
+ if (pm->pcihp_bridge_en && has_pci_bus) {
+ /* Scan all PCI buses. Generate tables to support hotplug. */
+ build_append_pci_bus_devices(scope, root_bus,
+ pm->pcihp_bridge_en);
+ scope_used = true;
+ }
if (misc->tpm_version != TPM_VERSION_UNSPEC) {
dev = aml_device("ISA.TPM");
@@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray *linker,
/* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
aml_append(dev, aml_name_decl("_CRS", crs));
aml_append(scope, dev);
+ scope_used = true;
+ }
+
+ if (scope_used) {
+ aml_append(sb_scope, scope);
}
- aml_append(sb_scope, scope);
}
aml_append(dsdt, sb_scope);
}
@@ -2870,7 +2885,7 @@ void acpi_setup(void)
build_state = g_malloc0(sizeof *build_state);
- acpi_set_pci_info();
+ acpi_set_pci_info(pcms->bus);
acpi_build_tables_init(&tables);
acpi_build(&tables, MACHINE(pcms));
--
2.4.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
` (5 preceding siblings ...)
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express Marcel Apfelbaum
@ 2016-05-31 17:48 ` Marcel Apfelbaum
2016-06-20 14:27 ` Igor Mammedov
6 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-05-31 17:48 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, mst, imammedo, pbonzini, ehabkost
Re-use the pci acpi hotplug code and enable it only for
the new machines using the 'acpi-pci-hotplug-with-bridge-support'
compat property.
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/acpi/ich9.c | 31 +++++++++++++++++++++++++++++++
hw/isa/lpc_ich9.c | 12 ++++++++++++
include/hw/acpi/ich9.h | 4 ++++
include/hw/i386/pc.h | 7 ++++++-
4 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 27e978f..499f744 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -230,6 +230,7 @@ static void pm_reset(void *opaque)
}
pm->smi_en_wmask = ~0;
+ acpi_pcihp_reset(&pm->acpi_pci_hotplug);
acpi_update_sci(&pm->acpi_regs, pm->irq);
}
@@ -273,6 +274,10 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
pm->powerdown_notifier.notify = pm_powerdown_req;
qemu_register_powerdown_notifier(&pm->powerdown_notifier);
+ acpi_pcihp_init(OBJECT(lpc_pci), &pm->acpi_pci_hotplug, lpc_pci->bus,
+ pci_address_space_io(lpc_pci),
+ pm->use_acpi_pci_hotplug);
+
acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
&pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
@@ -306,6 +311,21 @@ static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
s->pm.acpi_memory_hotplug.is_enabled = value;
}
+static bool ich9_pm_get_acpi_pci_hotplug_support(Object *obj, Error **errp)
+{
+ ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+ return s->pm.use_acpi_pci_hotplug;
+}
+
+static void ich9_pm_set_acpi_pci_hotplug_support(Object *obj, bool value,
+ Error **errp)
+{
+ ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+ s->pm.use_acpi_pci_hotplug = value;
+}
+
static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -397,6 +417,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
{
static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
pm->acpi_memory_hotplug.is_enabled = true;
+ pm->use_acpi_pci_hotplug = true;
pm->disable_s3 = 0;
pm->disable_s4 = 0;
pm->s4_val = 2;
@@ -412,6 +433,10 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
ich9_pm_get_memory_hotplug_support,
ich9_pm_set_memory_hotplug_support,
NULL);
+ object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
+ ich9_pm_get_acpi_pci_hotplug_support,
+ ich9_pm_set_acpi_pci_hotplug_support,
+ NULL);
object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
ich9_pm_get_disable_s3,
ich9_pm_set_disable_s3,
@@ -436,6 +461,9 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, &pm->acpi_memory_hotplug,
dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+ acpi_pcihp_device_plug_cb(&pm->acpi_regs, pm->irq,
+ &pm->acpi_pci_hotplug, dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, &pm->gpe_cpu, dev, errp);
} else {
@@ -451,6 +479,9 @@ void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
&pm->acpi_memory_hotplug, dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+ acpi_pcihp_device_unplug_cb(&pm->acpi_regs, pm->irq,
+ &pm->acpi_pci_hotplug, dev, errp);
} else {
error_setg(errp, "acpi: device unplug request for not supported device"
" type: %s", object_get_typename(OBJECT(dev)));
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 4f8ca45..fb2ce08 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -33,6 +33,7 @@
#include "hw/hw.h"
#include "qapi/visitor.h"
#include "qemu/range.h"
+#include "qapi/error.h"
#include "hw/isa/isa.h"
#include "hw/sysbus.h"
#include "hw/i386/pc.h"
@@ -511,6 +512,15 @@ static const MemoryRegionOps rcrb_mmio_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static void ich9_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
+{
+ ICH9LPCState *s = opaque;
+
+ if (!pci_bus_is_express(pci_bus)) {
+ qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
+ }
+}
+
static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
{
ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready);
@@ -534,6 +544,8 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
/* floppy */
pci_conf[0x82] |= 0x08;
}
+
+ pci_for_each_bus(s->d.bus, ich9_update_bus_hotplug, s);
}
/* reset control */
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 63fa198..c30c125 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -22,6 +22,7 @@
#define HW_ACPI_ICH9_H
#include "hw/acpi/acpi.h"
+#include "hw/acpi/pcihp.h"
#include "hw/acpi/cpu_hotplug.h"
#include "hw/acpi/memory_hotplug.h"
#include "hw/acpi/acpi_dev_interface.h"
@@ -52,6 +53,9 @@ typedef struct ICH9LPCPMRegs {
MemHotplugState acpi_memory_hotplug;
+ AcpiPciHpState acpi_pci_hotplug;
+ bool use_acpi_pci_hotplug;
+
uint8_t disable_s3;
uint8_t disable_s4;
uint8_t s4_val;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9ca2309..ee941b1 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -357,7 +357,12 @@ int e820_get_num_entries(void);
bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
#define PC_COMPAT_2_5 \
- HW_COMPAT_2_5
+ HW_COMPAT_2_5 \
+ {\
+ .driver = "ICH9-LPC",\
+ .property = "acpi-pci-hotplug-with-bridge-support",\
+ .value = "off",\
+ },
/* Helper for setting model-id for CPU models that changed model-id
* depending on QEMU versions up to QEMU 2.4.
--
2.4.3
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/7] hw/acpi: remove dead acpi code
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 1/7] hw/acpi: remove dead acpi code Marcel Apfelbaum
@ 2016-06-17 7:57 ` Igor Mammedov
0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2016-06-17 7:57 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 31 May 2016 20:48:32 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> PCST and PCSB operation regions are not used by the Q35 machine.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/i386/acpi-build.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 279f0d7..9aef25d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2016,16 +2016,6 @@ build_dsdt(GArray *table_data, GArray *linker,
> build_piix4_pci0_int(dsdt);
> } else {
> sb_scope = aml_scope("_SB");
> - aml_append(sb_scope,
> - aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x0c));
> - aml_append(sb_scope,
> - aml_operation_region("PCSB", AML_SYSTEM_IO, aml_int(0xae0c), 0x01));
> - field = aml_field("PCSB", AML_ANY_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> - aml_append(field, aml_named_field("PCIB", 8));
> - aml_append(sb_scope, field);
> - aml_append(dsdt, sb_scope);
> -
> - sb_scope = aml_scope("_SB");
> dev = aml_device("PCI0");
> aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 2/7] hw/acpi: simplify dsdt building code
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 2/7] hw/acpi: simplify dsdt building code Marcel Apfelbaum
@ 2016-06-17 8:39 ` Igor Mammedov
0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2016-06-17 8:39 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 31 May 2016 20:48:33 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> Get the pci root-bus once and pass it around.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> hw/i386/acpi-build.c | 58 +++++++++++++++++++++++-----------------------------
> 1 file changed, 26 insertions(+), 32 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9aef25d..c6f4afe 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1990,7 +1990,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> PCMachineState *pcms = PC_MACHINE(machine);
> uint32_t nr_mem = machine->ram_slots;
> int root_bus_limit = 0xFF;
> - PCIBus *bus = NULL;
> + PCIBus *root_bus = NULL;
> int i;
>
> dsdt = init_aml_allocator();
> @@ -2033,6 +2033,9 @@ build_dsdt(GArray *table_data, GArray *linker,
> build_q35_pci0_int(dsdt);
> }
>
> + root_bus = PC_MACHINE(machine)->bus;
> + assert(root_bus);
nit:
I'd put it in place of removed
bus = PC_MACHINE(machine)->bus;
in the next hunk
> +
> build_cpu_hotplug_aml(dsdt);
> build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
> pm->mem_hp_io_len);
> @@ -2077,8 +2080,8 @@ build_dsdt(GArray *table_data, GArray *linker,
> }
> aml_append(dsdt, scope);
>
> - bus = PC_MACHINE(machine)->bus;
> - if (bus) {
> + {
> + PCIBus *bus = root_bus;
> QLIST_FOREACH(bus, &bus->child, sibling) {
> uint8_t bus_num = pci_bus_num(bus);
> uint8_t numa_node = pci_bus_numa_node(bus);
> @@ -2318,38 +2321,29 @@ build_dsdt(GArray *table_data, GArray *linker,
> pm->mem_hp_io_len);
>
> {
> - Object *pci_host;
> - PCIBus *bus = NULL;
> + Aml *scope = aml_scope("PCI0");
>
> - pci_host = acpi_get_i386_pci_host();
perhaps it's worth to replace acpi_get_i386_pci_host() usage
in other places with PC_MACHINE(machine)->bus as well and remove
acpi_get_i386_pci_host()
> - if (pci_host) {
> - bus = PCI_HOST_BRIDGE(pci_host)->bus;
> - }
> -
> - if (bus) {
> - Aml *scope = aml_scope("PCI0");
> - /* Scan all PCI buses. Generate tables to support hotplug. */
> - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
> -
> - if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> - dev = aml_device("ISA.TPM");
> - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
> - aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> - crs = aml_resource_template();
> - aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> - TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> - /*
> - FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
> - Rewrite to take IRQ from TPM device model and
> - fix default IRQ value there to use some unused IRQ
> - */
> - /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> - aml_append(dev, aml_name_decl("_CRS", crs));
> - aml_append(scope, dev);
> - }
> + /* Scan all PCI buses. Generate tables to support hotplug. */
> + build_append_pci_bus_devices(scope, root_bus,
> + pm->pcihp_bridge_en);
no need for line break here, it all fits in 80 column limit
>
> - aml_append(sb_scope, scope);
> + if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> + dev = aml_device("ISA.TPM");
> + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
> + aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> + crs = aml_resource_template();
> + aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> + TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> + /*
> + FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
> + Rewrite to take IRQ from TPM device model and
> + fix default IRQ value there to use some unused IRQ
> + */
> + /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> + aml_append(dev, aml_name_decl("_CRS", crs));
> + aml_append(scope, dev);
> }
> + aml_append(sb_scope, scope);
> }
> aml_append(dsdt, sb_scope);
> }
Otherwise looks good,
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/7] hw/acpi: fix a DSDT table issue when a pxb is present.
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 3/7] hw/acpi: fix a DSDT table issue when a pxb is present Marcel Apfelbaum
@ 2016-06-17 8:57 ` Igor Mammedov
2016-06-21 8:50 ` Marcel Apfelbaum
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2016-06-17 8:57 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 31 May 2016 20:48:34 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> PXBs do not support hotplug so they don't have a PCNT function.
> Since the PXB's PCI root-bus is a child bus of bus 0, the
> build_dsdt code will add a call to the corresponding PCNT function.
>
> Fix this by skipping the PCNT call for the above case.
> While at it skip also PCIe child buses.
I'd really like to have PXB testcase bios-tables-test before this patch
so it would be easy to see what is being fixed.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> hw/i386/acpi-build.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index c6f4afe..0c329fb 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -589,6 +589,10 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> QLIST_FOREACH(sec, &bus->child, sibling) {
> int32_t devfn = sec->parent_dev->devfn;
>
> + if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> + continue;
> + }
> +
> aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> }
> }
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization Marcel Apfelbaum
@ 2016-06-17 9:04 ` Igor Mammedov
2016-06-21 8:57 ` Marcel Apfelbaum
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2016-06-17 9:04 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 31 May 2016 20:48:35 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> The pm initialization code assigns the pcihp IO base and length to -1 on error,
> but the later code will assume 0 as invalid value.
>
> Fix it initializing the above value to 0 as expected.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> hw/i386/acpi-build.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0c329fb..2097c4c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -124,17 +124,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> Object *lpc = ich9_lpc_find();
> Object *obj = NULL;
> QObject *o;
> + int pcihp_io_len, pcihp_io_base;
>
> pm->cpu_hp_io_base = 0;
> - pm->pcihp_io_base = 0;
> - pm->pcihp_io_len = 0;
this introduces uninitialized memory access in q35 case
> if (piix) {
> obj = piix;
> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> - pm->pcihp_io_base =
> + pcihp_io_base =
> object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> - pm->pcihp_io_len =
> + pcihp_io_len =
> object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> +
> + pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
> + pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
> }
> if (lpc) {
> obj = lpc;
how about something like that:
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4f9aec6..d753e25 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -125,7 +125,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
pm->cpu_hp_io_base = 0;
pm->pcihp_io_base = 0;
- pm->pcihp_io_len = 0;
+ pm->pcihp_io_len = UINT16_MAX;
if (piix) {
obj = piix;
pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
@@ -2053,7 +2053,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
g_ptr_array_free(mem_ranges, true);
/* reserve PCIHP resources */
- if (pm->pcihp_io_len) {
+ if (pm->pcihp_io_len != UINT16_MAX) {
dev = aml_device("PHPR");
aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
aml_append(dev,
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express Marcel Apfelbaum
@ 2016-06-20 14:11 ` Igor Mammedov
2016-06-21 9:05 ` Marcel Apfelbaum
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2016-06-20 14:11 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 31 May 2016 20:48:37 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
subj doesn't match patch, it does opposite i.e. makes sure that
pcie isn't counted in when building acpi hotplug aml
> Emit acpi code for pci hotplug on all PC machines:
> - if legacy pci hotpug is enabled (pcihp_bridge_en)
> - if there is at least one legacy pci bridge in current system
> - for PCI buses only (skip PCIe buses)
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> hw/i386/acpi-build.c | 49
> ++++++++++++++++++++++++++++++++----------------- 1 file changed, 32
> insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 8ae3e53..cc51f9e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo *info)
> NULL);
> }
>
> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
> +{
> + *(bool *)opaque |= !pci_bus_is_express(bus);
> +}
> +
> #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is
> APM_CNT_IOPORT */
> static void acpi_align_size(GArray *blob, unsigned align)
> @@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void
> *opaque) unsigned *bsel_alloc = opaque;
> unsigned *bus_bsel;
>
> - if (qbus_is_hotpluggable(BUS(bus))) {
> + if (qbus_is_hotpluggable(BUS(bus)) && !pci_bus_is_express(bus)) {
> bus_bsel = g_malloc(sizeof *bus_bsel);
>
> *bus_bsel = (*bsel_alloc)++;
> @@ -433,15 +438,12 @@ 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(PCIBus *bus)
> {
> - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> unsigned bsel_alloc = 0;
>
> - if (bus) {
> - /* Scan all PCI buses. Set property to enable acpi based
> hotplug. */
> - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> &bsel_alloc);
> - }
> + /* Scan all PCI buses. Set property to enable acpi based
> hotplug. */
> + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> &bsel_alloc); }
>
> static void build_append_pcihp_notify_entry(Aml *method, int slot)
> @@ -503,7 +505,7 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> * In this case they aren't themselves hot-pluggable.
> * Hotplugged bridges *are* hot-pluggable.
> */
> - bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
> + bridge_in_acpi = pc->is_bridge && !pc->is_express &&
> pcihp_bridge_en && !DEVICE(pdev)->hotplugged;
>
> hotplug_enabled_dev = bsel && dc->hotpluggable
> && !bridge_in_acpi; @@ -1892,7 +1894,7 @@ static void
> build_piix4_isa_bridge(Aml *table) aml_append(table, scope);
> }
>
> -static void build_piix4_pci_hotplug(Aml *table)
> +static void build_acpi_pci_hotplug(Aml *table)
> {
> Aml *scope;
> Aml *field;
> @@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> uint32_t nr_mem = machine->ram_slots;
> int root_bus_limit = 0xFF;
> PCIBus *root_bus = NULL;
> + bool has_pci_bus = false;
> int i;
>
> dsdt = init_aml_allocator();
> @@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray *linker,
> build_piix4_pm(dsdt);
> build_piix4_isa_bridge(dsdt);
> build_isa_devices_aml(dsdt);
> - build_piix4_pci_hotplug(dsdt);
> build_piix4_pci0_int(dsdt);
> } else {
> sb_scope = aml_scope("_SB");
> @@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray *linker,
> root_bus = PC_MACHINE(machine)->bus;
> assert(root_bus);
>
> + pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
> + if (pm->pcihp_bridge_en && has_pci_bus) {
^^^^^ wouldn't that forbid acpi hotplug on PCI0 is it's not
set?
> + build_acpi_pci_hotplug(dsdt);
> + }
> +
> build_cpu_hotplug_aml(dsdt);
> build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
> pm->mem_hp_io_len);
> @@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>
> aml_append(scope, aml_method("_L00", 0, AML_NOTSERIALIZED));
>
> - if (misc->is_piix4) {
> + if (pm->pcihp_bridge_en && has_pci_bus) {
same question as above
> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> aml_append(method,
> aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
> @@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> g_ptr_array_free(mem_ranges, true);
>
> /* reserve PCIHP resources */
> - if (pm->pcihp_io_len) {
> + if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
same question as above
> dev = aml_device("PHPR");
> aml_append(dev, aml_name_decl("_HID",
> aml_string("PNP0A06"))); aml_append(dev,
> @@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray *linker,
>
> {
> Aml *scope = aml_scope("PCI0");
> + bool scope_used = false;
Could you explain why ^^^ is needed?
>
> - /* Scan all PCI buses. Generate tables to support
> hotplug. */
> - build_append_pci_bus_devices(scope, root_bus,
> - pm->pcihp_bridge_en);
> + if (pm->pcihp_bridge_en && has_pci_bus) {
> + /* Scan all PCI buses. Generate tables to support
> hotplug. */
> + build_append_pci_bus_devices(scope, root_bus,
> + pm->pcihp_bridge_en);
> + scope_used = true;
> + }
>
> if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> dev = aml_device("ISA.TPM");
> @@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray *linker,
> /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> aml_append(dev, aml_name_decl("_CRS", crs));
> aml_append(scope, dev);
> + scope_used = true;
> + }
> +
> + if (scope_used) {
> + aml_append(sb_scope, scope);
> }
> - aml_append(sb_scope, scope);
> }
> aml_append(dsdt, sb_scope);
> }
> @@ -2870,7 +2885,7 @@ void acpi_setup(void)
>
> build_state = g_malloc0(sizeof *build_state);
>
> - acpi_set_pci_info();
> + acpi_set_pci_info(pcms->bus);
>
> acpi_build_tables_init(&tables);
> acpi_build(&tables, MACHINE(pcms));
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug Marcel Apfelbaum
@ 2016-06-20 14:27 ` Igor Mammedov
2016-06-21 9:06 ` Marcel Apfelbaum
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2016-06-20 14:27 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 31 May 2016 20:48:38 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> Re-use the pci acpi hotplug code and enable it only for
> the new machines using the 'acpi-pci-hotplug-with-bridge-support'
> compat property.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> hw/acpi/ich9.c | 31 +++++++++++++++++++++++++++++++
> hw/isa/lpc_ich9.c | 12 ++++++++++++
> include/hw/acpi/ich9.h | 4 ++++
> include/hw/i386/pc.h | 7 ++++++-
> 4 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 27e978f..499f744 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -230,6 +230,7 @@ static void pm_reset(void *opaque)
> }
> pm->smi_en_wmask = ~0;
>
> + acpi_pcihp_reset(&pm->acpi_pci_hotplug);
> acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
>
> @@ -273,6 +274,10 @@ void ich9_pm_init(PCIDevice *lpc_pci,
> ICH9LPCPMRegs *pm, pm->powerdown_notifier.notify = pm_powerdown_req;
> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>
> + acpi_pcihp_init(OBJECT(lpc_pci), &pm->acpi_pci_hotplug,
> lpc_pci->bus,
> + pci_address_space_io(lpc_pci),
> + pm->use_acpi_pci_hotplug);
this will register IO window quite high for q35 at ACPI_PCIHP_ADDR
0xae00
maybe it should be different for q35 like with CPU hotplug,
for example below 0xcd8
> +
> acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>
> @@ -306,6 +311,21 @@ static void
> ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
> s->pm.acpi_memory_hotplug.is_enabled = value; }
>
> +static bool ich9_pm_get_acpi_pci_hotplug_support(Object *obj, Error
> **errp) +{
> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> + return s->pm.use_acpi_pci_hotplug;
> +}
> +
> +static void ich9_pm_set_acpi_pci_hotplug_support(Object *obj, bool
> value,
> + Error **errp)
> +{
> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> + s->pm.use_acpi_pci_hotplug = value;
> +}
> +
> static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const
> char *name, void *opaque, Error **errp)
> {
> @@ -397,6 +417,7 @@ void ich9_pm_add_properties(Object *obj,
> ICH9LPCPMRegs *pm, Error **errp) {
> static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> pm->acpi_memory_hotplug.is_enabled = true;
> + pm->use_acpi_pci_hotplug = true;
> pm->disable_s3 = 0;
> pm->disable_s4 = 0;
> pm->s4_val = 2;
> @@ -412,6 +433,10 @@ void ich9_pm_add_properties(Object *obj,
> ICH9LPCPMRegs *pm, Error **errp) ich9_pm_get_memory_hotplug_support,
> ich9_pm_set_memory_hotplug_support,
> NULL);
> + object_property_add_bool(obj,
> "acpi-pci-hotplug-with-bridge-support",
> + ich9_pm_get_acpi_pci_hotplug_support,
> + ich9_pm_set_acpi_pci_hotplug_support,
> + NULL);
> object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
> ich9_pm_get_disable_s3,
> ich9_pm_set_disable_s3,
> @@ -436,6 +461,9 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm,
> DeviceState *dev, Error **errp) object_dynamic_cast(OBJECT(dev),
> TYPE_PC_DIMM)) { acpi_memory_plug_cb(&pm->acpi_regs, pm->irq,
> &pm->acpi_memory_hotplug, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_plug_cb(&pm->acpi_regs, pm->irq,
> + &pm->acpi_pci_hotplug, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, &pm->gpe_cpu, dev,
> errp); } else {
> @@ -451,6 +479,9 @@ void
> ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
> object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))
> { acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
> &pm->acpi_memory_hotplug, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_unplug_cb(&pm->acpi_regs, pm->irq,
> + &pm->acpi_pci_hotplug, dev,
> errp); } else {
> error_setg(errp, "acpi: device unplug request for not
> supported device" " type: %s", object_get_typename(OBJECT(dev)));
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 4f8ca45..fb2ce08 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -33,6 +33,7 @@
> #include "hw/hw.h"
> #include "qapi/visitor.h"
> #include "qemu/range.h"
> +#include "qapi/error.h"
> #include "hw/isa/isa.h"
> #include "hw/sysbus.h"
> #include "hw/i386/pc.h"
> @@ -511,6 +512,15 @@ static const MemoryRegionOps rcrb_mmio_ops = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> +static void ich9_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
> +{
> + ICH9LPCState *s = opaque;
> +
> + if (!pci_bus_is_express(pci_bus)) {
> + qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s),
> &error_abort);
> + }
> +}
> +
> static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
> {
> ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready);
> @@ -534,6 +544,8 @@ static void ich9_lpc_machine_ready(Notifier *n,
> void *opaque) /* floppy */
> pci_conf[0x82] |= 0x08;
> }
> +
> + pci_for_each_bus(s->d.bus, ich9_update_bus_hotplug, s);
> }
>
> /* reset control */
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 63fa198..c30c125 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -22,6 +22,7 @@
> #define HW_ACPI_ICH9_H
>
> #include "hw/acpi/acpi.h"
> +#include "hw/acpi/pcihp.h"
> #include "hw/acpi/cpu_hotplug.h"
> #include "hw/acpi/memory_hotplug.h"
> #include "hw/acpi/acpi_dev_interface.h"
> @@ -52,6 +53,9 @@ typedef struct ICH9LPCPMRegs {
>
> MemHotplugState acpi_memory_hotplug;
>
> + AcpiPciHpState acpi_pci_hotplug;
> + bool use_acpi_pci_hotplug;
> +
> uint8_t disable_s3;
> uint8_t disable_s4;
> uint8_t s4_val;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 9ca2309..ee941b1 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -357,7 +357,12 @@ int e820_get_num_entries(void);
> bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>
> #define PC_COMPAT_2_5 \
> - HW_COMPAT_2_5
> + HW_COMPAT_2_5 \
should it be 2_6 by now?
> + {\
> + .driver = "ICH9-LPC",\
> + .property = "acpi-pci-hotplug-with-bridge-support",\
> + .value = "off",\
> + },
>
> /* Helper for setting model-id for CPU models that changed model-id
> * depending on QEMU versions up to QEMU 2.4.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/7] hw/acpi: fix a DSDT table issue when a pxb is present.
2016-06-17 8:57 ` Igor Mammedov
@ 2016-06-21 8:50 ` Marcel Apfelbaum
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-06-21 8:50 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, ehabkost
On 06/17/2016 11:57 AM, Igor Mammedov wrote:
> On Tue, 31 May 2016 20:48:34 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> PXBs do not support hotplug so they don't have a PCNT function.
>> Since the PXB's PCI root-bus is a child bus of bus 0, the
>> build_dsdt code will add a call to the corresponding PCNT function.
>>
>> Fix this by skipping the PCNT call for the above case.
>> While at it skip also PCIe child buses.
> I'd really like to have PXB testcase bios-tables-test before this patch
> so it would be easy to see what is being fixed.
>
Sure, I'll add the test.
Thanks,
Marcel
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>> hw/i386/acpi-build.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index c6f4afe..0c329fb 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -589,6 +589,10 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>> QLIST_FOREACH(sec, &bus->child, sibling) {
>> int32_t devfn = sec->parent_dev->devfn;
>>
>> + if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
>> + continue;
>> + }
>> +
>> aml_append(method, aml_name("^S%.02X.PCNT", devfn));
>> }
>> }
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization
2016-06-17 9:04 ` Igor Mammedov
@ 2016-06-21 8:57 ` Marcel Apfelbaum
2016-06-21 11:19 ` Igor Mammedov
0 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-06-21 8:57 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, ehabkost
On 06/17/2016 12:04 PM, Igor Mammedov wrote:
> On Tue, 31 May 2016 20:48:35 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> The pm initialization code assigns the pcihp IO base and length to -1 on error,
>> but the later code will assume 0 as invalid value.
>>
>> Fix it initializing the above value to 0 as expected.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>> hw/i386/acpi-build.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 0c329fb..2097c4c 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -124,17 +124,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>> Object *lpc = ich9_lpc_find();
>> Object *obj = NULL;
>> QObject *o;
>> + int pcihp_io_len, pcihp_io_base;
>>
>> pm->cpu_hp_io_base = 0;
>> - pm->pcihp_io_base = 0;
>> - pm->pcihp_io_len = 0;
> this introduces uninitialized memory access in q35 case
>
How? We are always checking pcihp_io_len/pcihp_io_base.
>> if (piix) {
>> obj = piix;
>> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
>> - pm->pcihp_io_base =
>> + pcihp_io_base =
>> object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
>> - pm->pcihp_io_len =
>> + pcihp_io_len =
>> object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>> +
>> + pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
>> + pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
>> }
>> if (lpc) {
>> obj = lpc;
>
> how about something like that:
Please see the next patch. It is only a temporary measure, the next patch
initialize it right to ACPI_PCIHP_IO_BASE_PROP/ACPI_PCIHP_IO_LEN_PROP.
If the properties are not there, they will be 0, but we are checking this everywhere.
Also I'll prefer pm->pcihp_io_len 0 length to mean "unused" because it is used widely.
If you still think is a real issue, I'll change this, of course.
Thanks,
Marcel
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4f9aec6..d753e25 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -125,7 +125,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>
> pm->cpu_hp_io_base = 0;
> pm->pcihp_io_base = 0;
> - pm->pcihp_io_len = 0;
> + pm->pcihp_io_len = UINT16_MAX;
> if (piix) {
> obj = piix;
> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> @@ -2053,7 +2053,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> g_ptr_array_free(mem_ranges, true);
>
> /* reserve PCIHP resources */
> - if (pm->pcihp_io_len) {
> + if (pm->pcihp_io_len != UINT16_MAX) {
> dev = aml_device("PHPR");
> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
> aml_append(dev,
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express
2016-06-20 14:11 ` Igor Mammedov
@ 2016-06-21 9:05 ` Marcel Apfelbaum
2016-06-21 11:28 ` Igor Mammedov
0 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-06-21 9:05 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, ehabkost
On 06/20/2016 05:11 PM, Igor Mammedov wrote:
> On Tue, 31 May 2016 20:48:37 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
> subj doesn't match patch, it does opposite i.e. makes sure that
> pcie isn't counted in when building acpi hotplug aml
>
Maybe I wasn't clear enough. Until now q35 had no acpi hotplug code.
This patch adds acpi hotplug code for all Q35 PCI buses, but no for pcie buses
That means for pci-pci bridge and dmi-pci bridge.
Maybe I should rename it to hw/acpi: extend acpi pci hotplug support for Q35 pci buses.
>> Emit acpi code for pci hotplug on all PC machines:
>> - if legacy pci hotpug is enabled (pcihp_bridge_en)
>> - if there is at least one legacy pci bridge in current system
>> - for PCI buses only (skip PCIe buses)
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>> hw/i386/acpi-build.c | 49
>> ++++++++++++++++++++++++++++++++----------------- 1 file changed, 32
>> insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 8ae3e53..cc51f9e 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo *info)
>> NULL);
>> }
>>
>> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
>> +{
>> + *(bool *)opaque |= !pci_bus_is_express(bus);
>> +}
>> +
>> #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is
>> APM_CNT_IOPORT */
>> static void acpi_align_size(GArray *blob, unsigned align)
>> @@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void
>> *opaque) unsigned *bsel_alloc = opaque;
>> unsigned *bus_bsel;
>>
>> - if (qbus_is_hotpluggable(BUS(bus))) {
>> + if (qbus_is_hotpluggable(BUS(bus)) && !pci_bus_is_express(bus)) {
>> bus_bsel = g_malloc(sizeof *bus_bsel);
>>
>> *bus_bsel = (*bsel_alloc)++;
>> @@ -433,15 +438,12 @@ 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(PCIBus *bus)
>> {
>> - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
>> unsigned bsel_alloc = 0;
>>
>> - if (bus) {
>> - /* Scan all PCI buses. Set property to enable acpi based
>> hotplug. */
>> - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
>> &bsel_alloc);
>> - }
>> + /* Scan all PCI buses. Set property to enable acpi based
>> hotplug. */
>> + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
>> &bsel_alloc); }
>>
>> static void build_append_pcihp_notify_entry(Aml *method, int slot)
>> @@ -503,7 +505,7 @@ static void build_append_pci_bus_devices(Aml
>> *parent_scope, PCIBus *bus,
>> * In this case they aren't themselves hot-pluggable.
>> * Hotplugged bridges *are* hot-pluggable.
>> */
>> - bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
>> + bridge_in_acpi = pc->is_bridge && !pc->is_express &&
>> pcihp_bridge_en && !DEVICE(pdev)->hotplugged;
>>
>> hotplug_enabled_dev = bsel && dc->hotpluggable
>> && !bridge_in_acpi; @@ -1892,7 +1894,7 @@ static void
>> build_piix4_isa_bridge(Aml *table) aml_append(table, scope);
>> }
>>
>> -static void build_piix4_pci_hotplug(Aml *table)
>> +static void build_acpi_pci_hotplug(Aml *table)
>> {
>> Aml *scope;
>> Aml *field;
>> @@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>> uint32_t nr_mem = machine->ram_slots;
>> int root_bus_limit = 0xFF;
>> PCIBus *root_bus = NULL;
>> + bool has_pci_bus = false;
>> int i;
>>
>> dsdt = init_aml_allocator();
>> @@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray *linker,
>> build_piix4_pm(dsdt);
>> build_piix4_isa_bridge(dsdt);
>> build_isa_devices_aml(dsdt);
>> - build_piix4_pci_hotplug(dsdt);
>> build_piix4_pci0_int(dsdt);
>> } else {
>> sb_scope = aml_scope("_SB");
>> @@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray *linker,
>> root_bus = PC_MACHINE(machine)->bus;
>> assert(root_bus);
>>
>> + pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
>> + if (pm->pcihp_bridge_en && has_pci_bus) {
> ^^^^^ wouldn't that forbid acpi hotplug on PCI0 is it's not
> set?
Yes, and this is by design. 'pcihp_bridge_en' flag matches 'acpi hotplug'.
I followed the initialization phase, am I wrong? Can you have acpi hotplug
if the bit is off?
>
>> + build_acpi_pci_hotplug(dsdt);
>> + }
>> +
>> build_cpu_hotplug_aml(dsdt);
>> build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
>> pm->mem_hp_io_len);
>> @@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>>
>> aml_append(scope, aml_method("_L00", 0, AML_NOTSERIALIZED));
>>
>> - if (misc->is_piix4) {
>> + if (pm->pcihp_bridge_en && has_pci_bus) {
> same question as above
>
>> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
>> aml_append(method,
>> aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
>> @@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>> g_ptr_array_free(mem_ranges, true);
>>
>> /* reserve PCIHP resources */
>> - if (pm->pcihp_io_len) {
>> + if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
> same question as above
>
>> dev = aml_device("PHPR");
>> aml_append(dev, aml_name_decl("_HID",
>> aml_string("PNP0A06"))); aml_append(dev,
>> @@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray *linker,
>>
>> {
>> Aml *scope = aml_scope("PCI0");
>> + bool scope_used = false;
> Could you explain why ^^^ is needed?
>
Sure. If none ACPI code will be generated by the below 'if' statements
we will have an empty PCI0 scope. I wanted to get rid of it.
Thanks,
Marcel
>>
>> - /* Scan all PCI buses. Generate tables to support
>> hotplug. */
>> - build_append_pci_bus_devices(scope, root_bus,
>> - pm->pcihp_bridge_en);
>> + if (pm->pcihp_bridge_en && has_pci_bus) {
>> + /* Scan all PCI buses. Generate tables to support
>> hotplug. */
>> + build_append_pci_bus_devices(scope, root_bus,
>> + pm->pcihp_bridge_en);
>> + scope_used = true;
>> + }
>>
>> if (misc->tpm_version != TPM_VERSION_UNSPEC) {
>> dev = aml_device("ISA.TPM");
>> @@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray *linker,
>> /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
>> aml_append(dev, aml_name_decl("_CRS", crs));
>> aml_append(scope, dev);
>> + scope_used = true;
>> + }
>> +
>> + if (scope_used) {
>> + aml_append(sb_scope, scope);
>> }
>> - aml_append(sb_scope, scope);
>> }
>> aml_append(dsdt, sb_scope);
>> }
>> @@ -2870,7 +2885,7 @@ void acpi_setup(void)
>>
>> build_state = g_malloc0(sizeof *build_state);
>>
>> - acpi_set_pci_info();
>> + acpi_set_pci_info(pcms->bus);
>>
>> acpi_build_tables_init(&tables);
>> acpi_build(&tables, MACHINE(pcms));
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug
2016-06-20 14:27 ` Igor Mammedov
@ 2016-06-21 9:06 ` Marcel Apfelbaum
2016-06-21 11:30 ` Igor Mammedov
0 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-06-21 9:06 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, ehabkost
On 06/20/2016 05:27 PM, Igor Mammedov wrote:
> On Tue, 31 May 2016 20:48:38 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> Re-use the pci acpi hotplug code and enable it only for
>> the new machines using the 'acpi-pci-hotplug-with-bridge-support'
>> compat property.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>> hw/acpi/ich9.c | 31 +++++++++++++++++++++++++++++++
>> hw/isa/lpc_ich9.c | 12 ++++++++++++
>> include/hw/acpi/ich9.h | 4 ++++
>> include/hw/i386/pc.h | 7 ++++++-
>> 4 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 27e978f..499f744 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -230,6 +230,7 @@ static void pm_reset(void *opaque)
>> }
>> pm->smi_en_wmask = ~0;
>>
>> + acpi_pcihp_reset(&pm->acpi_pci_hotplug);
>> acpi_update_sci(&pm->acpi_regs, pm->irq);
>> }
>>
>> @@ -273,6 +274,10 @@ void ich9_pm_init(PCIDevice *lpc_pci,
>> ICH9LPCPMRegs *pm, pm->powerdown_notifier.notify = pm_powerdown_req;
>> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>>
>> + acpi_pcihp_init(OBJECT(lpc_pci), &pm->acpi_pci_hotplug,
>> lpc_pci->bus,
>> + pci_address_space_io(lpc_pci),
>> + pm->use_acpi_pci_hotplug);
> this will register IO window quite high for q35 at ACPI_PCIHP_ADDR
> 0xae00
>
> maybe it should be different for q35 like with CPU hotplug,
> for example below 0xcd8
>
I have no preference here. If you think that using the same IO address is
not desirable, do you have a preferred spot?
Thanks for the review!
Marcel
>> +
>> acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>> OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>>
>> @@ -306,6 +311,21 @@ static void
>> ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
>> s->pm.acpi_memory_hotplug.is_enabled = value; }
>>
>> +static bool ich9_pm_get_acpi_pci_hotplug_support(Object *obj, Error
>> **errp) +{
>> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
>> +
>> + return s->pm.use_acpi_pci_hotplug;
>> +}
>> +
>> +static void ich9_pm_set_acpi_pci_hotplug_support(Object *obj, bool
>> value,
>> + Error **errp)
>> +{
>> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
>> +
>> + s->pm.use_acpi_pci_hotplug = value;
>> +}
>> +
>> static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const
>> char *name, void *opaque, Error **errp)
>> {
>> @@ -397,6 +417,7 @@ void ich9_pm_add_properties(Object *obj,
>> ICH9LPCPMRegs *pm, Error **errp) {
>> static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
>> pm->acpi_memory_hotplug.is_enabled = true;
>> + pm->use_acpi_pci_hotplug = true;
>> pm->disable_s3 = 0;
>> pm->disable_s4 = 0;
>> pm->s4_val = 2;
>> @@ -412,6 +433,10 @@ void ich9_pm_add_properties(Object *obj,
>> ICH9LPCPMRegs *pm, Error **errp) ich9_pm_get_memory_hotplug_support,
>> ich9_pm_set_memory_hotplug_support,
>> NULL);
>> + object_property_add_bool(obj,
>> "acpi-pci-hotplug-with-bridge-support",
>> + ich9_pm_get_acpi_pci_hotplug_support,
>> + ich9_pm_set_acpi_pci_hotplug_support,
>> + NULL);
>> object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
>> ich9_pm_get_disable_s3,
>> ich9_pm_set_disable_s3,
>> @@ -436,6 +461,9 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm,
>> DeviceState *dev, Error **errp) object_dynamic_cast(OBJECT(dev),
>> TYPE_PC_DIMM)) { acpi_memory_plug_cb(&pm->acpi_regs, pm->irq,
>> &pm->acpi_memory_hotplug, dev, errp);
>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> + acpi_pcihp_device_plug_cb(&pm->acpi_regs, pm->irq,
>> + &pm->acpi_pci_hotplug, dev, errp);
>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, &pm->gpe_cpu, dev,
>> errp); } else {
>> @@ -451,6 +479,9 @@ void
>> ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
>> object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))
>> { acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
>> &pm->acpi_memory_hotplug, dev, errp);
>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> + acpi_pcihp_device_unplug_cb(&pm->acpi_regs, pm->irq,
>> + &pm->acpi_pci_hotplug, dev,
>> errp); } else {
>> error_setg(errp, "acpi: device unplug request for not
>> supported device" " type: %s", object_get_typename(OBJECT(dev)));
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 4f8ca45..fb2ce08 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -33,6 +33,7 @@
>> #include "hw/hw.h"
>> #include "qapi/visitor.h"
>> #include "qemu/range.h"
>> +#include "qapi/error.h"
>> #include "hw/isa/isa.h"
>> #include "hw/sysbus.h"
>> #include "hw/i386/pc.h"
>> @@ -511,6 +512,15 @@ static const MemoryRegionOps rcrb_mmio_ops = {
>> .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>>
>> +static void ich9_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
>> +{
>> + ICH9LPCState *s = opaque;
>> +
>> + if (!pci_bus_is_express(pci_bus)) {
>> + qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s),
>> &error_abort);
>> + }
>> +}
>> +
>> static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
>> {
>> ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready);
>> @@ -534,6 +544,8 @@ static void ich9_lpc_machine_ready(Notifier *n,
>> void *opaque) /* floppy */
>> pci_conf[0x82] |= 0x08;
>> }
>> +
>> + pci_for_each_bus(s->d.bus, ich9_update_bus_hotplug, s);
>> }
>>
>> /* reset control */
>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>> index 63fa198..c30c125 100644
>> --- a/include/hw/acpi/ich9.h
>> +++ b/include/hw/acpi/ich9.h
>> @@ -22,6 +22,7 @@
>> #define HW_ACPI_ICH9_H
>>
>> #include "hw/acpi/acpi.h"
>> +#include "hw/acpi/pcihp.h"
>> #include "hw/acpi/cpu_hotplug.h"
>> #include "hw/acpi/memory_hotplug.h"
>> #include "hw/acpi/acpi_dev_interface.h"
>> @@ -52,6 +53,9 @@ typedef struct ICH9LPCPMRegs {
>>
>> MemHotplugState acpi_memory_hotplug;
>>
>> + AcpiPciHpState acpi_pci_hotplug;
>> + bool use_acpi_pci_hotplug;
>> +
>> uint8_t disable_s3;
>> uint8_t disable_s4;
>> uint8_t s4_val;
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 9ca2309..ee941b1 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -357,7 +357,12 @@ int e820_get_num_entries(void);
>> bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>>
>> #define PC_COMPAT_2_5 \
>> - HW_COMPAT_2_5
>> + HW_COMPAT_2_5 \
> should it be 2_6 by now?
>
>> + {\
>> + .driver = "ICH9-LPC",\
>> + .property = "acpi-pci-hotplug-with-bridge-support",\
>> + .value = "off",\
>> + },
>>
>> /* Helper for setting model-id for CPU models that changed model-id
>> * depending on QEMU versions up to QEMU 2.4.
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization
2016-06-21 8:57 ` Marcel Apfelbaum
@ 2016-06-21 11:19 ` Igor Mammedov
2016-06-21 11:50 ` Marcel Apfelbaum
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2016-06-21 11:19 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: pbonzini, qemu-devel, ehabkost, mst
On Tue, 21 Jun 2016 11:57:29 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> On 06/17/2016 12:04 PM, Igor Mammedov wrote:
> > On Tue, 31 May 2016 20:48:35 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> >
> >> The pm initialization code assigns the pcihp IO base and length to
> >> -1 on error, but the later code will assume 0 as invalid value.
> >>
> >> Fix it initializing the above value to 0 as expected.
> >>
> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >> ---
> >> hw/i386/acpi-build.c | 10 ++++++----
> >> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 0c329fb..2097c4c 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -124,17 +124,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >> Object *lpc = ich9_lpc_find();
> >> Object *obj = NULL;
> >> QObject *o;
> >> + int pcihp_io_len, pcihp_io_base;
> >>
> >> pm->cpu_hp_io_base = 0;
> >> - pm->pcihp_io_base = 0;
> >> - pm->pcihp_io_len = 0;
> > this introduces uninitialized memory access in q35 case
> >
>
> How? We are always checking pcihp_io_len/pcihp_io_base.
pm->pcihp_io_len is left uninitialized in q35 case
and then following build_dsdt() would cause jump on uninitialized value
/* reserve PCIHP resources */
if (pm->pcihp_io_len) {
>
> >> if (piix) {
> >> obj = piix;
> >> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> >> - pm->pcihp_io_base =
> >> + pcihp_io_base =
> >> object_property_get_int(obj,
> >> ACPI_PCIHP_IO_BASE_PROP, NULL);
> >> - pm->pcihp_io_len =
> >> + pcihp_io_len =
> >> object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP,
> >> NULL); +
> >> + pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 :
> >> pcihp_io_base;
> >> + pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 :
> >> pcihp_io_len; }
> >> if (lpc) {
> >> obj = lpc;
> >
> > how about something like that:
>
> Please see the next patch. It is only a temporary measure, the next
> patch initialize it right to
> ACPI_PCIHP_IO_BASE_PROP/ACPI_PCIHP_IO_LEN_PROP. If the properties are
> not there, they will be 0, but we are checking this everywhere.
>
> Also I'll prefer pm->pcihp_io_len 0 length to mean "unused" because
> it is used widely. If you still think is a real issue, I'll change
> this, of course.
Maybe squash it into the next patch?
>
>
> Thanks,
> Marcel
>
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 4f9aec6..d753e25 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -125,7 +125,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >
> > pm->cpu_hp_io_base = 0;
> > pm->pcihp_io_base = 0;
> > - pm->pcihp_io_len = 0;
> > + pm->pcihp_io_len = UINT16_MAX;
> > if (piix) {
> > obj = piix;
> > pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > @@ -2053,7 +2053,7 @@ build_dsdt(GArray *table_data, BIOSLinker
> > *linker, g_ptr_array_free(mem_ranges, true);
> >
> > /* reserve PCIHP resources */
> > - if (pm->pcihp_io_len) {
> > + if (pm->pcihp_io_len != UINT16_MAX) {
> > dev = aml_device("PHPR");
> > aml_append(dev, aml_name_decl("_HID",
> > aml_string("PNP0A06"))); aml_append(dev,
> >
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express
2016-06-21 9:05 ` Marcel Apfelbaum
@ 2016-06-21 11:28 ` Igor Mammedov
2016-06-21 11:47 ` Marcel Apfelbaum
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2016-06-21 11:28 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 21 Jun 2016 12:05:12 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> On 06/20/2016 05:11 PM, Igor Mammedov wrote:
> > On Tue, 31 May 2016 20:48:37 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> >
> > subj doesn't match patch, it does opposite i.e. makes sure that
> > pcie isn't counted in when building acpi hotplug aml
> >
>
> Maybe I wasn't clear enough. Until now q35 had no acpi hotplug code.
> This patch adds acpi hotplug code for all Q35 PCI buses, but no for
> pcie buses That means for pci-pci bridge and dmi-pci bridge.
>
>
> Maybe I should rename it to hw/acpi: extend acpi pci hotplug support
> for Q35 pci buses.
this looks better to me
>
> >> Emit acpi code for pci hotplug on all PC machines:
> >> - if legacy pci hotpug is enabled (pcihp_bridge_en)
> >> - if there is at least one legacy pci bridge in current system
> >> - for PCI buses only (skip PCIe buses)
> >>
> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >> ---
> >> hw/i386/acpi-build.c | 49
> >> ++++++++++++++++++++++++++++++++----------------- 1 file changed,
> >> 32 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 8ae3e53..cc51f9e 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo *info)
> >> NULL);
> >> }
> >>
> >> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
> >> +{
> >> + *(bool *)opaque |= !pci_bus_is_express(bus);
> >> +}
> >> +
> >> #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is
> >> APM_CNT_IOPORT */
> >> static void acpi_align_size(GArray *blob, unsigned align)
> >> @@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void
> >> *opaque) unsigned *bsel_alloc = opaque;
> >> unsigned *bus_bsel;
> >>
> >> - if (qbus_is_hotpluggable(BUS(bus))) {
> >> + if (qbus_is_hotpluggable(BUS(bus))
> >> && !pci_bus_is_express(bus)) { bus_bsel = g_malloc(sizeof
> >> *bus_bsel);
> >>
> >> *bus_bsel = (*bsel_alloc)++;
> >> @@ -433,15 +438,12 @@ 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(PCIBus *bus)
> >> {
> >> - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> >> unsigned bsel_alloc = 0;
> >>
> >> - if (bus) {
> >> - /* Scan all PCI buses. Set property to enable acpi based
> >> hotplug. */
> >> - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> >> &bsel_alloc);
> >> - }
> >> + /* Scan all PCI buses. Set property to enable acpi based
> >> hotplug. */
> >> + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> >> &bsel_alloc); }
> >>
> >> static void build_append_pcihp_notify_entry(Aml *method, int
> >> slot) @@ -503,7 +505,7 @@ static void
> >> build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> >> * In this case they aren't themselves hot-pluggable.
> >> * Hotplugged bridges *are* hot-pluggable.
> >> */
> >> - bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
> >> + bridge_in_acpi = pc->is_bridge && !pc->is_express &&
> >> pcihp_bridge_en && !DEVICE(pdev)->hotplugged;
> >>
> >> hotplug_enabled_dev = bsel && dc->hotpluggable
> >> && !bridge_in_acpi; @@ -1892,7 +1894,7 @@ static void
> >> build_piix4_isa_bridge(Aml *table) aml_append(table, scope);
> >> }
> >>
> >> -static void build_piix4_pci_hotplug(Aml *table)
> >> +static void build_acpi_pci_hotplug(Aml *table)
> >> {
> >> Aml *scope;
> >> Aml *field;
> >> @@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray
> >> *linker, uint32_t nr_mem = machine->ram_slots;
> >> int root_bus_limit = 0xFF;
> >> PCIBus *root_bus = NULL;
> >> + bool has_pci_bus = false;
> >> int i;
> >>
> >> dsdt = init_aml_allocator();
> >> @@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray
> >> *linker, build_piix4_pm(dsdt);
> >> build_piix4_isa_bridge(dsdt);
> >> build_isa_devices_aml(dsdt);
> >> - build_piix4_pci_hotplug(dsdt);
> >> build_piix4_pci0_int(dsdt);
> >> } else {
> >> sb_scope = aml_scope("_SB");
> >> @@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray
> >> *linker, root_bus = PC_MACHINE(machine)->bus;
> >> assert(root_bus);
> >>
> >> + pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
> >> + if (pm->pcihp_bridge_en && has_pci_bus) {
> > ^^^^^ wouldn't that forbid acpi hotplug on PCI0 is it's
> > not set?
>
> Yes, and this is by design. 'pcihp_bridge_en' flag matches 'acpi
> hotplug'. I followed the initialization phase, am I wrong? Can you
> have acpi hotplug if the bit is off?
my understanding was that pcihp_bridge_en governs only hotplug
AML for bridges, while with it disabled hotplug AML would be
generated only for PCI0.
> >
> >> + build_acpi_pci_hotplug(dsdt);
> >> + }
> >> +
> >> build_cpu_hotplug_aml(dsdt);
> >> build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
> >> pm->mem_hp_io_len);
> >> @@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray
> >> *linker,
> >>
> >> aml_append(scope, aml_method("_L00", 0,
> >> AML_NOTSERIALIZED));
> >>
> >> - if (misc->is_piix4) {
> >> + if (pm->pcihp_bridge_en && has_pci_bus) {
> > same question as above
> >
> >> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> >> aml_append(method,
> >> aml_acquire(aml_name("\\_SB.PCI0.BLCK"),
> >> 0xFFFF)); @@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data,
> >> GArray *linker, g_ptr_array_free(mem_ranges, true);
> >>
> >> /* reserve PCIHP resources */
> >> - if (pm->pcihp_io_len) {
> >> + if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
> > same question as above
> >
> >> dev = aml_device("PHPR");
> >> aml_append(dev, aml_name_decl("_HID",
> >> aml_string("PNP0A06"))); aml_append(dev,
> >> @@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray
> >> *linker,
> >>
> >> {
> >> Aml *scope = aml_scope("PCI0");
> >> + bool scope_used = false;
> > Could you explain why ^^^ is needed?
> >
>
> Sure. If none ACPI code will be generated by the below 'if' statements
> we will have an empty PCI0 scope. I wanted to get rid of it.
considering that hotplug AML for PCI0 is always generated, that seems
useless.
>
>
> Thanks,
> Marcel
>
> >>
> >> - /* Scan all PCI buses. Generate tables to support
> >> hotplug. */
> >> - build_append_pci_bus_devices(scope, root_bus,
> >> - pm->pcihp_bridge_en);
> >> + if (pm->pcihp_bridge_en && has_pci_bus) {
> >> + /* Scan all PCI buses. Generate tables to support
> >> hotplug. */
> >> + build_append_pci_bus_devices(scope, root_bus,
> >> + pm->pcihp_bridge_en);
> >> + scope_used = true;
> >> + }
> >>
> >> if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> >> dev = aml_device("ISA.TPM");
> >> @@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray
> >> *linker, /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> >> aml_append(dev, aml_name_decl("_CRS", crs));
> >> aml_append(scope, dev);
> >> + scope_used = true;
> >> + }
> >> +
> >> + if (scope_used) {
> >> + aml_append(sb_scope, scope);
> >> }
> >> - aml_append(sb_scope, scope);
> >> }
> >> aml_append(dsdt, sb_scope);
> >> }
> >> @@ -2870,7 +2885,7 @@ void acpi_setup(void)
> >>
> >> build_state = g_malloc0(sizeof *build_state);
> >>
> >> - acpi_set_pci_info();
> >> + acpi_set_pci_info(pcms->bus);
> >>
> >> acpi_build_tables_init(&tables);
> >> acpi_build(&tables, MACHINE(pcms));
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug
2016-06-21 9:06 ` Marcel Apfelbaum
@ 2016-06-21 11:30 ` Igor Mammedov
2016-06-21 11:48 ` Marcel Apfelbaum
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2016-06-21 11:30 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 21 Jun 2016 12:06:56 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> On 06/20/2016 05:27 PM, Igor Mammedov wrote:
> > On Tue, 31 May 2016 20:48:38 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> >
> >> Re-use the pci acpi hotplug code and enable it only for
> >> the new machines using the 'acpi-pci-hotplug-with-bridge-support'
> >> compat property.
> >>
> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >> ---
> >> hw/acpi/ich9.c | 31 +++++++++++++++++++++++++++++++
> >> hw/isa/lpc_ich9.c | 12 ++++++++++++
> >> include/hw/acpi/ich9.h | 4 ++++
> >> include/hw/i386/pc.h | 7 ++++++-
> >> 4 files changed, 53 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >> index 27e978f..499f744 100644
> >> --- a/hw/acpi/ich9.c
> >> +++ b/hw/acpi/ich9.c
> >> @@ -230,6 +230,7 @@ static void pm_reset(void *opaque)
> >> }
> >> pm->smi_en_wmask = ~0;
> >>
> >> + acpi_pcihp_reset(&pm->acpi_pci_hotplug);
> >> acpi_update_sci(&pm->acpi_regs, pm->irq);
> >> }
> >>
> >> @@ -273,6 +274,10 @@ void ich9_pm_init(PCIDevice *lpc_pci,
> >> ICH9LPCPMRegs *pm, pm->powerdown_notifier.notify =
> >> pm_powerdown_req;
> >> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> >>
> >> + acpi_pcihp_init(OBJECT(lpc_pci), &pm->acpi_pci_hotplug,
> >> lpc_pci->bus,
> >> + pci_address_space_io(lpc_pci),
> >> + pm->use_acpi_pci_hotplug);
> > this will register IO window quite high for q35 at ACPI_PCIHP_ADDR
> > 0xae00
> >
> > maybe it should be different for q35 like with CPU hotplug,
> > for example below 0xcd8
> >
>
> I have no preference here. If you think that using the same IO
> address is not desirable, do you have a preferred spot?
putting it below 0xcd8 (cpu-hotplug IO region) would reduce
IO namespace fragmentation.
>
> Thanks for the review!
> Marcel
>
> >> +
> >> acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> >> OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
> >>
> >> @@ -306,6 +311,21 @@ static void
> >> ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
> >> s->pm.acpi_memory_hotplug.is_enabled = value; }
> >>
> >> +static bool ich9_pm_get_acpi_pci_hotplug_support(Object *obj,
> >> Error **errp) +{
> >> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> >> +
> >> + return s->pm.use_acpi_pci_hotplug;
> >> +}
> >> +
> >> +static void ich9_pm_set_acpi_pci_hotplug_support(Object *obj, bool
> >> value,
> >> + Error **errp)
> >> +{
> >> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> >> +
> >> + s->pm.use_acpi_pci_hotplug = value;
> >> +}
> >> +
> >> static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const
> >> char *name, void *opaque, Error **errp)
> >> {
> >> @@ -397,6 +417,7 @@ void ich9_pm_add_properties(Object *obj,
> >> ICH9LPCPMRegs *pm, Error **errp) {
> >> static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> >> pm->acpi_memory_hotplug.is_enabled = true;
> >> + pm->use_acpi_pci_hotplug = true;
> >> pm->disable_s3 = 0;
> >> pm->disable_s4 = 0;
> >> pm->s4_val = 2;
> >> @@ -412,6 +433,10 @@ void ich9_pm_add_properties(Object *obj,
> >> ICH9LPCPMRegs *pm, Error **errp)
> >> ich9_pm_get_memory_hotplug_support,
> >> ich9_pm_set_memory_hotplug_support, NULL);
> >> + object_property_add_bool(obj,
> >> "acpi-pci-hotplug-with-bridge-support",
> >> + ich9_pm_get_acpi_pci_hotplug_support,
> >> + ich9_pm_set_acpi_pci_hotplug_support,
> >> + NULL);
> >> object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
> >> ich9_pm_get_disable_s3,
> >> ich9_pm_set_disable_s3,
> >> @@ -436,6 +461,9 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm,
> >> DeviceState *dev, Error **errp) object_dynamic_cast(OBJECT(dev),
> >> TYPE_PC_DIMM)) { acpi_memory_plug_cb(&pm->acpi_regs, pm->irq,
> >> &pm->acpi_memory_hotplug, dev, errp);
> >> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE))
> >> {
> >> + acpi_pcihp_device_plug_cb(&pm->acpi_regs, pm->irq,
> >> + &pm->acpi_pci_hotplug, dev,
> >> errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >> acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, &pm->gpe_cpu,
> >> dev, errp); } else {
> >> @@ -451,6 +479,9 @@ void
> >> ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState
> >> *dev, object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))
> >> { acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
> >> &pm->acpi_memory_hotplug, dev, errp);
> >> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE))
> >> {
> >> + acpi_pcihp_device_unplug_cb(&pm->acpi_regs, pm->irq,
> >> + &pm->acpi_pci_hotplug, dev,
> >> errp); } else {
> >> error_setg(errp, "acpi: device unplug request for not
> >> supported device" " type: %s", object_get_typename(OBJECT(dev)));
> >> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> >> index 4f8ca45..fb2ce08 100644
> >> --- a/hw/isa/lpc_ich9.c
> >> +++ b/hw/isa/lpc_ich9.c
> >> @@ -33,6 +33,7 @@
> >> #include "hw/hw.h"
> >> #include "qapi/visitor.h"
> >> #include "qemu/range.h"
> >> +#include "qapi/error.h"
> >> #include "hw/isa/isa.h"
> >> #include "hw/sysbus.h"
> >> #include "hw/i386/pc.h"
> >> @@ -511,6 +512,15 @@ static const MemoryRegionOps rcrb_mmio_ops = {
> >> .endianness = DEVICE_LITTLE_ENDIAN,
> >> };
> >>
> >> +static void ich9_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
> >> +{
> >> + ICH9LPCState *s = opaque;
> >> +
> >> + if (!pci_bus_is_express(pci_bus)) {
> >> + qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s),
> >> &error_abort);
> >> + }
> >> +}
> >> +
> >> static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
> >> {
> >> ICH9LPCState *s = container_of(n, ICH9LPCState,
> >> machine_ready); @@ -534,6 +544,8 @@ static void
> >> ich9_lpc_machine_ready(Notifier *n, void *opaque) /* floppy */
> >> pci_conf[0x82] |= 0x08;
> >> }
> >> +
> >> + pci_for_each_bus(s->d.bus, ich9_update_bus_hotplug, s);
> >> }
> >>
> >> /* reset control */
> >> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> >> index 63fa198..c30c125 100644
> >> --- a/include/hw/acpi/ich9.h
> >> +++ b/include/hw/acpi/ich9.h
> >> @@ -22,6 +22,7 @@
> >> #define HW_ACPI_ICH9_H
> >>
> >> #include "hw/acpi/acpi.h"
> >> +#include "hw/acpi/pcihp.h"
> >> #include "hw/acpi/cpu_hotplug.h"
> >> #include "hw/acpi/memory_hotplug.h"
> >> #include "hw/acpi/acpi_dev_interface.h"
> >> @@ -52,6 +53,9 @@ typedef struct ICH9LPCPMRegs {
> >>
> >> MemHotplugState acpi_memory_hotplug;
> >>
> >> + AcpiPciHpState acpi_pci_hotplug;
> >> + bool use_acpi_pci_hotplug;
> >> +
> >> uint8_t disable_s3;
> >> uint8_t disable_s4;
> >> uint8_t s4_val;
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 9ca2309..ee941b1 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -357,7 +357,12 @@ int e820_get_num_entries(void);
> >> bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >>
> >> #define PC_COMPAT_2_5 \
> >> - HW_COMPAT_2_5
> >> + HW_COMPAT_2_5 \
> > should it be 2_6 by now?
> >
> >> + {\
> >> + .driver = "ICH9-LPC",\
> >> + .property = "acpi-pci-hotplug-with-bridge-support",\
> >> + .value = "off",\
> >> + },
> >>
> >> /* Helper for setting model-id for CPU models that changed
> >> model-id
> >> * depending on QEMU versions up to QEMU 2.4.
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express
2016-06-21 11:28 ` Igor Mammedov
@ 2016-06-21 11:47 ` Marcel Apfelbaum
2016-06-21 12:19 ` Igor Mammedov
0 siblings, 1 reply; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-06-21 11:47 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, ehabkost
On 06/21/2016 02:28 PM, Igor Mammedov wrote:
> On Tue, 21 Jun 2016 12:05:12 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 06/20/2016 05:11 PM, Igor Mammedov wrote:
>>> On Tue, 31 May 2016 20:48:37 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>> subj doesn't match patch, it does opposite i.e. makes sure that
>>> pcie isn't counted in when building acpi hotplug aml
>>>
>>
>> Maybe I wasn't clear enough. Until now q35 had no acpi hotplug code.
>> This patch adds acpi hotplug code for all Q35 PCI buses, but no for
>> pcie buses That means for pci-pci bridge and dmi-pci bridge.
>>
>>
>> Maybe I should rename it to hw/acpi: extend acpi pci hotplug support
>> for Q35 pci buses.
> this looks better to me
>
>>
>>>> Emit acpi code for pci hotplug on all PC machines:
>>>> - if legacy pci hotpug is enabled (pcihp_bridge_en)
>>>> - if there is at least one legacy pci bridge in current system
>>>> - for PCI buses only (skip PCIe buses)
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>> hw/i386/acpi-build.c | 49
>>>> ++++++++++++++++++++++++++++++++----------------- 1 file changed,
>>>> 32 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 8ae3e53..cc51f9e 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo *info)
>>>> NULL);
>>>> }
>>>>
>>>> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
>>>> +{
>>>> + *(bool *)opaque |= !pci_bus_is_express(bus);
>>>> +}
>>>> +
>>>> #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is
>>>> APM_CNT_IOPORT */
>>>> static void acpi_align_size(GArray *blob, unsigned align)
>>>> @@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void
>>>> *opaque) unsigned *bsel_alloc = opaque;
>>>> unsigned *bus_bsel;
>>>>
>>>> - if (qbus_is_hotpluggable(BUS(bus))) {
>>>> + if (qbus_is_hotpluggable(BUS(bus))
>>>> && !pci_bus_is_express(bus)) { bus_bsel = g_malloc(sizeof
>>>> *bus_bsel);
>>>>
>>>> *bus_bsel = (*bsel_alloc)++;
>>>> @@ -433,15 +438,12 @@ 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(PCIBus *bus)
>>>> {
>>>> - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
>>>> unsigned bsel_alloc = 0;
>>>>
>>>> - if (bus) {
>>>> - /* Scan all PCI buses. Set property to enable acpi based
>>>> hotplug. */
>>>> - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
>>>> &bsel_alloc);
>>>> - }
>>>> + /* Scan all PCI buses. Set property to enable acpi based
>>>> hotplug. */
>>>> + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
>>>> &bsel_alloc); }
>>>>
>>>> static void build_append_pcihp_notify_entry(Aml *method, int
>>>> slot) @@ -503,7 +505,7 @@ static void
>>>> build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>>>> * In this case they aren't themselves hot-pluggable.
>>>> * Hotplugged bridges *are* hot-pluggable.
>>>> */
>>>> - bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
>>>> + bridge_in_acpi = pc->is_bridge && !pc->is_express &&
>>>> pcihp_bridge_en && !DEVICE(pdev)->hotplugged;
>>>>
>>>> hotplug_enabled_dev = bsel && dc->hotpluggable
>>>> && !bridge_in_acpi; @@ -1892,7 +1894,7 @@ static void
>>>> build_piix4_isa_bridge(Aml *table) aml_append(table, scope);
>>>> }
>>>>
>>>> -static void build_piix4_pci_hotplug(Aml *table)
>>>> +static void build_acpi_pci_hotplug(Aml *table)
>>>> {
>>>> Aml *scope;
>>>> Aml *field;
>>>> @@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, uint32_t nr_mem = machine->ram_slots;
>>>> int root_bus_limit = 0xFF;
>>>> PCIBus *root_bus = NULL;
>>>> + bool has_pci_bus = false;
>>>> int i;
>>>>
>>>> dsdt = init_aml_allocator();
>>>> @@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, build_piix4_pm(dsdt);
>>>> build_piix4_isa_bridge(dsdt);
>>>> build_isa_devices_aml(dsdt);
>>>> - build_piix4_pci_hotplug(dsdt);
>>>> build_piix4_pci0_int(dsdt);
>>>> } else {
>>>> sb_scope = aml_scope("_SB");
>>>> @@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, root_bus = PC_MACHINE(machine)->bus;
>>>> assert(root_bus);
>>>>
>>>> + pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
>>>> + if (pm->pcihp_bridge_en && has_pci_bus) {
>>> ^^^^^ wouldn't that forbid acpi hotplug on PCI0 is it's
>>> not set?
>>
>> Yes, and this is by design. 'pcihp_bridge_en' flag matches 'acpi
>> hotplug'. I followed the initialization phase, am I wrong? Can you
>> have acpi hotplug if the bit is off?
> my understanding was that pcihp_bridge_en governs only hotplug
> AML for bridges, while with it disabled hotplug AML would be
> generated only for PCI0.
>
>>>
>>>> + build_acpi_pci_hotplug(dsdt);
>>>> + }
>>>> +
>>>> build_cpu_hotplug_aml(dsdt);
>>>> build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
>>>> pm->mem_hp_io_len);
>>>> @@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker,
>>>>
>>>> aml_append(scope, aml_method("_L00", 0,
>>>> AML_NOTSERIALIZED));
>>>>
>>>> - if (misc->is_piix4) {
>>>> + if (pm->pcihp_bridge_en && has_pci_bus) {
>>> same question as above
>>>
>>>> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
>>>> aml_append(method,
>>>> aml_acquire(aml_name("\\_SB.PCI0.BLCK"),
>>>> 0xFFFF)); @@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data,
>>>> GArray *linker, g_ptr_array_free(mem_ranges, true);
>>>>
>>>> /* reserve PCIHP resources */
>>>> - if (pm->pcihp_io_len) {
>>>> + if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
>>> same question as above
>>>
>>>> dev = aml_device("PHPR");
>>>> aml_append(dev, aml_name_decl("_HID",
>>>> aml_string("PNP0A06"))); aml_append(dev,
>>>> @@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker,
>>>>
>>>> {
>>>> Aml *scope = aml_scope("PCI0");
>>>> + bool scope_used = false;
>>> Could you explain why ^^^ is needed?
>>>
>>
>> Sure. If none ACPI code will be generated by the below 'if' statements
>> we will have an empty PCI0 scope. I wanted to get rid of it.
> considering that hotplug AML for PCI0 is always generated, that seems
> useless.
>
Not in Q35. pcie.0 is not hot-pluggable.
Run Q35 without a dmi-pci bridge or pci-bridge and you'll have an empty scope.
Thanks,
Marcel
>>
>>
>> Thanks,
>> Marcel
>>
>>>>
>>>> - /* Scan all PCI buses. Generate tables to support
>>>> hotplug. */
>>>> - build_append_pci_bus_devices(scope, root_bus,
>>>> - pm->pcihp_bridge_en);
>>>> + if (pm->pcihp_bridge_en && has_pci_bus) {
>>>> + /* Scan all PCI buses. Generate tables to support
>>>> hotplug. */
>>>> + build_append_pci_bus_devices(scope, root_bus,
>>>> + pm->pcihp_bridge_en);
>>>> + scope_used = true;
>>>> + }
>>>>
>>>> if (misc->tpm_version != TPM_VERSION_UNSPEC) {
>>>> dev = aml_device("ISA.TPM");
>>>> @@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
>>>> aml_append(dev, aml_name_decl("_CRS", crs));
>>>> aml_append(scope, dev);
>>>> + scope_used = true;
>>>> + }
>>>> +
>>>> + if (scope_used) {
>>>> + aml_append(sb_scope, scope);
>>>> }
>>>> - aml_append(sb_scope, scope);
>>>> }
>>>> aml_append(dsdt, sb_scope);
>>>> }
>>>> @@ -2870,7 +2885,7 @@ void acpi_setup(void)
>>>>
>>>> build_state = g_malloc0(sizeof *build_state);
>>>>
>>>> - acpi_set_pci_info();
>>>> + acpi_set_pci_info(pcms->bus);
>>>>
>>>> acpi_build_tables_init(&tables);
>>>> acpi_build(&tables, MACHINE(pcms));
>>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug
2016-06-21 11:30 ` Igor Mammedov
@ 2016-06-21 11:48 ` Marcel Apfelbaum
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-06-21 11:48 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, pbonzini, ehabkost
On 06/21/2016 02:30 PM, Igor Mammedov wrote:
> On Tue, 21 Jun 2016 12:06:56 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 06/20/2016 05:27 PM, Igor Mammedov wrote:
>>> On Tue, 31 May 2016 20:48:38 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>>> Re-use the pci acpi hotplug code and enable it only for
>>>> the new machines using the 'acpi-pci-hotplug-with-bridge-support'
>>>> compat property.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>> hw/acpi/ich9.c | 31 +++++++++++++++++++++++++++++++
>>>> hw/isa/lpc_ich9.c | 12 ++++++++++++
>>>> include/hw/acpi/ich9.h | 4 ++++
>>>> include/hw/i386/pc.h | 7 ++++++-
>>>> 4 files changed, 53 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>>> index 27e978f..499f744 100644
>>>> --- a/hw/acpi/ich9.c
>>>> +++ b/hw/acpi/ich9.c
>>>> @@ -230,6 +230,7 @@ static void pm_reset(void *opaque)
>>>> }
>>>> pm->smi_en_wmask = ~0;
>>>>
>>>> + acpi_pcihp_reset(&pm->acpi_pci_hotplug);
>>>> acpi_update_sci(&pm->acpi_regs, pm->irq);
>>>> }
>>>>
>>>> @@ -273,6 +274,10 @@ void ich9_pm_init(PCIDevice *lpc_pci,
>>>> ICH9LPCPMRegs *pm, pm->powerdown_notifier.notify =
>>>> pm_powerdown_req;
>>>> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>>>>
>>>> + acpi_pcihp_init(OBJECT(lpc_pci), &pm->acpi_pci_hotplug,
>>>> lpc_pci->bus,
>>>> + pci_address_space_io(lpc_pci),
>>>> + pm->use_acpi_pci_hotplug);
>>> this will register IO window quite high for q35 at ACPI_PCIHP_ADDR
>>> 0xae00
>>>
>>> maybe it should be different for q35 like with CPU hotplug,
>>> for example below 0xcd8
>>>
>>
>> I have no preference here. If you think that using the same IO
>> address is not desirable, do you have a preferred spot?
> putting it below 0xcd8 (cpu-hotplug IO region) would reduce
> IO namespace fragmentation.
>
I'll look for an unused region.
Thanks,
Marcel
>>
>> Thanks for the review!
>> Marcel
>>
>>>> +
>>>> acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>>>> OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>>>>
>>>> @@ -306,6 +311,21 @@ static void
>>>> ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
>>>> s->pm.acpi_memory_hotplug.is_enabled = value; }
>>>>
>>>> +static bool ich9_pm_get_acpi_pci_hotplug_support(Object *obj,
>>>> Error **errp) +{
>>>> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
>>>> +
>>>> + return s->pm.use_acpi_pci_hotplug;
>>>> +}
>>>> +
>>>> +static void ich9_pm_set_acpi_pci_hotplug_support(Object *obj, bool
>>>> value,
>>>> + Error **errp)
>>>> +{
>>>> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
>>>> +
>>>> + s->pm.use_acpi_pci_hotplug = value;
>>>> +}
>>>> +
>>>> static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const
>>>> char *name, void *opaque, Error **errp)
>>>> {
>>>> @@ -397,6 +417,7 @@ void ich9_pm_add_properties(Object *obj,
>>>> ICH9LPCPMRegs *pm, Error **errp) {
>>>> static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
>>>> pm->acpi_memory_hotplug.is_enabled = true;
>>>> + pm->use_acpi_pci_hotplug = true;
>>>> pm->disable_s3 = 0;
>>>> pm->disable_s4 = 0;
>>>> pm->s4_val = 2;
>>>> @@ -412,6 +433,10 @@ void ich9_pm_add_properties(Object *obj,
>>>> ICH9LPCPMRegs *pm, Error **errp)
>>>> ich9_pm_get_memory_hotplug_support,
>>>> ich9_pm_set_memory_hotplug_support, NULL);
>>>> + object_property_add_bool(obj,
>>>> "acpi-pci-hotplug-with-bridge-support",
>>>> + ich9_pm_get_acpi_pci_hotplug_support,
>>>> + ich9_pm_set_acpi_pci_hotplug_support,
>>>> + NULL);
>>>> object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
>>>> ich9_pm_get_disable_s3,
>>>> ich9_pm_set_disable_s3,
>>>> @@ -436,6 +461,9 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm,
>>>> DeviceState *dev, Error **errp) object_dynamic_cast(OBJECT(dev),
>>>> TYPE_PC_DIMM)) { acpi_memory_plug_cb(&pm->acpi_regs, pm->irq,
>>>> &pm->acpi_memory_hotplug, dev, errp);
>>>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE))
>>>> {
>>>> + acpi_pcihp_device_plug_cb(&pm->acpi_regs, pm->irq,
>>>> + &pm->acpi_pci_hotplug, dev,
>>>> errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, &pm->gpe_cpu,
>>>> dev, errp); } else {
>>>> @@ -451,6 +479,9 @@ void
>>>> ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState
>>>> *dev, object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))
>>>> { acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
>>>> &pm->acpi_memory_hotplug, dev, errp);
>>>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE))
>>>> {
>>>> + acpi_pcihp_device_unplug_cb(&pm->acpi_regs, pm->irq,
>>>> + &pm->acpi_pci_hotplug, dev,
>>>> errp); } else {
>>>> error_setg(errp, "acpi: device unplug request for not
>>>> supported device" " type: %s", object_get_typename(OBJECT(dev)));
>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>> index 4f8ca45..fb2ce08 100644
>>>> --- a/hw/isa/lpc_ich9.c
>>>> +++ b/hw/isa/lpc_ich9.c
>>>> @@ -33,6 +33,7 @@
>>>> #include "hw/hw.h"
>>>> #include "qapi/visitor.h"
>>>> #include "qemu/range.h"
>>>> +#include "qapi/error.h"
>>>> #include "hw/isa/isa.h"
>>>> #include "hw/sysbus.h"
>>>> #include "hw/i386/pc.h"
>>>> @@ -511,6 +512,15 @@ static const MemoryRegionOps rcrb_mmio_ops = {
>>>> .endianness = DEVICE_LITTLE_ENDIAN,
>>>> };
>>>>
>>>> +static void ich9_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
>>>> +{
>>>> + ICH9LPCState *s = opaque;
>>>> +
>>>> + if (!pci_bus_is_express(pci_bus)) {
>>>> + qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s),
>>>> &error_abort);
>>>> + }
>>>> +}
>>>> +
>>>> static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
>>>> {
>>>> ICH9LPCState *s = container_of(n, ICH9LPCState,
>>>> machine_ready); @@ -534,6 +544,8 @@ static void
>>>> ich9_lpc_machine_ready(Notifier *n, void *opaque) /* floppy */
>>>> pci_conf[0x82] |= 0x08;
>>>> }
>>>> +
>>>> + pci_for_each_bus(s->d.bus, ich9_update_bus_hotplug, s);
>>>> }
>>>>
>>>> /* reset control */
>>>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>>>> index 63fa198..c30c125 100644
>>>> --- a/include/hw/acpi/ich9.h
>>>> +++ b/include/hw/acpi/ich9.h
>>>> @@ -22,6 +22,7 @@
>>>> #define HW_ACPI_ICH9_H
>>>>
>>>> #include "hw/acpi/acpi.h"
>>>> +#include "hw/acpi/pcihp.h"
>>>> #include "hw/acpi/cpu_hotplug.h"
>>>> #include "hw/acpi/memory_hotplug.h"
>>>> #include "hw/acpi/acpi_dev_interface.h"
>>>> @@ -52,6 +53,9 @@ typedef struct ICH9LPCPMRegs {
>>>>
>>>> MemHotplugState acpi_memory_hotplug;
>>>>
>>>> + AcpiPciHpState acpi_pci_hotplug;
>>>> + bool use_acpi_pci_hotplug;
>>>> +
>>>> uint8_t disable_s3;
>>>> uint8_t disable_s4;
>>>> uint8_t s4_val;
>>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>>> index 9ca2309..ee941b1 100644
>>>> --- a/include/hw/i386/pc.h
>>>> +++ b/include/hw/i386/pc.h
>>>> @@ -357,7 +357,12 @@ int e820_get_num_entries(void);
>>>> bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>>>>
>>>> #define PC_COMPAT_2_5 \
>>>> - HW_COMPAT_2_5
>>>> + HW_COMPAT_2_5 \
>>> should it be 2_6 by now?
>>>
>>>> + {\
>>>> + .driver = "ICH9-LPC",\
>>>> + .property = "acpi-pci-hotplug-with-bridge-support",\
>>>> + .value = "off",\
>>>> + },
>>>>
>>>> /* Helper for setting model-id for CPU models that changed
>>>> model-id
>>>> * depending on QEMU versions up to QEMU 2.4.
>>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization
2016-06-21 11:19 ` Igor Mammedov
@ 2016-06-21 11:50 ` Marcel Apfelbaum
0 siblings, 0 replies; 25+ messages in thread
From: Marcel Apfelbaum @ 2016-06-21 11:50 UTC (permalink / raw)
To: Igor Mammedov; +Cc: pbonzini, qemu-devel, ehabkost, mst
On 06/21/2016 02:19 PM, Igor Mammedov wrote:
> On Tue, 21 Jun 2016 11:57:29 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 06/17/2016 12:04 PM, Igor Mammedov wrote:
>>> On Tue, 31 May 2016 20:48:35 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>>> The pm initialization code assigns the pcihp IO base and length to
>>>> -1 on error, but the later code will assume 0 as invalid value.
>>>>
>>>> Fix it initializing the above value to 0 as expected.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>> hw/i386/acpi-build.c | 10 ++++++----
>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 0c329fb..2097c4c 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -124,17 +124,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>>>> Object *lpc = ich9_lpc_find();
>>>> Object *obj = NULL;
>>>> QObject *o;
>>>> + int pcihp_io_len, pcihp_io_base;
>>>>
>>>> pm->cpu_hp_io_base = 0;
>>>> - pm->pcihp_io_base = 0;
>>>> - pm->pcihp_io_len = 0;
>>> this introduces uninitialized memory access in q35 case
>>>
>>
>> How? We are always checking pcihp_io_len/pcihp_io_base.
> pm->pcihp_io_len is left uninitialized in q35 case
>
> and then following build_dsdt() would cause jump on uninitialized value
> /* reserve PCIHP resources */
> if (pm->pcihp_io_len) {
>
>>
>>>> if (piix) {
>>>> obj = piix;
>>>> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
>>>> - pm->pcihp_io_base =
>>>> + pcihp_io_base =
>>>> object_property_get_int(obj,
>>>> ACPI_PCIHP_IO_BASE_PROP, NULL);
>>>> - pm->pcihp_io_len =
>>>> + pcihp_io_len =
>>>> object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP,
>>>> NULL); +
>>>> + pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 :
>>>> pcihp_io_base;
>>>> + pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 :
>>>> pcihp_io_len; }
>>>> if (lpc) {
>>>> obj = lpc;
>>>
>>> how about something like that:
>>
>> Please see the next patch. It is only a temporary measure, the next
>> patch initialize it right to
>> ACPI_PCIHP_IO_BASE_PROP/ACPI_PCIHP_IO_LEN_PROP. If the properties are
>> not there, they will be 0, but we are checking this everywhere.
>>
>> Also I'll prefer pm->pcihp_io_len 0 length to mean "unused" because
>> it is used widely. If you still think is a real issue, I'll change
>> this, of course.
> Maybe squash it into the next patch?
>
I can do that. At first I thought to make the change more readable,
but because of your point I'll squash it.
Thanks,
Marcel
>>
>>
>> Thanks,
>> Marcel
>>
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 4f9aec6..d753e25 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -125,7 +125,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>>>
>>> pm->cpu_hp_io_base = 0;
>>> pm->pcihp_io_base = 0;
>>> - pm->pcihp_io_len = 0;
>>> + pm->pcihp_io_len = UINT16_MAX;
>>> if (piix) {
>>> obj = piix;
>>> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
>>> @@ -2053,7 +2053,7 @@ build_dsdt(GArray *table_data, BIOSLinker
>>> *linker, g_ptr_array_free(mem_ranges, true);
>>>
>>> /* reserve PCIHP resources */
>>> - if (pm->pcihp_io_len) {
>>> + if (pm->pcihp_io_len != UINT16_MAX) {
>>> dev = aml_device("PHPR");
>>> aml_append(dev, aml_name_decl("_HID",
>>> aml_string("PNP0A06"))); aml_append(dev,
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express
2016-06-21 11:47 ` Marcel Apfelbaum
@ 2016-06-21 12:19 ` Igor Mammedov
0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2016-06-21 12:19 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: qemu-devel, mst, pbonzini, ehabkost
On Tue, 21 Jun 2016 14:47:24 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:
> On 06/21/2016 02:28 PM, Igor Mammedov wrote:
> > On Tue, 21 Jun 2016 12:05:12 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> >
> >> On 06/20/2016 05:11 PM, Igor Mammedov wrote:
> >>> On Tue, 31 May 2016 20:48:37 +0300
> >>> Marcel Apfelbaum <marcel@redhat.com> wrote:
> >>>
> >>> subj doesn't match patch, it does opposite i.e. makes sure that
> >>> pcie isn't counted in when building acpi hotplug aml
> >>>
> >>
> >> Maybe I wasn't clear enough. Until now q35 had no acpi hotplug
> >> code. This patch adds acpi hotplug code for all Q35 PCI buses, but
> >> no for pcie buses That means for pci-pci bridge and dmi-pci bridge.
> >>
> >>
> >> Maybe I should rename it to hw/acpi: extend acpi pci hotplug
> >> support for Q35 pci buses.
> > this looks better to me
> >
> >>
> >>>> Emit acpi code for pci hotplug on all PC machines:
> >>>> - if legacy pci hotpug is enabled (pcihp_bridge_en)
> >>>> - if there is at least one legacy pci bridge in current system
> >>>> - for PCI buses only (skip PCIe buses)
> >>>>
> >>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>> ---
> >>>> hw/i386/acpi-build.c | 49
> >>>> ++++++++++++++++++++++++++++++++----------------- 1 file changed,
> >>>> 32 insertions(+), 17 deletions(-)
> >>>>
> >>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>> index 8ae3e53..cc51f9e 100644
> >>>> --- a/hw/i386/acpi-build.c
> >>>> +++ b/hw/i386/acpi-build.c
> >>>> @@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo
> >>>> *info) NULL);
> >>>> }
> >>>>
> >>>> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
> >>>> +{
> >>>> + *(bool *)opaque |= !pci_bus_is_express(bus);
> >>>> +}
> >>>> +
> >>>> #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is
> >>>> APM_CNT_IOPORT */
> >>>> static void acpi_align_size(GArray *blob, unsigned align)
> >>>> @@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void
> >>>> *opaque) unsigned *bsel_alloc = opaque;
> >>>> unsigned *bus_bsel;
> >>>>
> >>>> - if (qbus_is_hotpluggable(BUS(bus))) {
> >>>> + if (qbus_is_hotpluggable(BUS(bus))
> >>>> && !pci_bus_is_express(bus)) { bus_bsel = g_malloc(sizeof
> >>>> *bus_bsel);
> >>>>
> >>>> *bus_bsel = (*bsel_alloc)++;
> >>>> @@ -433,15 +438,12 @@ 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(PCIBus *bus)
> >>>> {
> >>>> - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> >>>> unsigned bsel_alloc = 0;
> >>>>
> >>>> - if (bus) {
> >>>> - /* Scan all PCI buses. Set property to enable acpi based
> >>>> hotplug. */
> >>>> - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> >>>> &bsel_alloc);
> >>>> - }
> >>>> + /* Scan all PCI buses. Set property to enable acpi based
> >>>> hotplug. */
> >>>> + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> >>>> &bsel_alloc); }
> >>>>
> >>>> static void build_append_pcihp_notify_entry(Aml *method, int
> >>>> slot) @@ -503,7 +505,7 @@ static void
> >>>> build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> >>>> * In this case they aren't themselves hot-pluggable.
> >>>> * Hotplugged bridges *are* hot-pluggable.
> >>>> */
> >>>> - bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
> >>>> + bridge_in_acpi = pc->is_bridge && !pc->is_express &&
> >>>> pcihp_bridge_en && !DEVICE(pdev)->hotplugged;
> >>>>
> >>>> hotplug_enabled_dev = bsel && dc->hotpluggable
> >>>> && !bridge_in_acpi; @@ -1892,7 +1894,7 @@ static void
> >>>> build_piix4_isa_bridge(Aml *table) aml_append(table, scope);
> >>>> }
> >>>>
> >>>> -static void build_piix4_pci_hotplug(Aml *table)
> >>>> +static void build_acpi_pci_hotplug(Aml *table)
> >>>> {
> >>>> Aml *scope;
> >>>> Aml *field;
> >>>> @@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray
> >>>> *linker, uint32_t nr_mem = machine->ram_slots;
> >>>> int root_bus_limit = 0xFF;
> >>>> PCIBus *root_bus = NULL;
> >>>> + bool has_pci_bus = false;
> >>>> int i;
> >>>>
> >>>> dsdt = init_aml_allocator();
> >>>> @@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray
> >>>> *linker, build_piix4_pm(dsdt);
> >>>> build_piix4_isa_bridge(dsdt);
> >>>> build_isa_devices_aml(dsdt);
> >>>> - build_piix4_pci_hotplug(dsdt);
> >>>> build_piix4_pci0_int(dsdt);
> >>>> } else {
> >>>> sb_scope = aml_scope("_SB");
> >>>> @@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray
> >>>> *linker, root_bus = PC_MACHINE(machine)->bus;
> >>>> assert(root_bus);
> >>>>
> >>>> + pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
> >>>> + if (pm->pcihp_bridge_en && has_pci_bus) {
> >>> ^^^^^ wouldn't that forbid acpi hotplug on PCI0 is
> >>> it's not set?
> >>
> >> Yes, and this is by design. 'pcihp_bridge_en' flag matches 'acpi
> >> hotplug'. I followed the initialization phase, am I wrong? Can you
> >> have acpi hotplug if the bit is off?
> > my understanding was that pcihp_bridge_en governs only hotplug
> > AML for bridges, while with it disabled hotplug AML would be
> > generated only for PCI0.
> >
> >>>
> >>>> + build_acpi_pci_hotplug(dsdt);
> >>>> + }
> >>>> +
> >>>> build_cpu_hotplug_aml(dsdt);
> >>>> build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
> >>>> pm->mem_hp_io_len);
> >>>> @@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray
> >>>> *linker,
> >>>>
> >>>> aml_append(scope, aml_method("_L00", 0,
> >>>> AML_NOTSERIALIZED));
> >>>>
> >>>> - if (misc->is_piix4) {
> >>>> + if (pm->pcihp_bridge_en && has_pci_bus) {
> >>> same question as above
> >>>
> >>>> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> >>>> aml_append(method,
> >>>> aml_acquire(aml_name("\\_SB.PCI0.BLCK"),
> >>>> 0xFFFF)); @@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data,
> >>>> GArray *linker, g_ptr_array_free(mem_ranges, true);
> >>>>
> >>>> /* reserve PCIHP resources */
> >>>> - if (pm->pcihp_io_len) {
> >>>> + if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len)
> >>>> {
> >>> same question as above
> >>>
> >>>> dev = aml_device("PHPR");
> >>>> aml_append(dev, aml_name_decl("_HID",
> >>>> aml_string("PNP0A06"))); aml_append(dev,
> >>>> @@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray
> >>>> *linker,
> >>>>
> >>>> {
> >>>> Aml *scope = aml_scope("PCI0");
> >>>> + bool scope_used = false;
> >>> Could you explain why ^^^ is needed?
> >>>
> >>
> >> Sure. If none ACPI code will be generated by the below 'if'
> >> statements we will have an empty PCI0 scope. I wanted to get rid
> >> of it.
> > considering that hotplug AML for PCI0 is always generated, that
> > seems useless.
> >
>
> Not in Q35. pcie.0 is not hot-pluggable.
> Run Q35 without a dmi-pci bridge or pci-bridge and you'll have an
> empty scope.
ok,
just make sure patch affects only Q35 and doesn't regress piix4
(where PCI0 always has hotplug AML)
I'd write it a little bit different though:
if (pm->pcihp_bridge_en && has_pci_bus) {
Aml *scope = aml_scope("PCI0");
build_append_pci_bus_devices(scope, root_bus,...
aml_append(sb_scope, scope);
}
if (misc->tpm_version != TPM_VERSION_UNSPEC) {
dev = aml_device("PCI0.ISA.TPM");
...
aml_append(sb_scope, dev);
}
which looks somewhat cleaner than tracking if scope was used.
>
>
> Thanks,
> Marcel
>
> >>
> >>
> >> Thanks,
> >> Marcel
> >>
> >>>>
> >>>> - /* Scan all PCI buses. Generate tables to support
> >>>> hotplug. */
> >>>> - build_append_pci_bus_devices(scope, root_bus,
> >>>> - pm->pcihp_bridge_en);
> >>>> + if (pm->pcihp_bridge_en && has_pci_bus) {
> >>>> + /* Scan all PCI buses. Generate tables to
> >>>> support hotplug. */
> >>>> + build_append_pci_bus_devices(scope, root_bus,
> >>>> +
> >>>> pm->pcihp_bridge_en);
> >>>> + scope_used = true;
> >>>> + }
> >>>>
> >>>> if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> >>>> dev = aml_device("ISA.TPM");
> >>>> @@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray
> >>>> *linker, /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> >>>> aml_append(dev, aml_name_decl("_CRS", crs));
> >>>> aml_append(scope, dev);
> >>>> + scope_used = true;
> >>>> + }
> >>>> +
> >>>> + if (scope_used) {
> >>>> + aml_append(sb_scope, scope);
> >>>> }
> >>>> - aml_append(sb_scope, scope);
> >>>> }
> >>>> aml_append(dsdt, sb_scope);
> >>>> }
> >>>> @@ -2870,7 +2885,7 @@ void acpi_setup(void)
> >>>>
> >>>> build_state = g_malloc0(sizeof *build_state);
> >>>>
> >>>> - acpi_set_pci_info();
> >>>> + acpi_set_pci_info(pcms->bus);
> >>>>
> >>>> acpi_build_tables_init(&tables);
> >>>> acpi_build(&tables, MACHINE(pcms));
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2016-06-21 12:19 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-31 17:48 [Qemu-devel] [PATCH RFC 0/7] q35: add legacy pci acpi hotplug support Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 1/7] hw/acpi: remove dead acpi code Marcel Apfelbaum
2016-06-17 7:57 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 2/7] hw/acpi: simplify dsdt building code Marcel Apfelbaum
2016-06-17 8:39 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 3/7] hw/acpi: fix a DSDT table issue when a pxb is present Marcel Apfelbaum
2016-06-17 8:57 ` Igor Mammedov
2016-06-21 8:50 ` Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization Marcel Apfelbaum
2016-06-17 9:04 ` Igor Mammedov
2016-06-21 8:57 ` Marcel Apfelbaum
2016-06-21 11:19 ` Igor Mammedov
2016-06-21 11:50 ` Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 5/7] hw/acpi: prepare pci hotplug IO for ich9 Marcel Apfelbaum
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express Marcel Apfelbaum
2016-06-20 14:11 ` Igor Mammedov
2016-06-21 9:05 ` Marcel Apfelbaum
2016-06-21 11:28 ` Igor Mammedov
2016-06-21 11:47 ` Marcel Apfelbaum
2016-06-21 12:19 ` Igor Mammedov
2016-05-31 17:48 ` [Qemu-devel] [PATCH RFC 7/7] hw/ich9: enable pci acpi hotplug Marcel Apfelbaum
2016-06-20 14:27 ` Igor Mammedov
2016-06-21 9:06 ` Marcel Apfelbaum
2016-06-21 11:30 ` Igor Mammedov
2016-06-21 11:48 ` Marcel Apfelbaum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.