* [RFC PATCH v3 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
@ 2020-09-24 7:00 ` Julia Suvorova
2020-09-24 10:53 ` Igor Mammedov
2020-09-24 10:54 ` Ani Sinha
2020-09-24 7:00 ` [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 Julia Suvorova
` (9 subsequent siblings)
10 siblings, 2 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
PCI Express does not allow hot-plug on pcie.0. Check for Q35 in
acpi_pcihp_disable_root_bus() to be able to forbid hot-plug using the
'acpi-root-pci-hotplug' flag.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
hw/acpi/pcihp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 39b1f74442..ff23104aea 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -107,13 +107,14 @@ static void acpi_set_pci_info(void)
static void acpi_pcihp_disable_root_bus(void)
{
static bool root_hp_disabled;
+ Object *host = acpi_get_i386_pci_host();
PCIBus *bus;
if (root_hp_disabled) {
return;
}
- bus = find_i440fx();
+ bus = PCI_HOST_BRIDGE(host)->bus;
if (bus) {
/* setting the hotplug handler to NULL makes the bus non-hotpluggable */
qbus_set_hotplug_handler(BUS(bus), NULL);
--
2.25.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
2020-09-24 7:00 ` [RFC PATCH v3 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 Julia Suvorova
@ 2020-09-24 10:53 ` Igor Mammedov
2020-09-24 10:54 ` Ani Sinha
1 sibling, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2020-09-24 10:53 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020 09:00:07 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> PCI Express does not allow hot-plug on pcie.0. Check for Q35 in
> acpi_pcihp_disable_root_bus() to be able to forbid hot-plug using the
> 'acpi-root-pci-hotplug' flag.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/acpi/pcihp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 39b1f74442..ff23104aea 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -107,13 +107,14 @@ static void acpi_set_pci_info(void)
> static void acpi_pcihp_disable_root_bus(void)
> {
> static bool root_hp_disabled;
> + Object *host = acpi_get_i386_pci_host();
> PCIBus *bus;
>
> if (root_hp_disabled) {
> return;
> }
>
> - bus = find_i440fx();
> + bus = PCI_HOST_BRIDGE(host)->bus;
> if (bus) {
> /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> qbus_set_hotplug_handler(BUS(bus), NULL);
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
2020-09-24 7:00 ` [RFC PATCH v3 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 Julia Suvorova
2020-09-24 10:53 ` Igor Mammedov
@ 2020-09-24 10:54 ` Ani Sinha
1 sibling, 0 replies; 40+ messages in thread
From: Ani Sinha @ 2020-09-24 10:54 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Igor Mammedov, QEMU Developers, Michael S. Tsirkin
On Thu, Sep 24, 2020 at 12:30 PM Julia Suvorova <jusual@redhat.com> wrote:
>
> PCI Express does not allow hot-plug on pcie.0. Check for Q35 in
> acpi_pcihp_disable_root_bus() to be able to forbid hot-plug using the
> 'acpi-root-pci-hotplug' flag.
I think what this change is doing here is making
acpi_pcihp_disable_root_bus() function compatible with q35 as well.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/acpi/pcihp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 39b1f74442..ff23104aea 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -107,13 +107,14 @@ static void acpi_set_pci_info(void)
> static void acpi_pcihp_disable_root_bus(void)
> {
> static bool root_hp_disabled;
> + Object *host = acpi_get_i386_pci_host();
> PCIBus *bus;
>
> if (root_hp_disabled) {
> return;
> }
>
> - bus = find_i440fx();
> + bus = PCI_HOST_BRIDGE(host)->bus;
> if (bus) {
> /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> qbus_set_hotplug_handler(BUS(bus), NULL);
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
2020-09-24 7:00 ` [RFC PATCH v3 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 Julia Suvorova
@ 2020-09-24 7:00 ` Julia Suvorova
2020-09-24 11:04 ` Igor Mammedov
2020-09-24 13:15 ` Ani Sinha
2020-09-24 7:00 ` [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used Julia Suvorova
` (8 subsequent siblings)
10 siblings, 2 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
Implement notifications and gpe to support q35 ACPI PCI hot-plug.
Use 0xcc4 - 0xcd7 range for 'acpi-pci-hotplug' io ports.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
hw/i386/acpi-build.h | 4 ++++
include/hw/acpi/ich9.h | 2 ++
include/hw/acpi/pcihp.h | 3 ++-
hw/acpi/pcihp.c | 8 ++++----
hw/acpi/piix4.c | 4 +++-
hw/i386/acpi-build.c | 27 ++++++++++++++++-----------
6 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 74df5fc612..487ec7710f 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -5,6 +5,10 @@
extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
+/* PCI Hot-plug registers bases. See docs/spec/acpi_pci_hotplug.txt */
+#define ACPI_PCIHP_SEJ_BASE 0x8
+#define ACPI_PCIHP_BNMR_BASE 0x10
+
void acpi_setup(void);
#endif
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 28a53181cb..4d19571ed7 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -28,6 +28,8 @@
#include "hw/acpi/acpi_dev_interface.h"
#include "hw/acpi/tco.h"
+#define ACPI_PCIHP_ADDR_ICH9 0x0cc4
+
typedef struct ICH9LPCPMRegs {
/*
* In ich9 spec says that pm1_cnt register is 32bit width and
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 02f4665767..ce49fb03b9 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -54,7 +54,8 @@ typedef struct AcpiPciHpState {
} AcpiPciHpState;
void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
- MemoryRegion *address_space_io, bool bridges_enabled);
+ MemoryRegion *address_space_io, bool bridges_enabled,
+ uint16_t io_base);
void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp);
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index ff23104aea..bb457bc279 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -38,7 +38,6 @@
#include "qom/qom-qobject.h"
#include "trace.h"
-#define ACPI_PCIHP_ADDR 0xae00
#define ACPI_PCIHP_SIZE 0x0014
#define PCI_UP_BASE 0x0000
#define PCI_DOWN_BASE 0x0004
@@ -381,12 +380,13 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
};
void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
- MemoryRegion *address_space_io, bool bridges_enabled)
+ MemoryRegion *address_space_io, bool bridges_enabled,
+ uint16_t io_base)
{
s->io_len = ACPI_PCIHP_SIZE;
- s->io_base = ACPI_PCIHP_ADDR;
+ s->io_base = io_base;
- s->root= root_bus;
+ s->root = root_bus;
s->legacy_piix = !bridges_enabled;
memory_region_init_io(&s->io, owner, &acpi_pcihp_io_ops, s,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 832f8fba82..a505ab5bcf 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -50,6 +50,8 @@
#define GPE_BASE 0xafe0
#define GPE_LEN 4
+#define ACPI_PCIHP_ADDR_PIIX4 0xae00
+
struct pci_status {
uint32_t up; /* deprecated, maintained for migration compatibility */
uint32_t down;
@@ -597,7 +599,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
- s->use_acpi_hotplug_bridge);
+ s->use_acpi_hotplug_bridge, ACPI_PCIHP_ADDR_PIIX4);
s->cpu_hotplug_legacy = true;
object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0e0535d2e3..cf503b16af 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
/* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
pm->fadt.rev = 1;
pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
- pm->pcihp_io_base =
- object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
- pm->pcihp_io_len =
- object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
}
if (lpc) {
struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
@@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
}
+ pm->pcihp_io_base =
+ object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
+ pm->pcihp_io_len =
+ object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
/* The above need not be conditional on machine type because the reset port
* happens to be the same on PIIX (pc) and ICH9 (q35). */
@@ -472,7 +472,7 @@ 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)) {
+ if (pci_bus_is_root(sec)) {
continue;
}
@@ -1368,7 +1368,7 @@ static void build_piix4_isa_bridge(Aml *table)
aml_append(table, scope);
}
-static void build_piix4_pci_hotplug(Aml *table)
+static void build_x86_pci_hotplug(Aml *table, uint64_t pcihp_addr)
{
Aml *scope;
Aml *field;
@@ -1377,20 +1377,22 @@ static void build_piix4_pci_hotplug(Aml *table)
scope = aml_scope("_SB.PCI0");
aml_append(scope,
- aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x08));
+ aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(pcihp_addr), 0x08));
field = aml_field("PCST", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
aml_append(field, aml_named_field("PCIU", 32));
aml_append(field, aml_named_field("PCID", 32));
aml_append(scope, field);
aml_append(scope,
- aml_operation_region("SEJ", AML_SYSTEM_IO, aml_int(0xae08), 0x04));
+ aml_operation_region("SEJ", AML_SYSTEM_IO,
+ aml_int(pcihp_addr + ACPI_PCIHP_SEJ_BASE), 0x04));
field = aml_field("SEJ", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
aml_append(field, aml_named_field("B0EJ", 32));
aml_append(scope, field);
aml_append(scope,
- aml_operation_region("BNMR", AML_SYSTEM_IO, aml_int(0xae10), 0x04));
+ aml_operation_region("BNMR", AML_SYSTEM_IO,
+ aml_int(pcihp_addr + ACPI_PCIHP_BNMR_BASE), 0x04));
field = aml_field("BNMR", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
aml_append(field, aml_named_field("BNUM", 32));
aml_append(scope, field);
@@ -1504,7 +1506,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
build_hpet_aml(dsdt);
build_piix4_isa_bridge(dsdt);
build_isa_devices_aml(dsdt);
- build_piix4_pci_hotplug(dsdt);
+ build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
build_piix4_pci0_int(dsdt);
} else {
sb_scope = aml_scope("_SB");
@@ -1520,6 +1522,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
build_hpet_aml(dsdt);
build_q35_isa_bridge(dsdt);
build_isa_devices_aml(dsdt);
+ if (pm->pcihp_bridge_en) {
+ build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
+ }
build_q35_pci0_int(dsdt);
if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
@@ -1546,7 +1551,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
{
aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
- if (misc->is_piix4) {
+ if (misc->is_piix4 || pm->pcihp_bridge_en) {
method = aml_method("_E01", 0, AML_NOTSERIALIZED);
aml_append(method,
aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
--
2.25.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
2020-09-24 7:00 ` [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 Julia Suvorova
@ 2020-09-24 11:04 ` Igor Mammedov
2020-09-25 8:20 ` Gerd Hoffmann
2020-09-24 13:15 ` Ani Sinha
1 sibling, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2020-09-24 11:04 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Gerd Hoffmann, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020 09:00:08 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> Use 0xcc4 - 0xcd7 range for 'acpi-pci-hotplug' io ports.
Gerd,
does picked IO range looks acceptable to you?
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
with minor nits below fixed:
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/i386/acpi-build.h | 4 ++++
> include/hw/acpi/ich9.h | 2 ++
> include/hw/acpi/pcihp.h | 3 ++-
> hw/acpi/pcihp.c | 8 ++++----
> hw/acpi/piix4.c | 4 +++-
> hw/i386/acpi-build.c | 27 ++++++++++++++++-----------
> 6 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 74df5fc612..487ec7710f 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -5,6 +5,10 @@
>
> extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>
> +/* PCI Hot-plug registers bases. See docs/spec/acpi_pci_hotplug.txt */
> +#define ACPI_PCIHP_SEJ_BASE 0x8
> +#define ACPI_PCIHP_BNMR_BASE 0x10
> +
> void acpi_setup(void);
>
> #endif
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 28a53181cb..4d19571ed7 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -28,6 +28,8 @@
> #include "hw/acpi/acpi_dev_interface.h"
> #include "hw/acpi/tco.h"
>
> +#define ACPI_PCIHP_ADDR_ICH9 0x0cc4
> +
> typedef struct ICH9LPCPMRegs {
> /*
> * In ich9 spec says that pm1_cnt register is 32bit width and
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 02f4665767..ce49fb03b9 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -54,7 +54,8 @@ typedef struct AcpiPciHpState {
> } AcpiPciHpState;
>
> void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
> - MemoryRegion *address_space_io, bool bridges_enabled);
> + MemoryRegion *address_space_io, bool bridges_enabled,
> + uint16_t io_base);
>
> void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp);
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index ff23104aea..bb457bc279 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -38,7 +38,6 @@
> #include "qom/qom-qobject.h"
> #include "trace.h"
>
> -#define ACPI_PCIHP_ADDR 0xae00
> #define ACPI_PCIHP_SIZE 0x0014
> #define PCI_UP_BASE 0x0000
> #define PCI_DOWN_BASE 0x0004
> @@ -381,12 +380,13 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
> };
>
> void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> - MemoryRegion *address_space_io, bool bridges_enabled)
> + MemoryRegion *address_space_io, bool bridges_enabled,
> + uint16_t io_base)
> {
> s->io_len = ACPI_PCIHP_SIZE;
> - s->io_base = ACPI_PCIHP_ADDR;
> + s->io_base = io_base;
>
> - s->root= root_bus;
> + s->root = root_bus;
> s->legacy_piix = !bridges_enabled;
>
> memory_region_init_io(&s->io, owner, &acpi_pcihp_io_ops, s,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 832f8fba82..a505ab5bcf 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -50,6 +50,8 @@
> #define GPE_BASE 0xafe0
> #define GPE_LEN 4
>
> +#define ACPI_PCIHP_ADDR_PIIX4 0xae00
> +
> struct pci_status {
> uint32_t up; /* deprecated, maintained for migration compatibility */
> uint32_t down;
> @@ -597,7 +599,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>
> acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> - s->use_acpi_hotplug_bridge);
> + s->use_acpi_hotplug_bridge, ACPI_PCIHP_ADDR_PIIX4);
>
> s->cpu_hotplug_legacy = true;
> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0e0535d2e3..cf503b16af 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> pm->fadt.rev = 1;
> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> - pm->pcihp_io_base =
> - object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> - pm->pcihp_io_len =
> - object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> }
> if (lpc) {
> struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> }
> + pm->pcihp_io_base =
> + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> + pm->pcihp_io_len =
> + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>
> /* The above need not be conditional on machine type because the reset port
> * happens to be the same on PIIX (pc) and ICH9 (q35). */
> @@ -472,7 +472,7 @@ 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)) {
> + if (pci_bus_is_root(sec)) {
> continue;
> }
>
> @@ -1368,7 +1368,7 @@ static void build_piix4_isa_bridge(Aml *table)
> aml_append(table, scope);
> }
>
> -static void build_piix4_pci_hotplug(Aml *table)
> +static void build_x86_pci_hotplug(Aml *table, uint64_t pcihp_addr)
maybe
s/_pci_/_acpi_pci_/
otherwise it could be a bit confusing
> {
> Aml *scope;
> Aml *field;
> @@ -1377,20 +1377,22 @@ static void build_piix4_pci_hotplug(Aml *table)
> scope = aml_scope("_SB.PCI0");
>
> aml_append(scope,
> - aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x08));
> + aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(pcihp_addr), 0x08));
> field = aml_field("PCST", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> aml_append(field, aml_named_field("PCIU", 32));
> aml_append(field, aml_named_field("PCID", 32));
> aml_append(scope, field);
>
> aml_append(scope,
> - aml_operation_region("SEJ", AML_SYSTEM_IO, aml_int(0xae08), 0x04));
> + aml_operation_region("SEJ", AML_SYSTEM_IO,
not aligned properly
> + aml_int(pcihp_addr + ACPI_PCIHP_SEJ_BASE), 0x04));
> field = aml_field("SEJ", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> aml_append(field, aml_named_field("B0EJ", 32));
> aml_append(scope, field);
>
> aml_append(scope,
> - aml_operation_region("BNMR", AML_SYSTEM_IO, aml_int(0xae10), 0x04));
> + aml_operation_region("BNMR", AML_SYSTEM_IO,
> + aml_int(pcihp_addr + ACPI_PCIHP_BNMR_BASE), 0x04));
> field = aml_field("BNMR", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> aml_append(field, aml_named_field("BNUM", 32));
> aml_append(scope, field);
> @@ -1504,7 +1506,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> build_hpet_aml(dsdt);
> build_piix4_isa_bridge(dsdt);
> build_isa_devices_aml(dsdt);
> - build_piix4_pci_hotplug(dsdt);
> + build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
> build_piix4_pci0_int(dsdt);
> } else {
> sb_scope = aml_scope("_SB");
> @@ -1520,6 +1522,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> build_hpet_aml(dsdt);
> build_q35_isa_bridge(dsdt);
> build_isa_devices_aml(dsdt);
> + if (pm->pcihp_bridge_en) {
> + build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
> + }
> build_q35_pci0_int(dsdt);
> if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> @@ -1546,7 +1551,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> {
> aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
>
> - if (misc->is_piix4) {
> + if (misc->is_piix4 || pm->pcihp_bridge_en) {
> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> aml_append(method,
> aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
2020-09-24 11:04 ` Igor Mammedov
@ 2020-09-25 8:20 ` Gerd Hoffmann
0 siblings, 0 replies; 40+ messages in thread
From: Gerd Hoffmann @ 2020-09-25 8:20 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Ani Sinha, Julia Suvorova, qemu-devel, Michael S. Tsirkin
On Thu, Sep 24, 2020 at 01:04:19PM +0200, Igor Mammedov wrote:
> On Thu, 24 Sep 2020 09:00:08 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > Use 0xcc4 - 0xcd7 range for 'acpi-pci-hotplug' io ports.
>
> Gerd,
>
> does picked IO range looks acceptable to you?
Yes, that is fine.
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
2020-09-24 7:00 ` [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 Julia Suvorova
2020-09-24 11:04 ` Igor Mammedov
@ 2020-09-24 13:15 ` Ani Sinha
2020-09-24 13:58 ` Julia Suvorova
1 sibling, 1 reply; 40+ messages in thread
From: Ani Sinha @ 2020-09-24 13:15 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020, Julia Suvorova wrote:
> Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> Use 0xcc4 - 0xcd7 range for 'acpi-pci-hotplug' io ports.
>
For this patch, I would suggest maybe you can also add a diff of the
disassembly of the DSDT table so that we know what changes exactly we
shall see in the table as a result of this patch.
Add the diff to this commit message as it will be helpful for anyone
to take a look at it when they look at this patch.
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/i386/acpi-build.h | 4 ++++
> include/hw/acpi/ich9.h | 2 ++
> include/hw/acpi/pcihp.h | 3 ++-
> hw/acpi/pcihp.c | 8 ++++----
> hw/acpi/piix4.c | 4 +++-
> hw/i386/acpi-build.c | 27 ++++++++++++++++-----------
> 6 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 74df5fc612..487ec7710f 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -5,6 +5,10 @@
>
> extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>
> +/* PCI Hot-plug registers bases. See docs/spec/acpi_pci_hotplug.txt */
> +#define ACPI_PCIHP_SEJ_BASE 0x8
> +#define ACPI_PCIHP_BNMR_BASE 0x10
> +
> void acpi_setup(void);
>
> #endif
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 28a53181cb..4d19571ed7 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -28,6 +28,8 @@
> #include "hw/acpi/acpi_dev_interface.h"
> #include "hw/acpi/tco.h"
>
> +#define ACPI_PCIHP_ADDR_ICH9 0x0cc4
> +
> typedef struct ICH9LPCPMRegs {
> /*
> * In ich9 spec says that pm1_cnt register is 32bit width and
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 02f4665767..ce49fb03b9 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -54,7 +54,8 @@ typedef struct AcpiPciHpState {
> } AcpiPciHpState;
>
> void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
> - MemoryRegion *address_space_io, bool bridges_enabled);
> + MemoryRegion *address_space_io, bool bridges_enabled,
> + uint16_t io_base);
>
> void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp);
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index ff23104aea..bb457bc279 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -38,7 +38,6 @@
> #include "qom/qom-qobject.h"
> #include "trace.h"
>
> -#define ACPI_PCIHP_ADDR 0xae00
> #define ACPI_PCIHP_SIZE 0x0014
> #define PCI_UP_BASE 0x0000
> #define PCI_DOWN_BASE 0x0004
> @@ -381,12 +380,13 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
> };
>
> void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> - MemoryRegion *address_space_io, bool bridges_enabled)
> + MemoryRegion *address_space_io, bool bridges_enabled,
> + uint16_t io_base)
> {
> s->io_len = ACPI_PCIHP_SIZE;
> - s->io_base = ACPI_PCIHP_ADDR;
> + s->io_base = io_base;
Maybe you want to remove ACPI_PCIHP_ADDR ?
>
> - s->root= root_bus;
> + s->root = root_bus;
> s->legacy_piix = !bridges_enabled;
>
> memory_region_init_io(&s->io, owner, &acpi_pcihp_io_ops, s,
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 832f8fba82..a505ab5bcf 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -50,6 +50,8 @@
> #define GPE_BASE 0xafe0
> #define GPE_LEN 4
>
> +#define ACPI_PCIHP_ADDR_PIIX4 0xae00
> +
> struct pci_status {
> uint32_t up; /* deprecated, maintained for migration compatibility */
> uint32_t down;
> @@ -597,7 +599,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>
> acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> - s->use_acpi_hotplug_bridge);
> + s->use_acpi_hotplug_bridge, ACPI_PCIHP_ADDR_PIIX4);
>
> s->cpu_hotplug_legacy = true;
> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0e0535d2e3..cf503b16af 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> pm->fadt.rev = 1;
> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> - pm->pcihp_io_base =
> - object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> - pm->pcihp_io_len =
> - object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> }
> if (lpc) {
> struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> }
> + pm->pcihp_io_base =
> + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> + pm->pcihp_io_len =
> + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>
> /* The above need not be conditional on machine type because the reset port
> * happens to be the same on PIIX (pc) and ICH9 (q35). */
> @@ -472,7 +472,7 @@ 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)) {
> + if (pci_bus_is_root(sec)) {
> continue;
> }
>
> @@ -1368,7 +1368,7 @@ static void build_piix4_isa_bridge(Aml *table)
> aml_append(table, scope);
> }
>
> -static void build_piix4_pci_hotplug(Aml *table)
> +static void build_x86_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> {
> Aml *scope;
> Aml *field;
> @@ -1377,20 +1377,22 @@ static void build_piix4_pci_hotplug(Aml *table)
> scope = aml_scope("_SB.PCI0");
>
> aml_append(scope,
> - aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x08));
> + aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(pcihp_addr), 0x08));
> field = aml_field("PCST", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> aml_append(field, aml_named_field("PCIU", 32));
> aml_append(field, aml_named_field("PCID", 32));
> aml_append(scope, field);
>
> aml_append(scope,
> - aml_operation_region("SEJ", AML_SYSTEM_IO, aml_int(0xae08), 0x04));
> + aml_operation_region("SEJ", AML_SYSTEM_IO,
> + aml_int(pcihp_addr + ACPI_PCIHP_SEJ_BASE), 0x04));
> field = aml_field("SEJ", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> aml_append(field, aml_named_field("B0EJ", 32));
> aml_append(scope, field);
>
> aml_append(scope,
> - aml_operation_region("BNMR", AML_SYSTEM_IO, aml_int(0xae10), 0x04));
> + aml_operation_region("BNMR", AML_SYSTEM_IO,
> + aml_int(pcihp_addr + ACPI_PCIHP_BNMR_BASE), 0x04));
> field = aml_field("BNMR", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> aml_append(field, aml_named_field("BNUM", 32));
> aml_append(scope, field);
> @@ -1504,7 +1506,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> build_hpet_aml(dsdt);
> build_piix4_isa_bridge(dsdt);
> build_isa_devices_aml(dsdt);
> - build_piix4_pci_hotplug(dsdt);
> + build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
This will conflict here with my patch. I think you need to do something
like this:
if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
You'll get this from my patch.
> build_piix4_pci0_int(dsdt);
> } else {
> sb_scope = aml_scope("_SB");
> @@ -1520,6 +1522,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> build_hpet_aml(dsdt);
> build_q35_isa_bridge(dsdt);
> build_isa_devices_aml(dsdt);
> + if (pm->pcihp_bridge_en) {
> + build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
> + }
ditto as above.
> build_q35_pci0_int(dsdt);
> if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> @@ -1546,7 +1551,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> {
> aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
>
> - if (misc->is_piix4) {
> + if (misc->is_piix4 || pm->pcihp_bridge_en) {
> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> aml_append(method,
> aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
> --
> 2.25.4
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
2020-09-24 13:15 ` Ani Sinha
@ 2020-09-24 13:58 ` Julia Suvorova
0 siblings, 0 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 13:58 UTC (permalink / raw)
To: Ani Sinha; +Cc: Igor Mammedov, QEMU Developers, Michael S. Tsirkin
On Thu, Sep 24, 2020 at 3:15 PM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Thu, 24 Sep 2020, Julia Suvorova wrote:
>
> > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > Use 0xcc4 - 0xcd7 range for 'acpi-pci-hotplug' io ports.
> >
>
> For this patch, I would suggest maybe you can also add a diff of the
> disassembly of the DSDT table so that we know what changes exactly we
> shall see in the table as a result of this patch.
> Add the diff to this commit message as it will be helpful for anyone
> to take a look at it when they look at this patch.
There is no difference in golden DSDT, because this is an option,
which is disabled by default.
The diff is placed in the patch where it actually makes a difference.
But I agree that a list of changed registers would be a nice addition
to the commit message.
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > hw/i386/acpi-build.h | 4 ++++
> > include/hw/acpi/ich9.h | 2 ++
> > include/hw/acpi/pcihp.h | 3 ++-
> > hw/acpi/pcihp.c | 8 ++++----
> > hw/acpi/piix4.c | 4 +++-
> > hw/i386/acpi-build.c | 27 ++++++++++++++++-----------
> > 6 files changed, 31 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > index 74df5fc612..487ec7710f 100644
> > --- a/hw/i386/acpi-build.h
> > +++ b/hw/i386/acpi-build.h
> > @@ -5,6 +5,10 @@
> >
> > extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> >
> > +/* PCI Hot-plug registers bases. See docs/spec/acpi_pci_hotplug.txt */
> > +#define ACPI_PCIHP_SEJ_BASE 0x8
> > +#define ACPI_PCIHP_BNMR_BASE 0x10
> > +
> > void acpi_setup(void);
> >
> > #endif
> > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > index 28a53181cb..4d19571ed7 100644
> > --- a/include/hw/acpi/ich9.h
> > +++ b/include/hw/acpi/ich9.h
> > @@ -28,6 +28,8 @@
> > #include "hw/acpi/acpi_dev_interface.h"
> > #include "hw/acpi/tco.h"
> >
> > +#define ACPI_PCIHP_ADDR_ICH9 0x0cc4
> > +
> > typedef struct ICH9LPCPMRegs {
> > /*
> > * In ich9 spec says that pm1_cnt register is 32bit width and
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > index 02f4665767..ce49fb03b9 100644
> > --- a/include/hw/acpi/pcihp.h
> > +++ b/include/hw/acpi/pcihp.h
> > @@ -54,7 +54,8 @@ typedef struct AcpiPciHpState {
> > } AcpiPciHpState;
> >
> > void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
> > - MemoryRegion *address_space_io, bool bridges_enabled);
> > + MemoryRegion *address_space_io, bool bridges_enabled,
> > + uint16_t io_base);
> >
> > void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp);
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index ff23104aea..bb457bc279 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -38,7 +38,6 @@
> > #include "qom/qom-qobject.h"
> > #include "trace.h"
> >
> > -#define ACPI_PCIHP_ADDR 0xae00
> > #define ACPI_PCIHP_SIZE 0x0014
> > #define PCI_UP_BASE 0x0000
> > #define PCI_DOWN_BASE 0x0004
> > @@ -381,12 +380,13 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
> > };
> >
> > void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> > - MemoryRegion *address_space_io, bool bridges_enabled)
> > + MemoryRegion *address_space_io, bool bridges_enabled,
> > + uint16_t io_base)
> > {
> > s->io_len = ACPI_PCIHP_SIZE;
> > - s->io_base = ACPI_PCIHP_ADDR;
> > + s->io_base = io_base;
>
> Maybe you want to remove ACPI_PCIHP_ADDR ?
It is removed (a bit higher in this patch).
> > - s->root= root_bus;
> > + s->root = root_bus;
> > s->legacy_piix = !bridges_enabled;
> >
> > memory_region_init_io(&s->io, owner, &acpi_pcihp_io_ops, s,
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 832f8fba82..a505ab5bcf 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -50,6 +50,8 @@
> > #define GPE_BASE 0xafe0
> > #define GPE_LEN 4
> >
> > +#define ACPI_PCIHP_ADDR_PIIX4 0xae00
> > +
> > struct pci_status {
> > uint32_t up; /* deprecated, maintained for migration compatibility */
> > uint32_t down;
> > @@ -597,7 +599,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> >
> > acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > - s->use_acpi_hotplug_bridge);
> > + s->use_acpi_hotplug_bridge, ACPI_PCIHP_ADDR_PIIX4);
> >
> > s->cpu_hotplug_legacy = true;
> > object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 0e0535d2e3..cf503b16af 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> > /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> > pm->fadt.rev = 1;
> > pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > - pm->pcihp_io_base =
> > - object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > - pm->pcihp_io_len =
> > - object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> > }
> > if (lpc) {
> > struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> > pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> > pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> > }
> > + pm->pcihp_io_base =
> > + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > + pm->pcihp_io_len =
> > + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >
> > /* The above need not be conditional on machine type because the reset port
> > * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > @@ -472,7 +472,7 @@ 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)) {
> > + if (pci_bus_is_root(sec)) {
> > continue;
> > }
> >
> > @@ -1368,7 +1368,7 @@ static void build_piix4_isa_bridge(Aml *table)
> > aml_append(table, scope);
> > }
> >
> > -static void build_piix4_pci_hotplug(Aml *table)
> > +static void build_x86_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> > {
> > Aml *scope;
> > Aml *field;
> > @@ -1377,20 +1377,22 @@ static void build_piix4_pci_hotplug(Aml *table)
> > scope = aml_scope("_SB.PCI0");
> >
> > aml_append(scope,
> > - aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x08));
> > + aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(pcihp_addr), 0x08));
> > field = aml_field("PCST", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> > aml_append(field, aml_named_field("PCIU", 32));
> > aml_append(field, aml_named_field("PCID", 32));
> > aml_append(scope, field);
> >
> > aml_append(scope,
> > - aml_operation_region("SEJ", AML_SYSTEM_IO, aml_int(0xae08), 0x04));
> > + aml_operation_region("SEJ", AML_SYSTEM_IO,
> > + aml_int(pcihp_addr + ACPI_PCIHP_SEJ_BASE), 0x04));
> > field = aml_field("SEJ", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> > aml_append(field, aml_named_field("B0EJ", 32));
> > aml_append(scope, field);
> >
> > aml_append(scope,
> > - aml_operation_region("BNMR", AML_SYSTEM_IO, aml_int(0xae10), 0x04));
> > + aml_operation_region("BNMR", AML_SYSTEM_IO,
> > + aml_int(pcihp_addr + ACPI_PCIHP_BNMR_BASE), 0x04));
> > field = aml_field("BNMR", AML_DWORD_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> > aml_append(field, aml_named_field("BNUM", 32));
> > aml_append(scope, field);
> > @@ -1504,7 +1506,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > build_hpet_aml(dsdt);
> > build_piix4_isa_bridge(dsdt);
> > build_isa_devices_aml(dsdt);
> > - build_piix4_pci_hotplug(dsdt);
> > + build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
>
> This will conflict here with my patch. I think you need to do something
> like this:
>
> if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
Yes, and I change it if your patch set gets merged first. Otherwise,
you will need to rebase.
> You'll get this from my patch.
> > build_piix4_pci0_int(dsdt);
> > } else {
> > sb_scope = aml_scope("_SB");
> > @@ -1520,6 +1522,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > build_hpet_aml(dsdt);
> > build_q35_isa_bridge(dsdt);
> > build_isa_devices_aml(dsdt);
> > + if (pm->pcihp_bridge_en) {
> > + build_x86_pci_hotplug(dsdt, pm->pcihp_io_base);
> > + }
>
> ditto as above.
>
> > build_q35_pci0_int(dsdt);
> > if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> > build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> > @@ -1546,7 +1551,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > {
> > aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
> >
> > - if (misc->is_piix4) {
> > + if (misc->is_piix4 || pm->pcihp_bridge_en) {
> > method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> > aml_append(method,
> > aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
> > --
> > 2.25.4
> >
> >
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
2020-09-24 7:00 ` [RFC PATCH v3 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 Julia Suvorova
2020-09-24 7:00 ` [RFC PATCH v3 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 Julia Suvorova
@ 2020-09-24 7:00 ` Julia Suvorova
2020-09-24 7:36 ` Michael S. Tsirkin
2020-09-24 11:14 ` Igor Mammedov
2020-09-24 7:00 ` [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
` (7 subsequent siblings)
10 siblings, 2 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
Instead of changing the hot-plug type in _OSC register, do not
initialize the slot capability or set the 'Slot Implemented' flag.
This way guest will choose ACPI hot-plug if it is preferred and leave
the option to use SHPC with pcie-pci-bridge.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
hw/i386/acpi-build.h | 2 ++
hw/i386/acpi-build.c | 2 +-
hw/pci/pcie.c | 16 ++++++++++++++++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 487ec7710f..4c5bfb3d0b 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
void acpi_setup(void);
+Object *object_resolve_type_unambiguous(const char *typename);
+
#endif
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index cf503b16af..b7811a8912 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
*data = fadt;
}
-static Object *object_resolve_type_unambiguous(const char *typename)
+Object *object_resolve_type_unambiguous(const char *typename)
{
bool ambig;
Object *o = object_resolve_path_type("", typename, &ambig);
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 5b48bae0f6..c1a082e8b9 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -27,6 +27,8 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pcie_regs.h"
#include "hw/pci/pcie_port.h"
+#include "hw/i386/ich9.h"
+#include "hw/i386/acpi-build.h"
#include "qemu/range.h"
//#define DEBUG_PCIE
@@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
pcie_cap_slot_push_attention_button(hotplug_pdev);
}
+static bool acpi_pcihp_enabled(void)
+{
+ Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
+
+ return lpc &&
+ object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support",
+ NULL);
+
+}
+
/* pci express slot for pci express root/downstream port
PCI express capability slot registers */
void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
{
uint32_t pos = dev->exp.exp_cap;
+ if (acpi_pcihp_enabled()) {
+ return;
+ }
+
pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
PCI_EXP_FLAGS_SLOT);
--
2.25.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used
2020-09-24 7:00 ` [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used Julia Suvorova
@ 2020-09-24 7:36 ` Michael S. Tsirkin
2020-09-24 8:23 ` Julia Suvorova
2020-09-24 11:14 ` Igor Mammedov
1 sibling, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-09-24 7:36 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, qemu-devel
On Thu, Sep 24, 2020 at 09:00:09AM +0200, Julia Suvorova wrote:
> Instead of changing the hot-plug type in _OSC register, do not
> initialize the slot capability or set the 'Slot Implemented' flag.
> This way guest will choose ACPI hot-plug if it is preferred and leave
> the option to use SHPC with pcie-pci-bridge.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/i386/acpi-build.h | 2 ++
> hw/i386/acpi-build.c | 2 +-
> hw/pci/pcie.c | 16 ++++++++++++++++
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 487ec7710f..4c5bfb3d0b 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>
> void acpi_setup(void);
>
> +Object *object_resolve_type_unambiguous(const char *typename);
> +
> #endif
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index cf503b16af..b7811a8912 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
> *data = fadt;
> }
>
> -static Object *object_resolve_type_unambiguous(const char *typename)
> +Object *object_resolve_type_unambiguous(const char *typename)
> {
> bool ambig;
> Object *o = object_resolve_path_type("", typename, &ambig);
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 5b48bae0f6..c1a082e8b9 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -27,6 +27,8 @@
> #include "hw/pci/pci_bus.h"
> #include "hw/pci/pcie_regs.h"
> #include "hw/pci/pcie_port.h"
> +#include "hw/i386/ich9.h"
> +#include "hw/i386/acpi-build.h"
> #include "qemu/range.h"
>
> //#define DEBUG_PCIE
Not really happy with pcie.c getting an i386 dependency.
> @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> pcie_cap_slot_push_attention_button(hotplug_pdev);
> }
>
> +static bool acpi_pcihp_enabled(void)
> +{
> + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
> +
> + return lpc &&
> + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support",
> + NULL);
> +
> +}
> +
Why not just check the property unconditionally?
> /* pci express slot for pci express root/downstream port
> PCI express capability slot registers */
> void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
> {
> uint32_t pos = dev->exp.exp_cap;
>
> + if (acpi_pcihp_enabled()) {
> + return;
> + }
> +
I think I would rather not teach pcie about acpi. How about we
change the polarity, name the property
"pci-native-hotplug" or whatever makes sense.
> pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
> PCI_EXP_FLAGS_SLOT);
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used
2020-09-24 7:36 ` Michael S. Tsirkin
@ 2020-09-24 8:23 ` Julia Suvorova
2020-09-24 9:02 ` Michael S. Tsirkin
2020-09-24 11:54 ` Ani Sinha
0 siblings, 2 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 8:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ani Sinha, Igor Mammedov, QEMU Developers
On Thu, Sep 24, 2020 at 9:36 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Sep 24, 2020 at 09:00:09AM +0200, Julia Suvorova wrote:
> > Instead of changing the hot-plug type in _OSC register, do not
> > initialize the slot capability or set the 'Slot Implemented' flag.
> > This way guest will choose ACPI hot-plug if it is preferred and leave
> > the option to use SHPC with pcie-pci-bridge.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > hw/i386/acpi-build.h | 2 ++
> > hw/i386/acpi-build.c | 2 +-
> > hw/pci/pcie.c | 16 ++++++++++++++++
> > 3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > index 487ec7710f..4c5bfb3d0b 100644
> > --- a/hw/i386/acpi-build.h
> > +++ b/hw/i386/acpi-build.h
> > @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> >
> > void acpi_setup(void);
> >
> > +Object *object_resolve_type_unambiguous(const char *typename);
> > +
> > #endif
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index cf503b16af..b7811a8912 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
> > *data = fadt;
> > }
> >
> > -static Object *object_resolve_type_unambiguous(const char *typename)
> > +Object *object_resolve_type_unambiguous(const char *typename)
> > {
> > bool ambig;
> > Object *o = object_resolve_path_type("", typename, &ambig);
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 5b48bae0f6..c1a082e8b9 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -27,6 +27,8 @@
> > #include "hw/pci/pci_bus.h"
> > #include "hw/pci/pcie_regs.h"
> > #include "hw/pci/pcie_port.h"
> > +#include "hw/i386/ich9.h"
> > +#include "hw/i386/acpi-build.h"
> > #include "qemu/range.h"
> >
> > //#define DEBUG_PCIE
>
>
> Not really happy with pcie.c getting an i386 dependency.
>
>
>
> > @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > pcie_cap_slot_push_attention_button(hotplug_pdev);
> > }
> >
> > +static bool acpi_pcihp_enabled(void)
> > +{
> > + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
> > +
> > + return lpc &&
> > + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support",
> > + NULL);
> > +
> > +}
> > +
>
> Why not just check the property unconditionally?
Ok.
> > /* pci express slot for pci express root/downstream port
> > PCI express capability slot registers */
> > void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
> > {
> > uint32_t pos = dev->exp.exp_cap;
> >
> > + if (acpi_pcihp_enabled()) {
> > + return;
> > + }
> > +
>
> I think I would rather not teach pcie about acpi. How about we
> change the polarity, name the property
> "pci-native-hotplug" or whatever makes sense.
I'd prefer not to change the property name since the common code in
hw/i386/acpi-build.c depends on it, but I can add a new one if it
makes any sense.
> > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
> > PCI_EXP_FLAGS_SLOT);
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used
2020-09-24 8:23 ` Julia Suvorova
@ 2020-09-24 9:02 ` Michael S. Tsirkin
2020-09-24 11:54 ` Ani Sinha
1 sibling, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-09-24 9:02 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, QEMU Developers
On Thu, Sep 24, 2020 at 10:23:13AM +0200, Julia Suvorova wrote:
> On Thu, Sep 24, 2020 at 9:36 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Sep 24, 2020 at 09:00:09AM +0200, Julia Suvorova wrote:
> > > Instead of changing the hot-plug type in _OSC register, do not
> > > initialize the slot capability or set the 'Slot Implemented' flag.
> > > This way guest will choose ACPI hot-plug if it is preferred and leave
> > > the option to use SHPC with pcie-pci-bridge.
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > > hw/i386/acpi-build.h | 2 ++
> > > hw/i386/acpi-build.c | 2 +-
> > > hw/pci/pcie.c | 16 ++++++++++++++++
> > > 3 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > > index 487ec7710f..4c5bfb3d0b 100644
> > > --- a/hw/i386/acpi-build.h
> > > +++ b/hw/i386/acpi-build.h
> > > @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> > >
> > > void acpi_setup(void);
> > >
> > > +Object *object_resolve_type_unambiguous(const char *typename);
> > > +
> > > #endif
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index cf503b16af..b7811a8912 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
> > > *data = fadt;
> > > }
> > >
> > > -static Object *object_resolve_type_unambiguous(const char *typename)
> > > +Object *object_resolve_type_unambiguous(const char *typename)
> > > {
> > > bool ambig;
> > > Object *o = object_resolve_path_type("", typename, &ambig);
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 5b48bae0f6..c1a082e8b9 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -27,6 +27,8 @@
> > > #include "hw/pci/pci_bus.h"
> > > #include "hw/pci/pcie_regs.h"
> > > #include "hw/pci/pcie_port.h"
> > > +#include "hw/i386/ich9.h"
> > > +#include "hw/i386/acpi-build.h"
> > > #include "qemu/range.h"
> > >
> > > //#define DEBUG_PCIE
> >
> >
> > Not really happy with pcie.c getting an i386 dependency.
> >
> >
> >
> > > @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > pcie_cap_slot_push_attention_button(hotplug_pdev);
> > > }
> > >
> > > +static bool acpi_pcihp_enabled(void)
> > > +{
> > > + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
> > > +
> > > + return lpc &&
> > > + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support",
> > > + NULL);
> > > +
> > > +}
> > > +
> >
> > Why not just check the property unconditionally?
>
> Ok.
>
> > > /* pci express slot for pci express root/downstream port
> > > PCI express capability slot registers */
> > > void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
> > > {
> > > uint32_t pos = dev->exp.exp_cap;
> > >
> > > + if (acpi_pcihp_enabled()) {
> > > + return;
> > > + }
> > > +
> >
> > I think I would rather not teach pcie about acpi. How about we
> > change the polarity, name the property
> > "pci-native-hotplug" or whatever makes sense.
>
> I'd prefer not to change the property name since the common code in
> hw/i386/acpi-build.c depends on it, but I can add a new one if it
> makes any sense.
And maybe prefix with "x-" so we don't commit to it as an
external API.
> > > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
> > > PCI_EXP_FLAGS_SLOT);
> > >
> > > --
> > > 2.25.4
> >
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used
2020-09-24 8:23 ` Julia Suvorova
2020-09-24 9:02 ` Michael S. Tsirkin
@ 2020-09-24 11:54 ` Ani Sinha
1 sibling, 0 replies; 40+ messages in thread
From: Ani Sinha @ 2020-09-24 11:54 UTC (permalink / raw)
To: Julia Suvorova
Cc: Ani Sinha, Igor Mammedov, QEMU Developers, Michael S. Tsirkin
On Thu, 24 Sep 2020, Julia Suvorova wrote:
> On Thu, Sep 24, 2020 at 9:36 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Thu, Sep 24, 2020 at 09:00:09AM +0200, Julia Suvorova wrote:
>>> Instead of changing the hot-plug type in _OSC register, do not
>>> initialize the slot capability or set the 'Slot Implemented' flag.
>>> This way guest will choose ACPI hot-plug if it is preferred and leave
>>> the option to use SHPC with pcie-pci-bridge.
>>>
>>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
>>> ---
>>> hw/i386/acpi-build.h | 2 ++
>>> hw/i386/acpi-build.c | 2 +-
>>> hw/pci/pcie.c | 16 ++++++++++++++++
>>> 3 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
>>> index 487ec7710f..4c5bfb3d0b 100644
>>> --- a/hw/i386/acpi-build.h
>>> +++ b/hw/i386/acpi-build.h
>>> @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>>>
>>> void acpi_setup(void);
>>>
>>> +Object *object_resolve_type_unambiguous(const char *typename);
>>> +
>>> #endif
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index cf503b16af..b7811a8912 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
>>> *data = fadt;
>>> }
>>>
>>> -static Object *object_resolve_type_unambiguous(const char *typename)
>>> +Object *object_resolve_type_unambiguous(const char *typename)
>>> {
>>> bool ambig;
>>> Object *o = object_resolve_path_type("", typename, &ambig);
>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>>> index 5b48bae0f6..c1a082e8b9 100644
>>> --- a/hw/pci/pcie.c
>>> +++ b/hw/pci/pcie.c
>>> @@ -27,6 +27,8 @@
>>> #include "hw/pci/pci_bus.h"
>>> #include "hw/pci/pcie_regs.h"
>>> #include "hw/pci/pcie_port.h"
>>> +#include "hw/i386/ich9.h"
>>> +#include "hw/i386/acpi-build.h"
>>> #include "qemu/range.h"
>>>
>>> //#define DEBUG_PCIE
>>
>>
>> Not really happy with pcie.c getting an i386 dependency.
>>
>>
>>
>>> @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>> pcie_cap_slot_push_attention_button(hotplug_pdev);
>>> }
>>>
>>> +static bool acpi_pcihp_enabled(void)
>>> +{
>>> + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
>>> +
>>> + return lpc &&
>>> + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support",
>>> + NULL);
>>> +
>>> +}
>>> +
>>
>> Why not just check the property unconditionally?
>
> Ok.
I do not see how that would work if lpc is NULL.
object_resolve_type_unambiguous() can return NULL, at least in theory.
>
>>> /* pci express slot for pci express root/downstream port
>>> PCI express capability slot registers */
>>> void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
>>> {
>>> uint32_t pos = dev->exp.exp_cap;
>>>
>>> + if (acpi_pcihp_enabled()) {
>>> + return;
>>> + }
>>> +
>>
>> I think I would rather not teach pcie about acpi. How about we
>> change the polarity, name the property
>> "pci-native-hotplug" or whatever makes sense.
>
> I'd prefer not to change the property name since the common code in
> hw/i386/acpi-build.c depends on it, but I can add a new one if it
> makes any sense.
>
>>> pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
>>> PCI_EXP_FLAGS_SLOT);
>>>
>>> --
>>> 2.25.4
>>
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used
2020-09-24 7:00 ` [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used Julia Suvorova
2020-09-24 7:36 ` Michael S. Tsirkin
@ 2020-09-24 11:14 ` Igor Mammedov
2020-09-24 11:37 ` Ani Sinha
1 sibling, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2020-09-24 11:14 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020 09:00:09 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> Instead of changing the hot-plug type in _OSC register, do not
> initialize the slot capability or set the 'Slot Implemented' flag.
> This way guest will choose ACPI hot-plug if it is preferred and leave
> the option to use SHPC with pcie-pci-bridge.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/i386/acpi-build.h | 2 ++
> hw/i386/acpi-build.c | 2 +-
> hw/pci/pcie.c | 16 ++++++++++++++++
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 487ec7710f..4c5bfb3d0b 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>
> void acpi_setup(void);
>
> +Object *object_resolve_type_unambiguous(const char *typename);
> +
> #endif
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index cf503b16af..b7811a8912 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
> *data = fadt;
> }
>
> -static Object *object_resolve_type_unambiguous(const char *typename)
> +Object *object_resolve_type_unambiguous(const char *typename)
> {
> bool ambig;
> Object *o = object_resolve_path_type("", typename, &ambig);
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 5b48bae0f6..c1a082e8b9 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -27,6 +27,8 @@
> #include "hw/pci/pci_bus.h"
> #include "hw/pci/pcie_regs.h"
> #include "hw/pci/pcie_port.h"
> +#include "hw/i386/ich9.h"
> +#include "hw/i386/acpi-build.h"
> #include "qemu/range.h"
>
> //#define DEBUG_PCIE
> @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> pcie_cap_slot_push_attention_button(hotplug_pdev);
> }
>
> +static bool acpi_pcihp_enabled(void)
> +{
> + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
> +
> + return lpc &&
> + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support",
> + NULL);
> +
> +}
> +
> /* pci express slot for pci express root/downstream port
> PCI express capability slot registers */
> void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
> {
> uint32_t pos = dev->exp.exp_cap;
>
> + if (acpi_pcihp_enabled()) {
> + return;
> + }
> +
> pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
> PCI_EXP_FLAGS_SLOT);
>
why do you drop all slot caps instead of disabling just "if (s->hotplug) {" branch?
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used
2020-09-24 11:14 ` Igor Mammedov
@ 2020-09-24 11:37 ` Ani Sinha
0 siblings, 0 replies; 40+ messages in thread
From: Ani Sinha @ 2020-09-24 11:37 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Ani Sinha, Julia Suvorova, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020, Igor Mammedov wrote:
> On Thu, 24 Sep 2020 09:00:09 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
>> Instead of changing the hot-plug type in _OSC register, do not
>> initialize the slot capability or set the 'Slot Implemented' flag.
>> This way guest will choose ACPI hot-plug if it is preferred and leave
>> the option to use SHPC with pcie-pci-bridge.
>>
>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
>> ---
>> hw/i386/acpi-build.h | 2 ++
>> hw/i386/acpi-build.c | 2 +-
>> hw/pci/pcie.c | 16 ++++++++++++++++
>> 3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
>> index 487ec7710f..4c5bfb3d0b 100644
>> --- a/hw/i386/acpi-build.h
>> +++ b/hw/i386/acpi-build.h
>> @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>>
>> void acpi_setup(void);
>>
>> +Object *object_resolve_type_unambiguous(const char *typename);
>> +
>> #endif
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index cf503b16af..b7811a8912 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
>> *data = fadt;
>> }
>>
>> -static Object *object_resolve_type_unambiguous(const char *typename)
>> +Object *object_resolve_type_unambiguous(const char *typename)
>> {
>> bool ambig;
>> Object *o = object_resolve_path_type("", typename, &ambig);
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 5b48bae0f6..c1a082e8b9 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -27,6 +27,8 @@
>> #include "hw/pci/pci_bus.h"
>> #include "hw/pci/pcie_regs.h"
>> #include "hw/pci/pcie_port.h"
>> +#include "hw/i386/ich9.h"
>> +#include "hw/i386/acpi-build.h"
>> #include "qemu/range.h"
>>
>> //#define DEBUG_PCIE
>> @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
>> pcie_cap_slot_push_attention_button(hotplug_pdev);
>> }
>>
>> +static bool acpi_pcihp_enabled(void)
>> +{
>> + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
>> +
>> + return lpc &&
>> + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support",
>> + NULL);
>> +
>> +}
>> +
>> /* pci express slot for pci express root/downstream port
>> PCI express capability slot registers */
>> void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
>> {
>> uint32_t pos = dev->exp.exp_cap;
>>
>> + if (acpi_pcihp_enabled()) {
>> + return;
>> + }
>> +
>> pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
>> PCI_EXP_FLAGS_SLOT);
>>
> why do you drop all slot caps instead of disabling just "if (s->hotplug) {" branch?
+1 to Igor's suggestion.
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (2 preceding siblings ...)
2020-09-24 7:00 ` [RFC PATCH v3 3/7] hw/pci/pcie: Do not initialize slot capability if acpihp is used Julia Suvorova
@ 2020-09-24 7:00 ` Julia Suvorova
2020-09-24 7:37 ` Michael S. Tsirkin
2020-09-24 11:28 ` Ani Sinha
2020-09-24 7:00 ` [RFC PATCH v3 5/7] bios-tables-test: Allow changes in DSDT ACPI tables Julia Suvorova
` (6 subsequent siblings)
10 siblings, 2 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
Add acpi_pcihp to ich9_pm as part of
'acpi-pci-hotplug-with-bridge-support' option. Set default to false.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
hw/i386/acpi-build.h | 1 +
include/hw/acpi/ich9.h | 3 ++
hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++++++++++++++
hw/acpi/pcihp.c | 5 +++-
hw/i386/acpi-build.c | 2 +-
5 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 4c5bfb3d0b..39f143830a 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -10,6 +10,7 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
#define ACPI_PCIHP_BNMR_BASE 0x10
void acpi_setup(void);
+Object *acpi_get_i386_pci_host(void);
Object *object_resolve_type_unambiguous(const char *typename);
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 4d19571ed7..833e62fefe 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -24,6 +24,7 @@
#include "hw/acpi/acpi.h"
#include "hw/acpi/cpu_hotplug.h"
#include "hw/acpi/cpu.h"
+#include "hw/acpi/pcihp.h"
#include "hw/acpi/memory_hotplug.h"
#include "hw/acpi/acpi_dev_interface.h"
#include "hw/acpi/tco.h"
@@ -55,6 +56,8 @@ typedef struct ICH9LPCPMRegs {
AcpiCpuHotplug gpe_cpu;
CPUHotplugState cpuhp_state;
+ bool use_acpi_hotplug_bridge;
+ AcpiPciHpState acpi_pci_hotplug;
MemHotplugState acpi_memory_hotplug;
uint8_t disable_s3;
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 6a19070cec..987f23e388 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -218,6 +218,26 @@ static const VMStateDescription vmstate_cpuhp_state = {
}
};
+static bool vmstate_test_use_pcihp(void *opaque)
+{
+ ICH9LPCPMRegs *s = opaque;
+
+ return s->use_acpi_hotplug_bridge;
+}
+
+static const VMStateDescription vmstate_pcihp_state = {
+ .name = "ich9_pm/pcihp",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = vmstate_test_use_pcihp,
+ .fields = (VMStateField[]) {
+ VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
+ ICH9LPCPMRegs,
+ NULL),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
const VMStateDescription vmstate_ich9_pm = {
.name = "ich9_pm",
.version_id = 1,
@@ -239,6 +259,7 @@ const VMStateDescription vmstate_ich9_pm = {
&vmstate_memhp_state,
&vmstate_tco_io_state,
&vmstate_cpuhp_state,
+ &vmstate_pcihp_state,
NULL
}
};
@@ -260,6 +281,7 @@ static void pm_reset(void *opaque)
}
pm->smi_en_wmask = ~0;
+ acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
acpi_update_sci(&pm->acpi_regs, pm->irq);
}
@@ -298,6 +320,18 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
pm->enable_tco = true;
acpi_pm_tco_init(&pm->tco_regs, &pm->io);
+ if (pm->use_acpi_hotplug_bridge) {
+ acpi_pcihp_init(OBJECT(lpc_pci),
+ &pm->acpi_pci_hotplug,
+ pci_get_bus(lpc_pci),
+ pci_address_space_io(lpc_pci),
+ true,
+ ACPI_PCIHP_ADDR_ICH9);
+
+ qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
+ OBJECT(lpc_pci));
+ }
+
pm->irq = sci_irq;
qemu_register_reset(pm_reset, pm);
pm->powerdown_notifier.notify = pm_powerdown_req;
@@ -369,6 +403,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
s->pm.enable_tco = value;
}
+static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
+{
+ ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+ return s->pm.use_acpi_hotplug_bridge;
+}
+
+static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
+ Error **errp)
+{
+ ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+ s->pm.use_acpi_hotplug_bridge = value;
+}
void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
{
static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
@@ -377,6 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
pm->disable_s3 = 0;
pm->disable_s4 = 0;
pm->s4_val = 2;
+ pm->use_acpi_hotplug_bridge = false;
object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
&pm->pm_io_base, OBJ_PROP_FLAG_READ);
@@ -400,6 +449,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
ich9_pm_get_enable_tco,
ich9_pm_set_enable_tco);
+ object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
+ ich9_pm_get_acpi_pci_hotplug,
+ ich9_pm_set_acpi_pci_hotplug);
}
void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -407,6 +459,11 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
{
ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+ acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
+ return;
+ }
+
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
!lpc->pm.acpi_memory_hotplug.is_enabled)
error_setg(errp,
@@ -432,6 +489,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
} else {
acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
}
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+ acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
+ dev, errp);
} else {
error_setg(errp, "acpi: device plug request for not supported device"
" type: %s", object_get_typename(OBJECT(dev)));
@@ -452,6 +512,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
!lpc->pm.cpu_hotplug_legacy) {
acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+ acpi_pcihp_device_unplug_request_cb(hotplug_dev,
+ &lpc->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)));
@@ -469,6 +533,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
!lpc->pm.cpu_hotplug_legacy) {
acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+ acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
+ dev, errp);
} else {
error_setg(errp, "acpi: device unplug for not supported device"
" type: %s", object_get_typename(OBJECT(dev)));
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index bb457bc279..8ab65502ce 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -30,6 +30,8 @@
#include "hw/pci-host/i440fx.h"
#include "hw/pci/pci.h"
#include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_host.h"
+#include "hw/i386/acpi-build.h"
#include "hw/acpi/acpi.h"
#include "exec/address-spaces.h"
#include "hw/pci/pci_bus.h"
@@ -88,6 +90,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
static void acpi_set_pci_info(void)
{
static bool bsel_is_set;
+ Object *host = acpi_get_i386_pci_host();
PCIBus *bus;
unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
@@ -96,7 +99,7 @@ static void acpi_set_pci_info(void)
}
bsel_is_set = true;
- bus = find_i440fx(); /* TODO: Q35 support */
+ bus = PCI_HOST_BRIDGE(host)->bus;
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);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b7811a8912..8787b6fb33 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -270,7 +270,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
* Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
* On i386 arch we only have two pci hosts, so we can look only for them.
*/
-static Object *acpi_get_i386_pci_host(void)
+Object *acpi_get_i386_pci_host(void)
{
PCIHostState *host;
--
2.25.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug
2020-09-24 7:00 ` [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
@ 2020-09-24 7:37 ` Michael S. Tsirkin
2020-09-24 11:28 ` Ani Sinha
1 sibling, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-09-24 7:37 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, qemu-devel
On Thu, Sep 24, 2020 at 09:00:10AM +0200, Julia Suvorova wrote:
> Add acpi_pcihp to ich9_pm as part of
> 'acpi-pci-hotplug-with-bridge-support' option. Set default to false.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/i386/acpi-build.h | 1 +
> include/hw/acpi/ich9.h | 3 ++
> hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++++++++++++++
> hw/acpi/pcihp.c | 5 +++-
> hw/i386/acpi-build.c | 2 +-
> 5 files changed, 76 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 4c5bfb3d0b..39f143830a 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -10,6 +10,7 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> #define ACPI_PCIHP_BNMR_BASE 0x10
>
> void acpi_setup(void);
> +Object *acpi_get_i386_pci_host(void);
>
> Object *object_resolve_type_unambiguous(const char *typename);
>
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 4d19571ed7..833e62fefe 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -24,6 +24,7 @@
> #include "hw/acpi/acpi.h"
> #include "hw/acpi/cpu_hotplug.h"
> #include "hw/acpi/cpu.h"
> +#include "hw/acpi/pcihp.h"
> #include "hw/acpi/memory_hotplug.h"
> #include "hw/acpi/acpi_dev_interface.h"
> #include "hw/acpi/tco.h"
> @@ -55,6 +56,8 @@ typedef struct ICH9LPCPMRegs {
> AcpiCpuHotplug gpe_cpu;
> CPUHotplugState cpuhp_state;
>
> + bool use_acpi_hotplug_bridge;
> + AcpiPciHpState acpi_pci_hotplug;
> MemHotplugState acpi_memory_hotplug;
>
> uint8_t disable_s3;
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 6a19070cec..987f23e388 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -218,6 +218,26 @@ static const VMStateDescription vmstate_cpuhp_state = {
> }
> };
>
> +static bool vmstate_test_use_pcihp(void *opaque)
> +{
> + ICH9LPCPMRegs *s = opaque;
> +
> + return s->use_acpi_hotplug_bridge;
> +}
> +
> +static const VMStateDescription vmstate_pcihp_state = {
> + .name = "ich9_pm/pcihp",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = vmstate_test_use_pcihp,
> + .fields = (VMStateField[]) {
> + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
> + ICH9LPCPMRegs,
> + NULL),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> const VMStateDescription vmstate_ich9_pm = {
> .name = "ich9_pm",
> .version_id = 1,
> @@ -239,6 +259,7 @@ const VMStateDescription vmstate_ich9_pm = {
> &vmstate_memhp_state,
> &vmstate_tco_io_state,
> &vmstate_cpuhp_state,
> + &vmstate_pcihp_state,
> NULL
> }
> };
> @@ -260,6 +281,7 @@ static void pm_reset(void *opaque)
> }
> pm->smi_en_wmask = ~0;
>
> + acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
> acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
>
> @@ -298,6 +320,18 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> pm->enable_tco = true;
> acpi_pm_tco_init(&pm->tco_regs, &pm->io);
>
> + if (pm->use_acpi_hotplug_bridge) {
> + acpi_pcihp_init(OBJECT(lpc_pci),
> + &pm->acpi_pci_hotplug,
> + pci_get_bus(lpc_pci),
> + pci_address_space_io(lpc_pci),
> + true,
> + ACPI_PCIHP_ADDR_ICH9);
> +
> + qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
> + OBJECT(lpc_pci));
> + }
> +
> pm->irq = sci_irq;
> qemu_register_reset(pm_reset, pm);
> pm->powerdown_notifier.notify = pm_powerdown_req;
> @@ -369,6 +403,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
> s->pm.enable_tco = value;
> }
>
> +static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
> +{
> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> + return s->pm.use_acpi_hotplug_bridge;
> +}
> +
> +static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
> + Error **errp)
> +{
> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> + s->pm.use_acpi_hotplug_bridge = value;
> +}
> void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> {
> static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> @@ -377,6 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> pm->disable_s3 = 0;
> pm->disable_s4 = 0;
> pm->s4_val = 2;
> + pm->use_acpi_hotplug_bridge = false;
>
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> @@ -400,6 +449,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
> ich9_pm_get_enable_tco,
> ich9_pm_set_enable_tco);
> + object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> + ich9_pm_get_acpi_pci_hotplug,
> + ich9_pm_set_acpi_pci_hotplug);
> }
>
> void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
I guess the property name would make sense in pcie.h or something like
this.
> @@ -407,6 +459,11 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> + return;
> + }
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> !lpc->pm.acpi_memory_hotplug.is_enabled)
> error_setg(errp,
> @@ -432,6 +489,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> } else {
> acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
> }
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> + dev, errp);
> } else {
> error_setg(errp, "acpi: device plug request for not supported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> @@ -452,6 +512,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> !lpc->pm.cpu_hotplug_legacy) {
> acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
> dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_unplug_request_cb(hotplug_dev,
> + &lpc->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)));
> @@ -469,6 +533,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
> !lpc->pm.cpu_hotplug_legacy) {
> acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> + dev, errp);
> } else {
> error_setg(errp, "acpi: device unplug for not supported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index bb457bc279..8ab65502ce 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -30,6 +30,8 @@
> #include "hw/pci-host/i440fx.h"
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_bridge.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/i386/acpi-build.h"
> #include "hw/acpi/acpi.h"
> #include "exec/address-spaces.h"
> #include "hw/pci/pci_bus.h"
> @@ -88,6 +90,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> static void acpi_set_pci_info(void)
> {
> static bool bsel_is_set;
> + Object *host = acpi_get_i386_pci_host();
> PCIBus *bus;
> unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
>
> @@ -96,7 +99,7 @@ static void acpi_set_pci_info(void)
> }
> bsel_is_set = true;
>
> - bus = find_i440fx(); /* TODO: Q35 support */
> + bus = PCI_HOST_BRIDGE(host)->bus;
> 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);
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b7811a8912..8787b6fb33 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -270,7 +270,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> * On i386 arch we only have two pci hosts, so we can look only for them.
> */
> -static Object *acpi_get_i386_pci_host(void)
> +Object *acpi_get_i386_pci_host(void)
> {
> PCIHostState *host;
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug
2020-09-24 7:00 ` [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
2020-09-24 7:37 ` Michael S. Tsirkin
@ 2020-09-24 11:28 ` Ani Sinha
2020-09-24 14:27 ` Julia Suvorova
1 sibling, 1 reply; 40+ messages in thread
From: Ani Sinha @ 2020-09-24 11:28 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020, Julia Suvorova wrote:
> Add acpi_pcihp to ich9_pm as part of
> 'acpi-pci-hotplug-with-bridge-support' option. Set default to false.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/i386/acpi-build.h | 1 +
> include/hw/acpi/ich9.h | 3 ++
> hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++++++++++++++
> hw/acpi/pcihp.c | 5 +++-
> hw/i386/acpi-build.c | 2 +-
> 5 files changed, 76 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 4c5bfb3d0b..39f143830a 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -10,6 +10,7 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> #define ACPI_PCIHP_BNMR_BASE 0x10
>
> void acpi_setup(void);
> +Object *acpi_get_i386_pci_host(void);
>
> Object *object_resolve_type_unambiguous(const char *typename);
>
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 4d19571ed7..833e62fefe 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -24,6 +24,7 @@
> #include "hw/acpi/acpi.h"
> #include "hw/acpi/cpu_hotplug.h"
> #include "hw/acpi/cpu.h"
> +#include "hw/acpi/pcihp.h"
> #include "hw/acpi/memory_hotplug.h"
> #include "hw/acpi/acpi_dev_interface.h"
> #include "hw/acpi/tco.h"
> @@ -55,6 +56,8 @@ typedef struct ICH9LPCPMRegs {
> AcpiCpuHotplug gpe_cpu;
> CPUHotplugState cpuhp_state;
>
> + bool use_acpi_hotplug_bridge;
> + AcpiPciHpState acpi_pci_hotplug;
> MemHotplugState acpi_memory_hotplug;
>
> uint8_t disable_s3;
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 6a19070cec..987f23e388 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -218,6 +218,26 @@ static const VMStateDescription vmstate_cpuhp_state = {
> }
> };
>
> +static bool vmstate_test_use_pcihp(void *opaque)
> +{
> + ICH9LPCPMRegs *s = opaque;
> +
> + return s->use_acpi_hotplug_bridge;
> +}
> +
> +static const VMStateDescription vmstate_pcihp_state = {
> + .name = "ich9_pm/pcihp",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = vmstate_test_use_pcihp,
> + .fields = (VMStateField[]) {
> + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
> + ICH9LPCPMRegs,
> + NULL),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> const VMStateDescription vmstate_ich9_pm = {
> .name = "ich9_pm",
> .version_id = 1,
> @@ -239,6 +259,7 @@ const VMStateDescription vmstate_ich9_pm = {
> &vmstate_memhp_state,
> &vmstate_tco_io_state,
> &vmstate_cpuhp_state,
> + &vmstate_pcihp_state,
> NULL
> }
> };
> @@ -260,6 +281,7 @@ static void pm_reset(void *opaque)
> }
> pm->smi_en_wmask = ~0;
>
> + acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
Maybe add a comment here that acpi based hotplug is disabled both for
PCIE.0 and for the bridges.
> acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
>
> @@ -298,6 +320,18 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> pm->enable_tco = true;
> acpi_pm_tco_init(&pm->tco_regs, &pm->io);
>
> + if (pm->use_acpi_hotplug_bridge) {
> + acpi_pcihp_init(OBJECT(lpc_pci),
> + &pm->acpi_pci_hotplug,
> + pci_get_bus(lpc_pci),
> + pci_address_space_io(lpc_pci),
> + true,
> + ACPI_PCIHP_ADDR_ICH9);
> +
> + qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
> + OBJECT(lpc_pci));
> + }
> +
> pm->irq = sci_irq;
> qemu_register_reset(pm_reset, pm);
> pm->powerdown_notifier.notify = pm_powerdown_req;
> @@ -369,6 +403,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
> s->pm.enable_tco = value;
> }
>
> +static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
> +{
> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> + return s->pm.use_acpi_hotplug_bridge;
> +}
> +
> +static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
> + Error **errp)
> +{
> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> + s->pm.use_acpi_hotplug_bridge = value;
> +}
> void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> {
> static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> @@ -377,6 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> pm->disable_s3 = 0;
> pm->disable_s4 = 0;
> pm->s4_val = 2;
> + pm->use_acpi_hotplug_bridge = false;
Ditto regarding the comment as above.
>
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> @@ -400,6 +449,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
> ich9_pm_get_enable_tco,
> ich9_pm_set_enable_tco);
> + object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> + ich9_pm_get_acpi_pci_hotplug,
> + ich9_pm_set_acpi_pci_hotplug);
> }
>
> void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -407,6 +459,11 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> + return;
> + }
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> !lpc->pm.acpi_memory_hotplug.is_enabled)
> error_setg(errp,
> @@ -432,6 +489,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> } else {
> acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
> }
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> + dev, errp);
> } else {
> error_setg(errp, "acpi: device plug request for not supported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> @@ -452,6 +512,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> !lpc->pm.cpu_hotplug_legacy) {
> acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
> dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_unplug_request_cb(hotplug_dev,
> + &lpc->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)));
> @@ -469,6 +533,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
> !lpc->pm.cpu_hotplug_legacy) {
> acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> + dev, errp);
> } else {
> error_setg(errp, "acpi: device unplug for not supported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index bb457bc279..8ab65502ce 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -30,6 +30,8 @@
> #include "hw/pci-host/i440fx.h"
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_bridge.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/i386/acpi-build.h"
> #include "hw/acpi/acpi.h"
> #include "exec/address-spaces.h"
> #include "hw/pci/pci_bus.h"
> @@ -88,6 +90,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> static void acpi_set_pci_info(void)
> {
> static bool bsel_is_set;
> + Object *host = acpi_get_i386_pci_host();
> PCIBus *bus;
> unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
>
> @@ -96,7 +99,7 @@ static void acpi_set_pci_info(void)
> }
> bsel_is_set = true;
>
> - bus = find_i440fx(); /* TODO: Q35 support */
> + bus = PCI_HOST_BRIDGE(host)->bus;
Isn't this part of a differnet patch?
> 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);
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b7811a8912..8787b6fb33 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -270,7 +270,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> * On i386 arch we only have two pci hosts, so we can look only for them.
> */
> -static Object *acpi_get_i386_pci_host(void)
> +Object *acpi_get_i386_pci_host(void)
> {
> PCIHostState *host;
>
This is also part of the above change, so part of a diffent patch.
> --
> 2.25.4
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug
2020-09-24 11:28 ` Ani Sinha
@ 2020-09-24 14:27 ` Julia Suvorova
0 siblings, 0 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 14:27 UTC (permalink / raw)
To: Ani Sinha; +Cc: Igor Mammedov, QEMU Developers, Michael S. Tsirkin
On Thu, Sep 24, 2020 at 1:28 PM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Thu, 24 Sep 2020, Julia Suvorova wrote:
>
> > Add acpi_pcihp to ich9_pm as part of
> > 'acpi-pci-hotplug-with-bridge-support' option. Set default to false.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > hw/i386/acpi-build.h | 1 +
> > include/hw/acpi/ich9.h | 3 ++
> > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++++++++++++++
> > hw/acpi/pcihp.c | 5 +++-
> > hw/i386/acpi-build.c | 2 +-
> > 5 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > index 4c5bfb3d0b..39f143830a 100644
> > --- a/hw/i386/acpi-build.h
> > +++ b/hw/i386/acpi-build.h
> > @@ -10,6 +10,7 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> > #define ACPI_PCIHP_BNMR_BASE 0x10
> >
> > void acpi_setup(void);
> > +Object *acpi_get_i386_pci_host(void);
> >
> > Object *object_resolve_type_unambiguous(const char *typename);
> >
> > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > index 4d19571ed7..833e62fefe 100644
> > --- a/include/hw/acpi/ich9.h
> > +++ b/include/hw/acpi/ich9.h
> > @@ -24,6 +24,7 @@
> > #include "hw/acpi/acpi.h"
> > #include "hw/acpi/cpu_hotplug.h"
> > #include "hw/acpi/cpu.h"
> > +#include "hw/acpi/pcihp.h"
> > #include "hw/acpi/memory_hotplug.h"
> > #include "hw/acpi/acpi_dev_interface.h"
> > #include "hw/acpi/tco.h"
> > @@ -55,6 +56,8 @@ typedef struct ICH9LPCPMRegs {
> > AcpiCpuHotplug gpe_cpu;
> > CPUHotplugState cpuhp_state;
> >
> > + bool use_acpi_hotplug_bridge;
> > + AcpiPciHpState acpi_pci_hotplug;
> > MemHotplugState acpi_memory_hotplug;
> >
> > uint8_t disable_s3;
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 6a19070cec..987f23e388 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -218,6 +218,26 @@ static const VMStateDescription vmstate_cpuhp_state = {
> > }
> > };
> >
> > +static bool vmstate_test_use_pcihp(void *opaque)
> > +{
> > + ICH9LPCPMRegs *s = opaque;
> > +
> > + return s->use_acpi_hotplug_bridge;
> > +}
> > +
> > +static const VMStateDescription vmstate_pcihp_state = {
> > + .name = "ich9_pm/pcihp",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .needed = vmstate_test_use_pcihp,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
> > + ICH9LPCPMRegs,
> > + NULL),
> > + VMSTATE_END_OF_LIST()
> > + }
> > +};
> > +
> > const VMStateDescription vmstate_ich9_pm = {
> > .name = "ich9_pm",
> > .version_id = 1,
> > @@ -239,6 +259,7 @@ const VMStateDescription vmstate_ich9_pm = {
> > &vmstate_memhp_state,
> > &vmstate_tco_io_state,
> > &vmstate_cpuhp_state,
> > + &vmstate_pcihp_state,
> > NULL
> > }
> > };
> > @@ -260,6 +281,7 @@ static void pm_reset(void *opaque)
> > }
> > pm->smi_en_wmask = ~0;
> >
> > + acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
>
> Maybe add a comment here that acpi based hotplug is disabled both for
> PCIE.0 and for the bridges.
>
>
> > acpi_update_sci(&pm->acpi_regs, pm->irq);
> > }
> >
> > @@ -298,6 +320,18 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > pm->enable_tco = true;
> > acpi_pm_tco_init(&pm->tco_regs, &pm->io);
> >
> > + if (pm->use_acpi_hotplug_bridge) {
> > + acpi_pcihp_init(OBJECT(lpc_pci),
> > + &pm->acpi_pci_hotplug,
> > + pci_get_bus(lpc_pci),
> > + pci_address_space_io(lpc_pci),
> > + true,
> > + ACPI_PCIHP_ADDR_ICH9);
> > +
> > + qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
> > + OBJECT(lpc_pci));
> > + }
> > +
> > pm->irq = sci_irq;
> > qemu_register_reset(pm_reset, pm);
> > pm->powerdown_notifier.notify = pm_powerdown_req;
> > @@ -369,6 +403,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
> > s->pm.enable_tco = value;
> > }
> >
> > +static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
> > +{
> > + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > +
> > + return s->pm.use_acpi_hotplug_bridge;
> > +}
> > +
> > +static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
> > + Error **errp)
> > +{
> > + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > +
> > + s->pm.use_acpi_hotplug_bridge = value;
> > +}
> > void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > {
> > static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> > @@ -377,6 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > pm->disable_s3 = 0;
> > pm->disable_s4 = 0;
> > pm->s4_val = 2;
> > + pm->use_acpi_hotplug_bridge = false;
>
> Ditto regarding the comment as above.
>
> >
> > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> > &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> > @@ -400,6 +449,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
> > ich9_pm_get_enable_tco,
> > ich9_pm_set_enable_tco);
> > + object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> > + ich9_pm_get_acpi_pci_hotplug,
> > + ich9_pm_set_acpi_pci_hotplug);
> > }
> >
> > void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > @@ -407,6 +459,11 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > {
> > ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
> >
> > + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> > + return;
> > + }
> > +
> > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> > !lpc->pm.acpi_memory_hotplug.is_enabled)
> > error_setg(errp,
> > @@ -432,6 +489,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > } else {
> > acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
> > }
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > + dev, errp);
> > } else {
> > error_setg(errp, "acpi: device plug request for not supported device"
> > " type: %s", object_get_typename(OBJECT(dev)));
> > @@ -452,6 +512,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > !lpc->pm.cpu_hotplug_legacy) {
> > acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
> > dev, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_unplug_request_cb(hotplug_dev,
> > + &lpc->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)));
> > @@ -469,6 +533,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
> > !lpc->pm.cpu_hotplug_legacy) {
> > acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > + dev, errp);
> > } else {
> > error_setg(errp, "acpi: device unplug for not supported device"
> > " type: %s", object_get_typename(OBJECT(dev)));
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index bb457bc279..8ab65502ce 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -30,6 +30,8 @@
> > #include "hw/pci-host/i440fx.h"
> > #include "hw/pci/pci.h"
> > #include "hw/pci/pci_bridge.h"
> > +#include "hw/pci/pci_host.h"
> > +#include "hw/i386/acpi-build.h"
> > #include "hw/acpi/acpi.h"
> > #include "exec/address-spaces.h"
> > #include "hw/pci/pci_bus.h"
> > @@ -88,6 +90,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> > static void acpi_set_pci_info(void)
> > {
> > static bool bsel_is_set;
> > + Object *host = acpi_get_i386_pci_host();
> > PCIBus *bus;
> > unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> >
> > @@ -96,7 +99,7 @@ static void acpi_set_pci_info(void)
> > }
> > bsel_is_set = true;
> >
> > - bus = find_i440fx(); /* TODO: Q35 support */
> > + bus = PCI_HOST_BRIDGE(host)->bus;
>
> Isn't this part of a differnet patch?
It was, but since there is no need for a separate function, I merged
it into this patch.
> > 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);
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b7811a8912..8787b6fb33 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -270,7 +270,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> > * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> > * On i386 arch we only have two pci hosts, so we can look only for them.
> > */
> > -static Object *acpi_get_i386_pci_host(void)
> > +Object *acpi_get_i386_pci_host(void)
> > {
> > PCIHostState *host;
> >
>
> This is also part of the above change, so part of a diffent patch.
>
> > --
> > 2.25.4
> >
> >
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v3 5/7] bios-tables-test: Allow changes in DSDT ACPI tables
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (3 preceding siblings ...)
2020-09-24 7:00 ` [RFC PATCH v3 4/7] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
@ 2020-09-24 7:00 ` Julia Suvorova
2020-09-24 10:57 ` Ani Sinha
2020-09-24 7:00 ` [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default Julia Suvorova
` (5 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
All DSDT Q35 tables will be modified because ACPI hot-plug is enabled
by default.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..84f56b14db 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,11 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT",
+"tests/data/acpi/q35/DSDT.tis",
+"tests/data/acpi/q35/DSDT.bridge",
+"tests/data/acpi/q35/DSDT.mmio64",
+"tests/data/acpi/q35/DSDT.ipmibt",
+"tests/data/acpi/q35/DSDT.cphp",
+"tests/data/acpi/q35/DSDT.memhp",
+"tests/data/acpi/q35/DSDT.acpihmat",
+"tests/data/acpi/q35/DSDT.numamem",
+"tests/data/acpi/q35/DSDT.dimmpxm",
--
2.25.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 5/7] bios-tables-test: Allow changes in DSDT ACPI tables
2020-09-24 7:00 ` [RFC PATCH v3 5/7] bios-tables-test: Allow changes in DSDT ACPI tables Julia Suvorova
@ 2020-09-24 10:57 ` Ani Sinha
0 siblings, 0 replies; 40+ messages in thread
From: Ani Sinha @ 2020-09-24 10:57 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Igor Mammedov, QEMU Developers, Michael S. Tsirkin
On Thu, Sep 24, 2020 at 12:30 PM Julia Suvorova <jusual@redhat.com> wrote:
>
> All DSDT Q35 tables will be modified because ACPI hot-plug is enabled
> by default.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..84f56b14db 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,11 @@
> /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT",
> +"tests/data/acpi/q35/DSDT.tis",
> +"tests/data/acpi/q35/DSDT.bridge",
> +"tests/data/acpi/q35/DSDT.mmio64",
> +"tests/data/acpi/q35/DSDT.ipmibt",
> +"tests/data/acpi/q35/DSDT.cphp",
> +"tests/data/acpi/q35/DSDT.memhp",
> +"tests/data/acpi/q35/DSDT.acpihmat",
> +"tests/data/acpi/q35/DSDT.numamem",
> +"tests/data/acpi/q35/DSDT.dimmpxm",
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (4 preceding siblings ...)
2020-09-24 7:00 ` [RFC PATCH v3 5/7] bios-tables-test: Allow changes in DSDT ACPI tables Julia Suvorova
@ 2020-09-24 7:00 ` Julia Suvorova
2020-09-24 9:33 ` Igor Mammedov
2020-09-24 10:36 ` Daniel P. Berrangé
2020-09-24 7:00 ` [RFC PATCH v3 7/7] bios-tables-test: Update golden binaries Julia Suvorova
` (4 subsequent siblings)
10 siblings, 2 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
hw/acpi/ich9.c | 2 +-
hw/i386/pc.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 987f23e388..c67c20de4e 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -425,7 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
pm->disable_s3 = 0;
pm->disable_s4 = 0;
pm->s4_val = 2;
- pm->use_acpi_hotplug_bridge = false;
+ pm->use_acpi_hotplug_bridge = true;
object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
&pm->pm_io_base, OBJ_PROP_FLAG_READ);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b55369357e..5de4475570 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -101,6 +101,7 @@ GlobalProperty pc_compat_5_1[] = {};
const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
GlobalProperty pc_compat_5_0[] = {
+ { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
};
const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
--
2.25.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default
2020-09-24 7:00 ` [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default Julia Suvorova
@ 2020-09-24 9:33 ` Igor Mammedov
2020-09-24 9:41 ` Michael S. Tsirkin
2020-09-24 10:36 ` Daniel P. Berrangé
1 sibling, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2020-09-24 9:33 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020 09:00:12 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
I'd drop this patch for now.
mgmt can turn it on for Windows guests to workaround
its native hotplug issues.
> ---
> hw/acpi/ich9.c | 2 +-
> hw/i386/pc.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 987f23e388..c67c20de4e 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -425,7 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> pm->disable_s3 = 0;
> pm->disable_s4 = 0;
> pm->s4_val = 2;
> - pm->use_acpi_hotplug_bridge = false;
> + pm->use_acpi_hotplug_bridge = true;
>
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b55369357e..5de4475570 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -101,6 +101,7 @@ GlobalProperty pc_compat_5_1[] = {};
> const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
>
> GlobalProperty pc_compat_5_0[] = {
> + { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
> };
> const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default
2020-09-24 9:33 ` Igor Mammedov
@ 2020-09-24 9:41 ` Michael S. Tsirkin
0 siblings, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-09-24 9:41 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Ani Sinha, Julia Suvorova, qemu-devel
On Thu, Sep 24, 2020 at 11:33:45AM +0200, Igor Mammedov wrote:
> On Thu, 24 Sep 2020 09:00:12 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
>
> I'd drop this patch for now.
> mgmt can turn it on for Windows guests to workaround
> its native hotplug issues.
It's not just windows either, the hack we have in the hotplug code is an
example of a linux issue. I'd rather not let management worry about
which type of hotplug is in use. Nothing here sets any policy, this is
strictly a mechanism thing.
> > ---
> > hw/acpi/ich9.c | 2 +-
> > hw/i386/pc.c | 1 +
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 987f23e388..c67c20de4e 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -425,7 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > pm->disable_s3 = 0;
> > pm->disable_s4 = 0;
> > pm->s4_val = 2;
> > - pm->use_acpi_hotplug_bridge = false;
> > + pm->use_acpi_hotplug_bridge = true;
> >
> > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> > &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index b55369357e..5de4475570 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -101,6 +101,7 @@ GlobalProperty pc_compat_5_1[] = {};
> > const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> >
> > GlobalProperty pc_compat_5_0[] = {
> > + { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
> > };
> > const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
> >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default
2020-09-24 7:00 ` [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default Julia Suvorova
2020-09-24 9:33 ` Igor Mammedov
@ 2020-09-24 10:36 ` Daniel P. Berrangé
2020-09-24 14:24 ` Julia Suvorova
1 sibling, 1 reply; 40+ messages in thread
From: Daniel P. Berrangé @ 2020-09-24 10:36 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, qemu-devel, Michael S. Tsirkin
On Thu, Sep 24, 2020 at 09:00:12AM +0200, Julia Suvorova wrote:
There needs to be a commit message to explain / justify why this change
is a benefit. You have a nice list of pros/cons in the cover letter
which could serve as a good commit message here.
The cover letter gets discarded, only the commit message is in git
history, so its beneficial to capture that info here.
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/acpi/ich9.c | 2 +-
> hw/i386/pc.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 987f23e388..c67c20de4e 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -425,7 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> pm->disable_s3 = 0;
> pm->disable_s4 = 0;
> pm->s4_val = 2;
> - pm->use_acpi_hotplug_bridge = false;
> + pm->use_acpi_hotplug_bridge = true;
>
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b55369357e..5de4475570 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -101,6 +101,7 @@ GlobalProperty pc_compat_5_1[] = {};
> const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
>
> GlobalProperty pc_compat_5_0[] = {
> + { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
> };
> const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>
> --
> 2.25.4
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default
2020-09-24 10:36 ` Daniel P. Berrangé
@ 2020-09-24 14:24 ` Julia Suvorova
0 siblings, 0 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 14:24 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Ani Sinha, Igor Mammedov, QEMU Developers, Michael S. Tsirkin
On Thu, Sep 24, 2020 at 12:36 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Sep 24, 2020 at 09:00:12AM +0200, Julia Suvorova wrote:
>
> There needs to be a commit message to explain / justify why this change
> is a benefit. You have a nice list of pros/cons in the cover letter
> which could serve as a good commit message here.
>
> The cover letter gets discarded, only the commit message is in git
> history, so its beneficial to capture that info here.
Sure, I'll add it.
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > hw/acpi/ich9.c | 2 +-
> > hw/i386/pc.c | 1 +
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 987f23e388..c67c20de4e 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -425,7 +425,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > pm->disable_s3 = 0;
> > pm->disable_s4 = 0;
> > pm->s4_val = 2;
> > - pm->use_acpi_hotplug_bridge = false;
> > + pm->use_acpi_hotplug_bridge = true;
> >
> > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> > &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index b55369357e..5de4475570 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -101,6 +101,7 @@ GlobalProperty pc_compat_5_1[] = {};
> > const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> >
> > GlobalProperty pc_compat_5_0[] = {
> > + { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
> > };
> > const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
> >
> > --
> > 2.25.4
> >
> >
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH v3 7/7] bios-tables-test: Update golden binaries
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (5 preceding siblings ...)
2020-09-24 7:00 ` [RFC PATCH v3 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default Julia Suvorova
@ 2020-09-24 7:00 ` Julia Suvorova
2020-09-24 8:57 ` [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 no-reply
` (3 subsequent siblings)
10 siblings, 0 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-09-24 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Ani Sinha, Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
Add ACPI hot-plug registers to DSDT Q35 tables.
Changes in the tables:
+ Scope (_SB.PCI0)
+ {
+ OperationRegion (PCST, SystemIO, 0x0CC4, 0x08)
+ Field (PCST, DWordAcc, NoLock, WriteAsZeros)
+ {
+ PCIU, 32,
+ PCID, 32
+ }
+
+ OperationRegion (SEJ, SystemIO, 0x0CCC, 0x04)
+ Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
+ {
+ B0EJ, 32
+ }
+
+ OperationRegion (BNMR, SystemIO, 0x0CD4, 0x04)
+ Field (BNMR, DWordAcc, NoLock, WriteAsZeros)
+ {
+ BNUM, 32
+ }
+
+ Mutex (BLCK, 0x00)
+ Method (PCEJ, 2, NotSerialized)
+ {
+ Acquire (BLCK, 0xFFFF)
+ BNUM = Arg0
+ B0EJ = (One << Arg1)
+ Release (BLCK)
+ Return (Zero)
+ }
+ }
+
...
Scope (_GPE)
{
Name (_HID, "ACPI0006" /* GPE Block Device */) // _HID: Hardware ID
+ Method (_E01, 0, NotSerialized) // _Exx: Edge-Triggered GPE, xx=0x00-0xFF
+ {
+ Acquire (\_SB.PCI0.BLCK, 0xFFFF)
+ \_SB.PCI0.PCNT ()
+ Release (\_SB.PCI0.BLCK)
+ }
}
...
+
+ Device (PHPR)
+ {
+ Name (_HID, "PNP0A06" /* Generic Container Device */) // _HID: Hardware ID
+ Name (_UID, "PCI Hotplug resources") // _UID: Unique ID
+ Name (_STA, 0x0B) // _STA: Status
+ Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
+ {
+ IO (Decode16,
+ 0x0CC4, // Range Minimum
+ 0x0CC4, // Range Maximum
+ 0x01, // Alignment
+ 0x14, // Length
+ )
+ })
+ }
}
And if there is a port in configuration:
Device (S10)
{
Name (_ADR, 0x00020000) // _ADR: Address
+ Name (BSEL, Zero)
+ Device (S00)
+ {
+ Name (_SUN, Zero) // _SUN: Slot User Number
+ Name (_ADR, Zero) // _ADR: Address
+ Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device, x=0-9
+ {
+ PCEJ (BSEL, _SUN)
+ }
+ }
+
...
+
+ Method (DVNT, 2, NotSerialized)
+ {
+ If ((Arg0 & One))
+ {
+ Notify (S00, Arg1)
+ }
+
+ If ((Arg0 & 0x02))
+ {
+ Notify (S08, Arg1)
+ }
+
...
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 10 ----------
tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
11 files changed, 10 deletions(-)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 84f56b14db..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,11 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT",
-"tests/data/acpi/q35/DSDT.tis",
-"tests/data/acpi/q35/DSDT.bridge",
-"tests/data/acpi/q35/DSDT.mmio64",
-"tests/data/acpi/q35/DSDT.ipmibt",
-"tests/data/acpi/q35/DSDT.cphp",
-"tests/data/acpi/q35/DSDT.memhp",
-"tests/data/acpi/q35/DSDT.acpihmat",
-"tests/data/acpi/q35/DSDT.numamem",
-"tests/data/acpi/q35/DSDT.dimmpxm",
diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
index bba8884073a27427b88ac0d733c9c87330a59366..56e5b111f3239ea0af2cfb6dea962e3cd837da80 100644
GIT binary patch
delta 329
zcmexo-Dk(;66_MfC(ppZ*twDGAXB}72ZvsKuv2`1v!_9HLx6K|2qX6q9xjgPMgb7V
z87LmA03=)#q8ox;z2X_U&+u@uL^pDSIL=N6u3kV1CqLgHM(!&R2@uEG$uHDbA)3+2
z$Jv`fL^Z(K)r%=w8N~blzaRr7Sy0KC$>3zb>FO1&4iaCo`4m&6WRO2gynBEvN4$rp
z3$LSdfTw|hff<8{WxT6_Aw#rsj6O5Wtq`-21OlA>LZa1?1VAbTd_^}~$?!9JMK^h|
z1b74lK}-(t3ovj58q5(N3bY64I|YyYl7gJlbcLeS;{4L0<kVuITyTgZ7dJ<|b5O7#
UFBg|P;}M=CJd7ff-DS4}00K@~#Q*>R
delta 57
zcmeCP`)AGN66_N4PnLm!am_}qgG`%4m?I>`MdIB9Tsh)BJY9GlodY}#3=GT|1RMo7
N|C8ZooE#~)6#(|i5Rd=>
diff --git a/tests/data/acpi/q35/DSDT.acpihmat b/tests/data/acpi/q35/DSDT.acpihmat
index 9cac92418b5fcc2767dc74603d599642b59623fe..aff5e7d14fdb5b1a332dcc1866d33bff5247a996 100644
GIT binary patch
delta 305
zcmZ4Gw#$RdCD<jzN`--eF?=J}L8f{E4-UQfV5j&1XHSFZh5+Z_5Jv7JJX{>njRGK!
zGf+HK0Z6zgL^lMxdc`wxpW)$RiEiWuah#nDT)lu2PJX^YjNDfs5+IJVlV7N>LNue3
zkFz&}h-!eds~1zWGKly8e?bOFvY?VNlflV=)72|l9VEVF^C_lQxgdX*c=rHTj(87G
z7hXr_08ax012YB@%Xn7<LxyPO7=31#TOnp42?RL%g+!|(34l}x_=;{$Q{-n<k8bi}
x3GfIAf|wlO7hvEBG?*hk6lf37cM2Z)B?UR9>5~JLWZ91J9N}RUnOv;29RTA-R3QKW
delta 57
zcmdnxvC56hCD<iIOPPUzv2r8VL8i?i%q?=_BJu74t{m|mo-VwO&H<hV1_ovf0*->4
NwUqc7C$}nZ1pvgG4-fzV
diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index f08b7245f59aad491fcaa60e2bab1085c369ea1c..355c4462bb414efb83f530aa1eb9073dda899379 100644
GIT binary patch
delta 2280
zcmZ{mO-~a+7{_O~MV4tx7g*jYG(kl#xTrueCenQoTIsUswj^eWdoaY3n9zWjn38Pa
zK*GrbUx0&Oz;N?s!pV5_`{46*_bELvm!|*u|N5KR?(DSRUtd(X<yp0h$BaqlPySmD
z)lFqF@QlDUOS<27O|K@UZqi)wPgM*ijMww<V&(m|XO{yh^*7Cv{%Hz>Y3Ozt3d5;X
zrPRNuz+f6qy^{AOW6ONUv`4OKmt}t*)9r1ufo1cXw9gEEz%Kg>IR4}MkNkeJx}6L*
zTw4h?OID_6x+PuLSD3aI*!mLl=XakaXswZ}XTUX`n!nI9fP`8Zy?!;5mCF8EIq7b?
z)yV8Ru3iMO64YTHxVrr2!I$Re<4^g+y`zKU!;gDM(0R2YPbq;}^@cK>Z%8-ko3x}|
zZshiP>0}odq83jsf92kU+h4sUJy3$8RZTM{GsW=iE#{v+^mH8d>JI3jWZH0Gmvsrh
z4@9u>&z^XCA)+KwNcWRZqiNt%TqaazT6muo*x1Sw#3n@A=kYRmlF+0OC2Jp{eW8eb
zLD~nzJ`kd$?I*NfguW!J5~Cqgge++XNIM`xmr2VB<wBIKusz_mhH%qUB6fweY0{>J
zC}}f<W<=;JSqA|PJKJth$dWco+N=mIkX9vB6{2JvQo?xS4~f_{(hie$ScsB#gwPQY
zS|n?z5vvJV(vA{3Dnbp?juAR0L?dhH@Nr_tMXX8M2|_1?C@pr9&`A+$p*6$<VT#x(
zAxqk6LZ?NjO^cl&bVi64g0OSM@f@)^5&M?3vxLqHQCjRAp>y}4nv=73LZ>EQrCJS%
zo#TPKf0b@ExQw9?ha?0zp`%8@K#d9kPQR$hW1wao0vupb)5d^JrqxI>oCr>#ILRT#
zZi#cjSrV5x*>bnT+2BBkJDhH%+u?lhSjQbsxQbo2)qvwgVvynKq%3!G+~fI^!~uBD
z2v9otPM~6jCx-fbC*Sq!m|^Qv-|pnQzH`m6hpC5OxpU8q`VdzLD>(OHJ9YuQa;ryx
z*ADxt+vhR&^y`pegLL~gX6Us!{Ap)RhJPEZ9jAtWNu>CV#S@yE?EoJZ@fPFT|1AtG
AF#rGn
delta 95
zcmeD5?YH4_33dtLmt$aH^xnvIkZE%WbA+V0NW6Q1D@VMCrwgy6bAYFTfq@x=fTQ5%
ze=_`xlOyHQx%^pU;)9*y1>9LDpOUi|;}148i06oRbP3{NU|?bpVGD5f3t<2NdEgd>
diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
index 57d859cef9fa16a8f125c4b338611c8472699f38..4d66dc1c564e6b3ff2a5cc1cf9c8447a095ab9a4 100644
GIT binary patch
delta 329
zcmX?Wf7g-ACD<k8t^xxCBlAYCgG}`T9vph{!A|i3&YlL*4FS%<A&lHdc(^#C8wEfd
zXP|hf0+4V~h;9gW^@?ZYKEuPs65YrR;y61QxOxF4ocw%)7`d-NBtRTzC%;f%g=j`6
zA7^g{5!C=^S1+b$Wf1TG|AGvVWI-ikCWDg!r>j@AI!JuU=2J`>(n0<#@$Lbx9Pu8W
zF1(J;0iFg124)N*mhr9zh78flG5X9fw?fQ95(sei3yD@o5&)?X@D<(6DaX&~72V{;
z65tUK1Ti_lFTlVNXfQ{7D9|3D?-V@pOA2yI(-n$Ri}Op1l2eO;a={^vT-+S-&OyO~
Uyj)!Fj7NBm@Gy!@7L(r&0AY4nrvLx|
delta 57
zcmccXc-EfFCD<k8tULn)qv}SkgG`%4nAN4lMdIB9Tsh)BJY9GlodY}#3=GT|1RMo7
NpOxchoUEa+6#&;`4{`ti
diff --git a/tests/data/acpi/q35/DSDT.dimmpxm b/tests/data/acpi/q35/DSDT.dimmpxm
index 9d5bd5744e2ba2e0f6126c3aba0bb36af865e499..050533a0353f38cdb4d23fd52898dd4ce1aef9a9 100644
GIT binary patch
delta 310
zcmezD(d^CT66_MvtjfT^_-Z59L8f{E4-UQfV5j&1XHSFZh5+Z_5Jv7JJX{>njRGK!
zGf+HK0Z6zgL^lMxdc`wxpW)$RiEiWuah#nDT)lu2PJX^YjNDfs5+IJVlV7N>LNue3
zkFz&}h-!eds~1zWGKly8e?bOFvY?VNlflV=)72|l9VEVF^C_lX@@dX&@$Lbx9Pu8W
zF1(J;0iFg124)N*mhr9zh78flG5X9fw?fQ95(sei3yD@o5&)?X5eDisVQ7m1`_V7V
zC5Wp*z*lthdL@2Fj_4*YmH>}{pvjHOBHTbPDtP3V6y%hqPoAkP%XWn42oIyk<UPvU
E0s3xOTmS$7
delta 77
zcmZqn{_Mf!66_LEtir&+_--TDL8i?i%sb_^HR9a^Tsh)BJY9GlodY}#3=GT|M1<pA
h4NMr?V)WvJo#Oq%T!Oe71RMo77c28KPCloy6##DT7A^n)
diff --git a/tests/data/acpi/q35/DSDT.ipmibt b/tests/data/acpi/q35/DSDT.ipmibt
index 5cd11de6a8fe47324e5f922823a22746882f19f5..31eeedfb978aaeb469e442ae9748dd62a5285bd2 100644
GIT binary patch
delta 301
zcmX?UbJLE?CD<h-Ql5c<aqmVh9_D%h4-UQfV5j&1XHSFZh5+Z_5Jv7JJX{>njRGK!
zGf+HK0Z6zgL^lMxdc`wxpW)$RiEiWuah#nDT)lu2PJX^YjNDfs5+IJVlV7N>LNue3
zkFz&}h-!eds~1zWGKly8e?bOFvY?VNlflV=)72|l9VEVFvnX?<WRO2gynBEvN4$rp
z3$LSdfTw|hff<8{WxT6_Aw#rsj6O5Wtq`-21OlA>LZa1?1VAbTd__06%J4JFM>lz~
t1b74lK}-(t3ovj58q5(N3bY64yUBu*!jr3Hh1ibp9N}RUnLJ5$I{*hoQAGd%
delta 57
zcmca<chZK-CD<jzQ;vaw@%~0G9_G#SnIj~{MdIB9Tsh)BJY9GlodY}#3=GT|1RMo7
Nd&=@NPF^Cn6#&f^4^#jE
diff --git a/tests/data/acpi/q35/DSDT.memhp b/tests/data/acpi/q35/DSDT.memhp
index 05a7a73ec43130d5c3018bb462fd84981bfb151c..798a12399399ebb163b8231ef54d5e566d8dfc88 100644
GIT binary patch
delta 329
zcmX@>cGrW;CD<h-R)v9qv3eudL8f{E4-UQfV5j&1XHSFZh5+Z_5Jv7JJX{>njRGK!
zGf+HK0Z6zgL^lMxdc`wxpW)$RiEiWuah#nDT)lu2PJX^YjNDfs5+IJVlV7N>LNue3
zkFz&}h-!eds~1zWGKly8e?bOFvY?VNlflV=)72|l9VEVF^C_kkazXwq@$Lbx9Pu8W
zF1(J;0iFg124)N*mhr9zh78flG5X9fw?fQ95(sei3yD@o5&)?X@D<(MsmRah72V{;
z65tUK1Ti_lFTlVNXfQ{7D9|3D?-V@pOA2yI(-n$Ri}Op1l2eO;a={^vT-+S-&OyO~
Uyj)!Fj7NBm@Gy!@o~E=N0Pb*GIsgCw
delta 57
zcmccXan_B?CD<jzSDAr<aqdR0gG`%4n3v0mi^RJJxN^jMc)IX9ItO?f7#Nr_2sjFE
N_Eq9%oV-GLD*)!v5C#AM
diff --git a/tests/data/acpi/q35/DSDT.mmio64 b/tests/data/acpi/q35/DSDT.mmio64
index efd3f1188f2b55da1514212d4be081a61c2a96e9..61e0515c6007ec2d16db57e8f6052376c6467359 100644
GIT binary patch
delta 305
zcmaFi^23eGCD<jTLYaYq(R?G<L8f{E4-UQfV5j&1XHSFZh5+Z_5Jv7JJX{>njRGK!
zGf+HK0Z6zgL^lMxdc`wxpW)$RiEiWuah#nDT)lu2PJX^YjNDfs5+IJVlV7N>LNue3
zkFz&}h-!eds~1zWGKly8e?bOFvY?VNlflV=)72|l9VEVF^C_lHvO)eV@$Lbx9Pu8W
zF1(J;0iFg124)N*mhr9zh78flG5X9fw?fQ95(sei3yD@o5&)?X@D<%WS%IHXJ-W$@
xCBP#f2x4-8Ux0xl&|r@EP@p|P-zj+HmlWibrcbU_lw~`@bA*RcWby*V69BpsRd)aY
delta 57
zcmez2_QHkBCD<h-Ly3WbF>)i<L8i?i%o}CJMdIB9Tsh)BJY9GlodY}#3=GT|1RMo7
Nhb!_kPTr(+3;^ux5IO(=
diff --git a/tests/data/acpi/q35/DSDT.numamem b/tests/data/acpi/q35/DSDT.numamem
index 1978b55f1255402bf9bade0b91150b5cb49789a4..5ae686dd818d1427dfe4c3e73cc0342cc4e578a7 100644
GIT binary patch
delta 305
zcmZp%nPSJ~66_KpBG16UXuOf@AXB}72ZvsKuv2`1v!_9HLx6K|2qX6q9xjgPMgb7V
z87LmA03=)#q8ox;z2X_U&+u@uL^pDSIL=N6u3kV1CqLgHM(!&R2@uEG$uHDbA)3+2
z$Jv`fL^Z(K)r%=w8N~blzaRr7Sy0KC$>3zb>FO1&4iaCo`4m&UWRO2gynBEvN4$rp
z3$LSdfTw|hff<8{WxT6_Aw#rsj6O5Wtq`-21OlA>LZa1?1VAbTd_^}q$nZ0&M>lz~
x1b74lK}-(t3ovj58q5(N3bY64I|YyYl7gJl^vPPXvTR3qj_@#wO!k%C4gm3?QpNxP
delta 57
zcmbPY*J8uv66_MfBFDhM7`l<`Ak*d$<~T`lk$Cq2SB`iOPZwTC=KxOw0|PS#0Y|~j
NEVBHJljG&K0sxuq4d4I(
diff --git a/tests/data/acpi/q35/DSDT.tis b/tests/data/acpi/q35/DSDT.tis
index 638de3872673d17b1958497d0e62c83653de1602..dd712090ee9c09ed98f3b658951ac8a62b31a7d4 100644
GIT binary patch
delta 306
zcmccZ@Y;#XCD<h-Takf*v2Y{TL8f{E4-UQfV5j&1XHSFZh5+Z_5Jv7JJX{>njRGK!
zGf+HK0Z6zgL^lMxdc`wxpW)$RiEiWuah#nDT)lu2PJX^YjNDfs5+IJVlV7N>LNue3
zkFz&}h-!eds~1zWGKly8e?bOFvY?VNlflV=)72|l9VEVF^C_lC$sm80c=rHTj(87G
z7hXr_08ax012YB@%Xn7<LxyPO7=31#TOnp42?RL%g+!|(34l}xc#3Vdl96Uqk8bi}
y3GfIAf|wlO7hvEBG?*hk6lf37cM2Z)B?UR9>5~m)W!aAK9N}RU*&Hm(!3+S%CsU9B
delta 58
zcmaFublZW;CD<h-T7iLqv1KFIL8i?i%n_2}BJu74t{m|mo-VwO&H<hV1_ovf0=|Nq
O|H()*ZcdeBV+H{6kq@H)
--
2.25.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (6 preceding siblings ...)
2020-09-24 7:00 ` [RFC PATCH v3 7/7] bios-tables-test: Update golden binaries Julia Suvorova
@ 2020-09-24 8:57 ` no-reply
2020-09-24 9:03 ` no-reply
` (2 subsequent siblings)
10 siblings, 0 replies; 40+ messages in thread
From: no-reply @ 2020-09-24 8:57 UTC (permalink / raw)
To: jusual; +Cc: ani, imammedo, jusual, qemu-devel, mst
Patchew URL: https://patchew.org/QEMU/20200924070013.165026-1-jusual@redhat.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
Host machine cpu: x86_64
Target machine cpu family: x86
Target machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
The manual pages are in docs.
/usr/lib/gcc/x86_64-w64-mingw32/9.2.1/../../../../x86_64-w64-mingw32/bin/ld: libcommon.fa.p/hw_pci_pcie.c.obj: in function `acpi_pcihp_enabled':
/tmp/qemu-test/build/../src/hw/pci/pcie.c:522: undefined reference to `object_resolve_type_unambiguous'
collect2: error: ld returned 1 exit status
make: *** [Makefile.ninja:1878: qemu-system-aarch64w.exe] Error 1
make: *** Waiting for unfinished jobs....
/usr/lib/gcc/x86_64-w64-mingw32/9.2.1/../../../../x86_64-w64-mingw32/bin/ld: libcommon.fa.p/hw_pci_pcie.c.obj: in function `acpi_pcihp_enabled':
/tmp/qemu-test/build/../src/hw/pci/pcie.c:522: undefined reference to `object_resolve_type_unambiguous'
collect2: error: ld returned 1 exit status
make: *** [Makefile.ninja:1876: qemu-system-aarch64.exe] Error 1
Traceback (most recent call last):
File "./tests/docker/docker.py", line 709, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=342bc147a50847f886da9b01c0ca5ec9', '-u', '1003', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-vmsaoknh/src/docker-src.2020-09-24-04.53.10.30016:/var/tmp/qemu:z,ro', 'qemu/fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=342bc147a50847f886da9b01c0ca5ec9
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-vmsaoknh/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 4m42.837s
user 0m18.829s
The full log is available at
http://patchew.org/logs/20200924070013.165026-1-jusual@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (7 preceding siblings ...)
2020-09-24 8:57 ` [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 no-reply
@ 2020-09-24 9:03 ` no-reply
2020-09-24 9:20 ` Michael S. Tsirkin
2020-09-24 9:30 ` Igor Mammedov
10 siblings, 0 replies; 40+ messages in thread
From: no-reply @ 2020-09-24 9:03 UTC (permalink / raw)
To: jusual; +Cc: ani, imammedo, jusual, qemu-devel, mst
Patchew URL: https://patchew.org/QEMU/20200924070013.165026-1-jusual@redhat.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
C linker for the host machine: cc ld.bfd 2.27-43
Host machine cpu family: x86_64
Host machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
Linking target tests/test-replication
libcommon.fa.p/hw_pci_pcie.c.o: In function `acpi_pcihp_enabled':
/tmp/qemu-test/build/../src/hw/pci/pcie.c:522: undefined reference to `object_resolve_type_unambiguous'
collect2: error: ld returned 1 exit status
make: *** [qemu-system-aarch64] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 709, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=38644690a9934226b7e7108ea4390530', '-u', '1003', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-2jqqex0j/src/docker-src.2020-09-24-04.59.06.4662:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=38644690a9934226b7e7108ea4390530
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-2jqqex0j/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 4m43.237s
user 0m20.603s
The full log is available at
http://patchew.org/logs/20200924070013.165026-1-jusual@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (8 preceding siblings ...)
2020-09-24 9:03 ` no-reply
@ 2020-09-24 9:20 ` Michael S. Tsirkin
2020-10-01 8:55 ` Julia Suvorova
2020-09-24 9:30 ` Igor Mammedov
10 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-09-24 9:20 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, qemu-devel
On Thu, Sep 24, 2020 at 09:00:06AM +0200, Julia Suvorova wrote:
> The patch set consists of two parts:
> patches 1-4: introduce new feature
> 'acpi-pci-hotplug-with-bridge-support' on Q35
> patches 5-7: make the feature default along with changes in ACPI tables
>
> This way maintainers can decide which way to choose without breaking
> the patch set.
>
> With the feature disabled Q35 falls back to the native hot-plug.
>
> Pros
> * no racy behavior during boot (see 110c477c2ed)
> * eject is possible - according to PCIe spec, attention button
> press should lead to power off, and then the adapter should be
> removed manually. As there is no power down state exists in QEMU,
> we cannot distinguish between an eject and a power down
> request.
> * no delay during deleting - after the actual power off software
> must wait at least 1 second before indicating about it. This case
> is quite important for users, it even has its own bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594168
> * no timer-based behavior - in addition to the previous example,
> the attention button has a 5-second waiting period, during which
> the operation can be canceled with a second press. While this
> looks fine for manual button control, automation will result in
> the need to queue or drop events, and the software receiving
> events in all sort of unspecified combinations of attention/power
> indicator states, which is racy and uppredictable.
> * fixes:
> * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
> * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
>
> Cons:
> * lose per-port control over hot-plug (can be resolved)
> * no access to possible features presented in slot capabilities
> (this is only surprise removal AFAIK)
something I don't quite get is whether with this one can still add
new pcie bridges and then hotplug into these with native
hotplug.
the challenge to answering this with certainty is that right now qemu
does not allow hotplugging complex devices such as bridges at all,
we only have a hack for multifunction devices.
Maybe adding a bridge as function 1 on command line, then hotplugging another device as
function 0 will work to test this.
> v3:
> * drop change of _OSC to allow SHPC on hotplugged bridges
> * use 'acpi-root-pci-hotplug'
> * add migration states [Igor]
> * minor style changes
>
> v2:
> * new ioport range for acpiphp [Gerd]
> * drop find_pci_host() [Igor]
> * explain magic numbers in _OSC [Igor]
> * drop build_q35_pci_hotplug() wrapper [Igor]
>
> Julia Suvorova (7):
> hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
> hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
> hw/pci/pcie: Do not initialize slot capability if acpihp is used
> hw/acpi/ich9: Enable ACPI PCI hot-plug
> bios-tables-test: Allow changes in DSDT ACPI tables
> hw/acpi/ich9: Set ACPI PCI hot-plug as default
> bios-tables-test: Update golden binaries
>
> hw/i386/acpi-build.h | 7 ++++
> include/hw/acpi/ich9.h | 5 +++
> include/hw/acpi/pcihp.h | 3 +-
> hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++
> hw/acpi/pcihp.c | 16 ++++---
> hw/acpi/piix4.c | 4 +-
> hw/i386/acpi-build.c | 31 ++++++++------
> hw/i386/pc.c | 1 +
> hw/pci/pcie.c | 16 +++++++
> tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
> tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
> tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
> tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
> tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
> tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
> tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
> tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
> tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
> tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
> 19 files changed, 129 insertions(+), 21 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-09-24 9:20 ` Michael S. Tsirkin
@ 2020-10-01 8:55 ` Julia Suvorova
2020-10-01 11:40 ` Michael S. Tsirkin
0 siblings, 1 reply; 40+ messages in thread
From: Julia Suvorova @ 2020-10-01 8:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ani Sinha, Igor Mammedov, QEMU Developers
On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Sep 24, 2020 at 09:00:06AM +0200, Julia Suvorova wrote:
> > The patch set consists of two parts:
> > patches 1-4: introduce new feature
> > 'acpi-pci-hotplug-with-bridge-support' on Q35
> > patches 5-7: make the feature default along with changes in ACPI tables
> >
> > This way maintainers can decide which way to choose without breaking
> > the patch set.
> >
> > With the feature disabled Q35 falls back to the native hot-plug.
> >
> > Pros
> > * no racy behavior during boot (see 110c477c2ed)
> > * eject is possible - according to PCIe spec, attention button
> > press should lead to power off, and then the adapter should be
> > removed manually. As there is no power down state exists in QEMU,
> > we cannot distinguish between an eject and a power down
> > request.
> > * no delay during deleting - after the actual power off software
> > must wait at least 1 second before indicating about it. This case
> > is quite important for users, it even has its own bug:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1594168
> > * no timer-based behavior - in addition to the previous example,
> > the attention button has a 5-second waiting period, during which
> > the operation can be canceled with a second press. While this
> > looks fine for manual button control, automation will result in
> > the need to queue or drop events, and the software receiving
> > events in all sort of unspecified combinations of attention/power
> > indicator states, which is racy and uppredictable.
> > * fixes:
> > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
> > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
> >
> > Cons:
> > * lose per-port control over hot-plug (can be resolved)
> > * no access to possible features presented in slot capabilities
> > (this is only surprise removal AFAIK)
>
> something I don't quite get is whether with this one can still add
> new pcie bridges and then hotplug into these with native
> hotplug.
Right now I disable native if there is acpihp anywhere, but even if
you enable it for hotplugged devices, native hot-plug will not work.
> the challenge to answering this with certainty is that right now qemu
> does not allow hotplugging complex devices such as bridges at all,
> we only have a hack for multifunction devices.
> Maybe adding a bridge as function 1 on command line, then hotplugging another device as
> function 0 will work to test this.
>
>
>
> > v3:
> > * drop change of _OSC to allow SHPC on hotplugged bridges
> > * use 'acpi-root-pci-hotplug'
> > * add migration states [Igor]
> > * minor style changes
> >
> > v2:
> > * new ioport range for acpiphp [Gerd]
> > * drop find_pci_host() [Igor]
> > * explain magic numbers in _OSC [Igor]
> > * drop build_q35_pci_hotplug() wrapper [Igor]
> >
> > Julia Suvorova (7):
> > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
> > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
> > hw/pci/pcie: Do not initialize slot capability if acpihp is used
> > hw/acpi/ich9: Enable ACPI PCI hot-plug
> > bios-tables-test: Allow changes in DSDT ACPI tables
> > hw/acpi/ich9: Set ACPI PCI hot-plug as default
> > bios-tables-test: Update golden binaries
> >
> > hw/i386/acpi-build.h | 7 ++++
> > include/hw/acpi/ich9.h | 5 +++
> > include/hw/acpi/pcihp.h | 3 +-
> > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++
> > hw/acpi/pcihp.c | 16 ++++---
> > hw/acpi/piix4.c | 4 +-
> > hw/i386/acpi-build.c | 31 ++++++++------
> > hw/i386/pc.c | 1 +
> > hw/pci/pcie.c | 16 +++++++
> > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
> > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
> > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
> > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
> > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
> > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
> > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
> > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
> > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
> > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
> > 19 files changed, 129 insertions(+), 21 deletions(-)
> >
> > --
> > 2.25.4
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-10-01 8:55 ` Julia Suvorova
@ 2020-10-01 11:40 ` Michael S. Tsirkin
2020-10-01 13:01 ` Ani Sinha
2020-10-01 15:54 ` Julia Suvorova
0 siblings, 2 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-10-01 11:40 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, QEMU Developers
On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote:
> On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Sep 24, 2020 at 09:00:06AM +0200, Julia Suvorova wrote:
> > > The patch set consists of two parts:
> > > patches 1-4: introduce new feature
> > > 'acpi-pci-hotplug-with-bridge-support' on Q35
> > > patches 5-7: make the feature default along with changes in ACPI tables
> > >
> > > This way maintainers can decide which way to choose without breaking
> > > the patch set.
> > >
> > > With the feature disabled Q35 falls back to the native hot-plug.
> > >
> > > Pros
> > > * no racy behavior during boot (see 110c477c2ed)
> > > * eject is possible - according to PCIe spec, attention button
> > > press should lead to power off, and then the adapter should be
> > > removed manually. As there is no power down state exists in QEMU,
> > > we cannot distinguish between an eject and a power down
> > > request.
> > > * no delay during deleting - after the actual power off software
> > > must wait at least 1 second before indicating about it. This case
> > > is quite important for users, it even has its own bug:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1594168
> > > * no timer-based behavior - in addition to the previous example,
> > > the attention button has a 5-second waiting period, during which
> > > the operation can be canceled with a second press. While this
> > > looks fine for manual button control, automation will result in
> > > the need to queue or drop events, and the software receiving
> > > events in all sort of unspecified combinations of attention/power
> > > indicator states, which is racy and uppredictable.
> > > * fixes:
> > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
> > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
> > >
> > > Cons:
> > > * lose per-port control over hot-plug (can be resolved)
> > > * no access to possible features presented in slot capabilities
> > > (this is only surprise removal AFAIK)
> >
> > something I don't quite get is whether with this one can still add
> > new pcie bridges and then hotplug into these with native
> > hotplug.
>
> Right now I disable native if there is acpihp anywhere, but even if
> you enable it for hotplugged devices, native hot-plug will not work.
So that's a minor regression in functionality, right?
Why is that the case? Because you disable it in ACPI?
What if we don't?
> > the challenge to answering this with certainty is that right now qemu
> > does not allow hotplugging complex devices such as bridges at all,
> > we only have a hack for multifunction devices.
> > Maybe adding a bridge as function 1 on command line, then hotplugging another device as
> > function 0 will work to test this.
> >
> >
> >
> > > v3:
> > > * drop change of _OSC to allow SHPC on hotplugged bridges
> > > * use 'acpi-root-pci-hotplug'
> > > * add migration states [Igor]
> > > * minor style changes
> > >
> > > v2:
> > > * new ioport range for acpiphp [Gerd]
> > > * drop find_pci_host() [Igor]
> > > * explain magic numbers in _OSC [Igor]
> > > * drop build_q35_pci_hotplug() wrapper [Igor]
> > >
> > > Julia Suvorova (7):
> > > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
> > > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
> > > hw/pci/pcie: Do not initialize slot capability if acpihp is used
> > > hw/acpi/ich9: Enable ACPI PCI hot-plug
> > > bios-tables-test: Allow changes in DSDT ACPI tables
> > > hw/acpi/ich9: Set ACPI PCI hot-plug as default
> > > bios-tables-test: Update golden binaries
> > >
> > > hw/i386/acpi-build.h | 7 ++++
> > > include/hw/acpi/ich9.h | 5 +++
> > > include/hw/acpi/pcihp.h | 3 +-
> > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++
> > > hw/acpi/pcihp.c | 16 ++++---
> > > hw/acpi/piix4.c | 4 +-
> > > hw/i386/acpi-build.c | 31 ++++++++------
> > > hw/i386/pc.c | 1 +
> > > hw/pci/pcie.c | 16 +++++++
> > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
> > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
> > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
> > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
> > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
> > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
> > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
> > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
> > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
> > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
> > > 19 files changed, 129 insertions(+), 21 deletions(-)
> > >
> > > --
> > > 2.25.4
> >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-10-01 11:40 ` Michael S. Tsirkin
@ 2020-10-01 13:01 ` Ani Sinha
2020-10-01 15:25 ` Julia Suvorova
2020-10-01 15:54 ` Julia Suvorova
1 sibling, 1 reply; 40+ messages in thread
From: Ani Sinha @ 2020-10-01 13:01 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Igor Mammedov, Julia Suvorova, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 5522 bytes --]
On Thu, Oct 1, 2020 at 17:10 Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote:
> > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> > >
> > > On Thu, Sep 24, 2020 at 09:00:06AM +0200, Julia Suvorova wrote:
> > > > The patch set consists of two parts:
> > > > patches 1-4: introduce new feature
> > > > 'acpi-pci-hotplug-with-bridge-support' on Q35
> > > > patches 5-7: make the feature default along with changes in ACPI
> tables
> > > >
> > > > This way maintainers can decide which way to choose without breaking
> > > > the patch set.
> > > >
> > > > With the feature disabled Q35 falls back to the native hot-plug.
> > > >
> > > > Pros
> > > > * no racy behavior during boot (see 110c477c2ed)
> > > > * eject is possible - according to PCIe spec, attention button
> > > > press should lead to power off, and then the adapter should be
> > > > removed manually. As there is no power down state exists in
> QEMU,
> > > > we cannot distinguish between an eject and a power down
> > > > request.
> > > > * no delay during deleting - after the actual power off software
> > > > must wait at least 1 second before indicating about it. This
> case
> > > > is quite important for users, it even has its own bug:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1594168
> > > > * no timer-based behavior - in addition to the previous example,
> > > > the attention button has a 5-second waiting period, during
> which
> > > > the operation can be canceled with a second press. While this
> > > > looks fine for manual button control, automation will result in
> > > > the need to queue or drop events, and the software receiving
> > > > events in all sort of unspecified combinations of
> attention/power
> > > > indicator states, which is racy and uppredictable.
> > > > * fixes:
> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
> > > >
> > > > Cons:
> > > > * lose per-port control over hot-plug (can be resolved)
> > > > * no access to possible features presented in slot capabilities
> > > > (this is only surprise removal AFAIK)
> > >
> > > something I don't quite get is whether with this one can still add
> > > new pcie bridges and then hotplug into these with native
> > > hotplug.
> >
> > Right now I disable native if there is acpihp anywhere, but even if
> > you enable it for hotplugged devices, native hot-plug will not work.
>
> So that's a minor regression in functionality, right?
> Why is that the case? Because you disable it in ACPI?
> What if we don't?
Stupid question. If both native and acpi is enabled which one does OS
chose? Does this choice vary between OSes and different flavours of the
same OS like Windows?
>
> > > the challenge to answering this with certainty is that right now qemu
> > > does not allow hotplugging complex devices such as bridges at all,
> > > we only have a hack for multifunction devices.
> > > Maybe adding a bridge as function 1 on command line, then hotplugging
> another device as
> > > function 0 will work to test this.
> > >
> > >
> > >
> > > > v3:
> > > > * drop change of _OSC to allow SHPC on hotplugged bridges
> > > > * use 'acpi-root-pci-hotplug'
> > > > * add migration states [Igor]
> > > > * minor style changes
> > > >
> > > > v2:
> > > > * new ioport range for acpiphp [Gerd]
> > > > * drop find_pci_host() [Igor]
> > > > * explain magic numbers in _OSC [Igor]
> > > > * drop build_q35_pci_hotplug() wrapper [Igor]
> > > >
> > > > Julia Suvorova (7):
> > > > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
> > > > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
> > > > hw/pci/pcie: Do not initialize slot capability if acpihp is used
> > > > hw/acpi/ich9: Enable ACPI PCI hot-plug
> > > > bios-tables-test: Allow changes in DSDT ACPI tables
> > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default
> > > > bios-tables-test: Update golden binaries
> > > >
> > > > hw/i386/acpi-build.h | 7 ++++
> > > > include/hw/acpi/ich9.h | 5 +++
> > > > include/hw/acpi/pcihp.h | 3 +-
> > > > hw/acpi/ich9.c | 67
> ++++++++++++++++++++++++++++++
> > > > hw/acpi/pcihp.c | 16 ++++---
> > > > hw/acpi/piix4.c | 4 +-
> > > > hw/i386/acpi-build.c | 31 ++++++++------
> > > > hw/i386/pc.c | 1 +
> > > > hw/pci/pcie.c | 16 +++++++
> > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
> > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
> > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
> > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
> > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
> > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
> > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
> > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
> > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
> > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
> > > > 19 files changed, 129 insertions(+), 21 deletions(-)
> > > >
> > > > --
> > > > 2.25.4
> > >
>
>
[-- Attachment #2: Type: text/html, Size: 7907 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-10-01 13:01 ` Ani Sinha
@ 2020-10-01 15:25 ` Julia Suvorova
0 siblings, 0 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-10-01 15:25 UTC (permalink / raw)
To: Ani Sinha; +Cc: Igor Mammedov, QEMU Developers, Michael S. Tsirkin
On Thu, Oct 1, 2020 at 3:02 PM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Thu, Oct 1, 2020 at 17:10 Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote:
>> > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> > >
>> > > On Thu, Sep 24, 2020 at 09:00:06AM +0200, Julia Suvorova wrote:
>> > > > The patch set consists of two parts:
>> > > > patches 1-4: introduce new feature
>> > > > 'acpi-pci-hotplug-with-bridge-support' on Q35
>> > > > patches 5-7: make the feature default along with changes in ACPI tables
>> > > >
>> > > > This way maintainers can decide which way to choose without breaking
>> > > > the patch set.
>> > > >
>> > > > With the feature disabled Q35 falls back to the native hot-plug.
>> > > >
>> > > > Pros
>> > > > * no racy behavior during boot (see 110c477c2ed)
>> > > > * eject is possible - according to PCIe spec, attention button
>> > > > press should lead to power off, and then the adapter should be
>> > > > removed manually. As there is no power down state exists in QEMU,
>> > > > we cannot distinguish between an eject and a power down
>> > > > request.
>> > > > * no delay during deleting - after the actual power off software
>> > > > must wait at least 1 second before indicating about it. This case
>> > > > is quite important for users, it even has its own bug:
>> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1594168
>> > > > * no timer-based behavior - in addition to the previous example,
>> > > > the attention button has a 5-second waiting period, during which
>> > > > the operation can be canceled with a second press. While this
>> > > > looks fine for manual button control, automation will result in
>> > > > the need to queue or drop events, and the software receiving
>> > > > events in all sort of unspecified combinations of attention/power
>> > > > indicator states, which is racy and uppredictable.
>> > > > * fixes:
>> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
>> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
>> > > >
>> > > > Cons:
>> > > > * lose per-port control over hot-plug (can be resolved)
>> > > > * no access to possible features presented in slot capabilities
>> > > > (this is only surprise removal AFAIK)
>> > >
>> > > something I don't quite get is whether with this one can still add
>> > > new pcie bridges and then hotplug into these with native
>> > > hotplug.
>> >
>> > Right now I disable native if there is acpihp anywhere, but even if
>> > you enable it for hotplugged devices, native hot-plug will not work.
>>
>> So that's a minor regression in functionality, right?
>> Why is that the case? Because you disable it in ACPI?
>> What if we don't?
>
>
> Stupid question. If both native and acpi is enabled which one does OS chose? Does this choice vary between OSes and different flavours of the same OS like Windows?
It will always choose native.
>>
>>
>> > > the challenge to answering this with certainty is that right now qemu
>> > > does not allow hotplugging complex devices such as bridges at all,
>> > > we only have a hack for multifunction devices.
>> > > Maybe adding a bridge as function 1 on command line, then hotplugging another device as
>> > > function 0 will work to test this.
>> > >
>> > >
>> > >
>> > > > v3:
>> > > > * drop change of _OSC to allow SHPC on hotplugged bridges
>> > > > * use 'acpi-root-pci-hotplug'
>> > > > * add migration states [Igor]
>> > > > * minor style changes
>> > > >
>> > > > v2:
>> > > > * new ioport range for acpiphp [Gerd]
>> > > > * drop find_pci_host() [Igor]
>> > > > * explain magic numbers in _OSC [Igor]
>> > > > * drop build_q35_pci_hotplug() wrapper [Igor]
>> > > >
>> > > > Julia Suvorova (7):
>> > > > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
>> > > > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
>> > > > hw/pci/pcie: Do not initialize slot capability if acpihp is used
>> > > > hw/acpi/ich9: Enable ACPI PCI hot-plug
>> > > > bios-tables-test: Allow changes in DSDT ACPI tables
>> > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default
>> > > > bios-tables-test: Update golden binaries
>> > > >
>> > > > hw/i386/acpi-build.h | 7 ++++
>> > > > include/hw/acpi/ich9.h | 5 +++
>> > > > include/hw/acpi/pcihp.h | 3 +-
>> > > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++
>> > > > hw/acpi/pcihp.c | 16 ++++---
>> > > > hw/acpi/piix4.c | 4 +-
>> > > > hw/i386/acpi-build.c | 31 ++++++++------
>> > > > hw/i386/pc.c | 1 +
>> > > > hw/pci/pcie.c | 16 +++++++
>> > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
>> > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
>> > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
>> > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
>> > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
>> > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
>> > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
>> > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
>> > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
>> > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
>> > > > 19 files changed, 129 insertions(+), 21 deletions(-)
>> > > >
>> > > > --
>> > > > 2.25.4
>> > >
>>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-10-01 11:40 ` Michael S. Tsirkin
2020-10-01 13:01 ` Ani Sinha
@ 2020-10-01 15:54 ` Julia Suvorova
2020-10-01 16:11 ` Ani Sinha
2020-10-06 6:44 ` Michael S. Tsirkin
1 sibling, 2 replies; 40+ messages in thread
From: Julia Suvorova @ 2020-10-01 15:54 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ani Sinha, Igor Mammedov, QEMU Developers
On Thu, Oct 1, 2020 at 1:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote:
> > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Sep 24, 2020 at 09:00:06AM +0200, Julia Suvorova wrote:
> > > > The patch set consists of two parts:
> > > > patches 1-4: introduce new feature
> > > > 'acpi-pci-hotplug-with-bridge-support' on Q35
> > > > patches 5-7: make the feature default along with changes in ACPI tables
> > > >
> > > > This way maintainers can decide which way to choose without breaking
> > > > the patch set.
> > > >
> > > > With the feature disabled Q35 falls back to the native hot-plug.
> > > >
> > > > Pros
> > > > * no racy behavior during boot (see 110c477c2ed)
> > > > * eject is possible - according to PCIe spec, attention button
> > > > press should lead to power off, and then the adapter should be
> > > > removed manually. As there is no power down state exists in QEMU,
> > > > we cannot distinguish between an eject and a power down
> > > > request.
> > > > * no delay during deleting - after the actual power off software
> > > > must wait at least 1 second before indicating about it. This case
> > > > is quite important for users, it even has its own bug:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1594168
> > > > * no timer-based behavior - in addition to the previous example,
> > > > the attention button has a 5-second waiting period, during which
> > > > the operation can be canceled with a second press. While this
> > > > looks fine for manual button control, automation will result in
> > > > the need to queue or drop events, and the software receiving
> > > > events in all sort of unspecified combinations of attention/power
> > > > indicator states, which is racy and uppredictable.
> > > > * fixes:
> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
> > > >
> > > > Cons:
> > > > * lose per-port control over hot-plug (can be resolved)
> > > > * no access to possible features presented in slot capabilities
> > > > (this is only surprise removal AFAIK)
> > >
> > > something I don't quite get is whether with this one can still add
> > > new pcie bridges and then hotplug into these with native
> > > hotplug.
> >
> > Right now I disable native if there is acpihp anywhere, but even if
> > you enable it for hotplugged devices, native hot-plug will not work.
>
> So that's a minor regression in functionality, right?
> Why is that the case? Because you disable it in ACPI?
> What if we don't?
I meant that I disable slot hotplug capabilities, nothing in ACPI
prevents native from working. Actually, I don't see if there's any
regression at all. Configurations like hot-plugging downstream port or
switch to another downstream port haven't worked before, and they
don't work now. I can enable native for hotplugged bridges, but that
doesn't make sense, because you won't be able to hot-plug anything to
it. It's not an issue of ACPI, it's PCIe behaviour. Also, native-acpi
combination may seem bizarre to os (slot enumeration is independent,
that's why I suggested disabling pcie slot flags).
> > > the challenge to answering this with certainty is that right now qemu
> > > does not allow hotplugging complex devices such as bridges at all,
> > > we only have a hack for multifunction devices.
> > > Maybe adding a bridge as function 1 on command line, then hotplugging another device as
> > > function 0 will work to test this.
> > >
> > >
> > >
> > > > v3:
> > > > * drop change of _OSC to allow SHPC on hotplugged bridges
> > > > * use 'acpi-root-pci-hotplug'
> > > > * add migration states [Igor]
> > > > * minor style changes
> > > >
> > > > v2:
> > > > * new ioport range for acpiphp [Gerd]
> > > > * drop find_pci_host() [Igor]
> > > > * explain magic numbers in _OSC [Igor]
> > > > * drop build_q35_pci_hotplug() wrapper [Igor]
> > > >
> > > > Julia Suvorova (7):
> > > > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
> > > > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
> > > > hw/pci/pcie: Do not initialize slot capability if acpihp is used
> > > > hw/acpi/ich9: Enable ACPI PCI hot-plug
> > > > bios-tables-test: Allow changes in DSDT ACPI tables
> > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default
> > > > bios-tables-test: Update golden binaries
> > > >
> > > > hw/i386/acpi-build.h | 7 ++++
> > > > include/hw/acpi/ich9.h | 5 +++
> > > > include/hw/acpi/pcihp.h | 3 +-
> > > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++
> > > > hw/acpi/pcihp.c | 16 ++++---
> > > > hw/acpi/piix4.c | 4 +-
> > > > hw/i386/acpi-build.c | 31 ++++++++------
> > > > hw/i386/pc.c | 1 +
> > > > hw/pci/pcie.c | 16 +++++++
> > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
> > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
> > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
> > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
> > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
> > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
> > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
> > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
> > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
> > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
> > > > 19 files changed, 129 insertions(+), 21 deletions(-)
> > > >
> > > > --
> > > > 2.25.4
> > >
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-10-01 15:54 ` Julia Suvorova
@ 2020-10-01 16:11 ` Ani Sinha
2020-10-06 6:44 ` Michael S. Tsirkin
1 sibling, 0 replies; 40+ messages in thread
From: Ani Sinha @ 2020-10-01 16:11 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Igor Mammedov, QEMU Developers, Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 6532 bytes --]
On Thu, Oct 1, 2020 at 9:24 PM Julia Suvorova <jusual@redhat.com> wrote:
> On Thu, Oct 1, 2020 at 1:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> >
>
> > On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote:
>
> > > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com>
> wrote:
>
> > > >
>
> > > > On Thu, Sep 24, 2020 at 09:00:06AM +0200, Julia Suvorova wrote:
>
> > > > > The patch set consists of two parts:
>
> > > > > patches 1-4: introduce new feature
>
> > > > > 'acpi-pci-hotplug-with-bridge-support' on Q35
>
> > > > > patches 5-7: make the feature default along with changes in ACPI
> tables
>
> > > > >
>
> > > > > This way maintainers can decide which way to choose without
> breaking
>
> > > > > the patch set.
>
> > > > >
>
> > > > > With the feature disabled Q35 falls back to the native hot-plug.
>
> > > > >
>
> > > > > Pros
>
> > > > > * no racy behavior during boot (see 110c477c2ed)
>
> > > > > * eject is possible - according to PCIe spec, attention button
>
> > > > > press should lead to power off, and then the adapter should
> be
>
> > > > > removed manually. As there is no power down state exists in
> QEMU,
>
> > > > > we cannot distinguish between an eject and a power down
>
> > > > > request.
>
> > > > > * no delay during deleting - after the actual power off
> software
>
> > > > > must wait at least 1 second before indicating about it. This
> case
>
> > > > > is quite important for users, it even has its own bug:
>
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1594168
>
> > > > > * no timer-based behavior - in addition to the previous
> example,
>
> > > > > the attention button has a 5-second waiting period, during
> which
>
> > > > > the operation can be canceled with a second press. While this
>
> > > > > looks fine for manual button control, automation will result
> in
>
> > > > > the need to queue or drop events, and the software receiving
>
> > > > > events in all sort of unspecified combinations of
> attention/power
>
> > > > > indicator states, which is racy and uppredictable.
>
> > > > > * fixes:
>
> > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
>
> > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
>
> > > > >
>
> > > > > Cons:
>
> > > > > * lose per-port control over hot-plug (can be resolved)
>
> > > > > * no access to possible features presented in slot capabilities
>
> > > > > (this is only surprise removal AFAIK)
>
> > > >
>
> > > > something I don't quite get is whether with this one can still add
>
> > > > new pcie bridges and then hotplug into these with native
>
> > > > hotplug.
>
> > >
>
> > > Right now I disable native if there is acpihp anywhere, but even if
>
> > > you enable it for hotplugged devices, native hot-plug will not work.
>
> >
>
> > So that's a minor regression in functionality, right?
>
> > Why is that the case? Because you disable it in ACPI?
>
> > What if we don't?
>
>
>
> I meant that I disable slot hotplug capabilities, nothing in ACPI
>
> prevents native from working. Actually, I don't see if there's any
>
> regression at all. Configurations like hot-plugging downstream port or
>
> switch to another downstream port haven't worked before, and they
>
> don't work now. I can enable native for hotplugged bridges, but that
>
> doesn't make sense, because you won't be able to hot-plug anything to
>
> it.
Why is that?
It's not an issue of ACPI, it's PCIe behaviour. Also, native-acpi
>
> combination may seem bizarre to os (slot enumeration is independent,
>
> that's why I suggested disabling pcie slot flags).
>
>
>
> > > > the challenge to answering this with certainty is that right now qemu
>
> > > > does not allow hotplugging complex devices such as bridges at all,
>
> > > > we only have a hack for multifunction devices.
>
> > > > Maybe adding a bridge as function 1 on command line, then
> hotplugging another device as
>
> > > > function 0 will work to test this.
>
> > > >
>
> > > >
>
> > > >
>
> > > > > v3:
>
> > > > > * drop change of _OSC to allow SHPC on hotplugged bridges
>
> > > > > * use 'acpi-root-pci-hotplug'
>
> > > > > * add migration states [Igor]
>
> > > > > * minor style changes
>
> > > > >
>
> > > > > v2:
>
> > > > > * new ioport range for acpiphp [Gerd]
>
> > > > > * drop find_pci_host() [Igor]
>
> > > > > * explain magic numbers in _OSC [Igor]
>
> > > > > * drop build_q35_pci_hotplug() wrapper [Igor]
>
> > > > >
>
> > > > > Julia Suvorova (7):
>
> > > > > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support
> Q35
>
> > > > > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
>
> > > > > hw/pci/pcie: Do not initialize slot capability if acpihp is used
>
> > > > > hw/acpi/ich9: Enable ACPI PCI hot-plug
>
> > > > > bios-tables-test: Allow changes in DSDT ACPI tables
>
> > > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default
>
> > > > > bios-tables-test: Update golden binaries
>
> > > > >
>
> > > > > hw/i386/acpi-build.h | 7 ++++
>
> > > > > include/hw/acpi/ich9.h | 5 +++
>
> > > > > include/hw/acpi/pcihp.h | 3 +-
>
> > > > > hw/acpi/ich9.c | 67
> ++++++++++++++++++++++++++++++
>
> > > > > hw/acpi/pcihp.c | 16 ++++---
>
> > > > > hw/acpi/piix4.c | 4 +-
>
> > > > > hw/i386/acpi-build.c | 31 ++++++++------
>
> > > > > hw/i386/pc.c | 1 +
>
> > > > > hw/pci/pcie.c | 16 +++++++
>
> > > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
>
> > > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
>
> > > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
>
> > > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
>
> > > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
>
> > > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
>
> > > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
>
> > > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
>
> > > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
>
> > > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
>
> > > > > 19 files changed, 129 insertions(+), 21 deletions(-)
>
> > > > >
>
> > > > > --
>
> > > > > 2.25.4
>
> > > >
>
> >
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 9359 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-10-01 15:54 ` Julia Suvorova
2020-10-01 16:11 ` Ani Sinha
@ 2020-10-06 6:44 ` Michael S. Tsirkin
1 sibling, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-10-06 6:44 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, Igor Mammedov, QEMU Developers
On Thu, Oct 01, 2020 at 05:54:39PM +0200, Julia Suvorova wrote:
> > > Right now I disable native if there is acpihp anywhere, but even if
> > > you enable it for hotplugged devices, native hot-plug will not work.
> >
> > So that's a minor regression in functionality, right?
> > Why is that the case? Because you disable it in ACPI?
> > What if we don't?
>
> I meant that I disable slot hotplug capabilities, nothing in ACPI
> prevents native from working. Actually, I don't see if there's any
> regression at all. Configurations like hot-plugging downstream port or
> switch to another downstream port haven't worked before, and they
> don't work now. I can enable native for hotplugged bridges, but that
> doesn't make sense, because you won't be able to hot-plug anything to
> it.
You can do the following hack right now:
1- add an upstream port as function 1
2- add a downstream port behind it
3- add some other device (e.g. another upstream port?) as function 0
As this point both ports should be detected.
Going forward we can consider support for adding ports in a hidden state
(not visible to guest) so one won't need an extra function.
> It's not an issue of ACPI, it's PCIe behaviour. Also, native-acpi
> combination may seem bizarre to os
Maybe, maybe not ...
Worth testing whether this works with existing guests.
> (slot enumeration is independent,
> that's why I suggested disabling pcie slot flags).
Yes that part makes sense imho.
--
MST
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-09-24 7:00 [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
` (9 preceding siblings ...)
2020-09-24 9:20 ` Michael S. Tsirkin
@ 2020-09-24 9:30 ` Igor Mammedov
2020-09-24 9:39 ` Michael S. Tsirkin
10 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2020-09-24 9:30 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Ani Sinha, qemu-devel, Michael S. Tsirkin
On Thu, 24 Sep 2020 09:00:06 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> The patch set consists of two parts:
> patches 1-4: introduce new feature
> 'acpi-pci-hotplug-with-bridge-support' on Q35
> patches 5-7: make the feature default along with changes in ACPI tables
>
> This way maintainers can decide which way to choose without breaking
> the patch set.
>
> With the feature disabled Q35 falls back to the native hot-plug.
>
> Pros
> * no racy behavior during boot (see 110c477c2ed)
> * eject is possible - according to PCIe spec, attention button
> press should lead to power off, and then the adapter should be
> removed manually. As there is no power down state exists in QEMU,
> we cannot distinguish between an eject and a power down
> request.
if I recall right, you had a idea about
keeping pending removal request to distinguish between eject and poweroff
(i.e. eject if mgmt asked for removal and otherwise it could be poweroff|nop)
Why it didn't work out in the end?
> * no delay during deleting - after the actual power off software
> must wait at least 1 second before indicating about it. This case
> is quite important for users, it even has its own bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594168
> * no timer-based behavior - in addition to the previous example,
> the attention button has a 5-second waiting period, during which
> the operation can be canceled with a second press. While this
> looks fine for manual button control, automation will result in
> the need to queue or drop events, and the software receiving
> events in all sort of unspecified combinations of attention/power
> indicator states, which is racy and uppredictable.
> * fixes:
> * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
> * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
>
> Cons:
> * lose per-port control over hot-plug (can be resolved)
> * no access to possible features presented in slot capabilities
> (this is only surprise removal AFAIK)
>
> v3:
> * drop change of _OSC to allow SHPC on hotplugged bridges
> * use 'acpi-root-pci-hotplug'
> * add migration states [Igor]
> * minor style changes
>
> v2:
> * new ioport range for acpiphp [Gerd]
> * drop find_pci_host() [Igor]
> * explain magic numbers in _OSC [Igor]
> * drop build_q35_pci_hotplug() wrapper [Igor]
>
> Julia Suvorova (7):
> hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
> hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
> hw/pci/pcie: Do not initialize slot capability if acpihp is used
> hw/acpi/ich9: Enable ACPI PCI hot-plug
> bios-tables-test: Allow changes in DSDT ACPI tables
> hw/acpi/ich9: Set ACPI PCI hot-plug as default
> bios-tables-test: Update golden binaries
>
> hw/i386/acpi-build.h | 7 ++++
> include/hw/acpi/ich9.h | 5 +++
> include/hw/acpi/pcihp.h | 3 +-
> hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++
> hw/acpi/pcihp.c | 16 ++++---
> hw/acpi/piix4.c | 4 +-
> hw/i386/acpi-build.c | 31 ++++++++------
> hw/i386/pc.c | 1 +
> hw/pci/pcie.c | 16 +++++++
> tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
> tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
> tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
> tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
> tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
> tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
> tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
> tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
> tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
> tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
> 19 files changed, 129 insertions(+), 21 deletions(-)
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [RFC PATCH v3 0/7] Use ACPI PCI hot-plug for Q35
2020-09-24 9:30 ` Igor Mammedov
@ 2020-09-24 9:39 ` Michael S. Tsirkin
0 siblings, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2020-09-24 9:39 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Ani Sinha, Julia Suvorova, qemu-devel
On Thu, Sep 24, 2020 at 11:30:00AM +0200, Igor Mammedov wrote:
> On Thu, 24 Sep 2020 09:00:06 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > The patch set consists of two parts:
> > patches 1-4: introduce new feature
> > 'acpi-pci-hotplug-with-bridge-support' on Q35
> > patches 5-7: make the feature default along with changes in ACPI tables
> >
> > This way maintainers can decide which way to choose without breaking
> > the patch set.
> >
> > With the feature disabled Q35 falls back to the native hot-plug.
> >
> > Pros
> > * no racy behavior during boot (see 110c477c2ed)
> > * eject is possible - according to PCIe spec, attention button
> > press should lead to power off, and then the adapter should be
> > removed manually. As there is no power down state exists in QEMU,
> > we cannot distinguish between an eject and a power down
> > request.
>
> if I recall right, you had a idea about
> keeping pending removal request to distinguish between eject and poweroff
> (i.e. eject if mgmt asked for removal and otherwise it could be poweroff|nop)
> Why it didn't work out in the end?
For better or worse guests expect to eject and have it happen,
without also asking management to do it ...
> > * no delay during deleting - after the actual power off software
> > must wait at least 1 second before indicating about it. This case
> > is quite important for users, it even has its own bug:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1594168
> > * no timer-based behavior - in addition to the previous example,
> > the attention button has a 5-second waiting period, during which
> > the operation can be canceled with a second press. While this
> > looks fine for manual button control, automation will result in
> > the need to queue or drop events, and the software receiving
> > events in all sort of unspecified combinations of attention/power
> > indicator states, which is racy and uppredictable.
> > * fixes:
> > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465
> > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256
> >
> > Cons:
> > * lose per-port control over hot-plug (can be resolved)
> > * no access to possible features presented in slot capabilities
> > (this is only surprise removal AFAIK)
> >
> > v3:
> > * drop change of _OSC to allow SHPC on hotplugged bridges
> > * use 'acpi-root-pci-hotplug'
> > * add migration states [Igor]
> > * minor style changes
> >
> > v2:
> > * new ioport range for acpiphp [Gerd]
> > * drop find_pci_host() [Igor]
> > * explain magic numbers in _OSC [Igor]
> > * drop build_q35_pci_hotplug() wrapper [Igor]
> >
> > Julia Suvorova (7):
> > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35
> > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35
> > hw/pci/pcie: Do not initialize slot capability if acpihp is used
> > hw/acpi/ich9: Enable ACPI PCI hot-plug
> > bios-tables-test: Allow changes in DSDT ACPI tables
> > hw/acpi/ich9: Set ACPI PCI hot-plug as default
> > bios-tables-test: Update golden binaries
> >
> > hw/i386/acpi-build.h | 7 ++++
> > include/hw/acpi/ich9.h | 5 +++
> > include/hw/acpi/pcihp.h | 3 +-
> > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++
> > hw/acpi/pcihp.c | 16 ++++---
> > hw/acpi/piix4.c | 4 +-
> > hw/i386/acpi-build.c | 31 ++++++++------
> > hw/i386/pc.c | 1 +
> > hw/pci/pcie.c | 16 +++++++
> > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes
> > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes
> > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes
> > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes
> > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes
> > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes
> > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes
> > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes
> > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes
> > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes
> > 19 files changed, 129 insertions(+), 21 deletions(-)
> >
^ permalink raw reply [flat|nested] 40+ messages in thread