* [RFC PATCH v2 1/4] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
2020-08-18 21:52 [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 Julia Suvorova
@ 2020-08-18 21:52 ` Julia Suvorova
2020-08-19 3:14 ` Philippe Mathieu-Daudé
2020-08-18 21:52 ` [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35 Julia Suvorova
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Julia Suvorova @ 2020-08-18 21:52 UTC (permalink / raw)
To: qemu-devel
Cc: Igor Mammedov, Julia Suvorova, Gerd Hoffmann, Michael S. Tsirkin
Add trace events similar to piix4_gpe_readb() to check gpe status.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/ich9.c | 7 ++++++-
hw/acpi/trace-events | 4 ++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 6a19070cec..a2a1742aa6 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -36,6 +36,7 @@
#include "hw/acpi/acpi.h"
#include "hw/acpi/tco.h"
#include "exec/address-spaces.h"
+#include "trace.h"
#include "hw/i386/ich9.h"
#include "hw/mem/pc-dimm.h"
@@ -59,13 +60,17 @@ static void ich9_pm_update_sci_fn(ACPIREGS *regs)
static uint64_t ich9_gpe_readb(void *opaque, hwaddr addr, unsigned width)
{
ICH9LPCPMRegs *pm = opaque;
- return acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
+ uint64_t val = acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
+
+ trace_ich9_gpe_readb(addr, width, val);
+ return val;
}
static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
unsigned width)
{
ICH9LPCPMRegs *pm = opaque;
+ trace_ich9_gpe_writeb(addr, width, val);
acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
acpi_update_sci(&pm->acpi_regs, pm->irq);
}
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index afbc77de1c..b9f4827afc 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -32,6 +32,10 @@ cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32
cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
+# ich9.c
+ich9_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d ==> 0x%" PRIx64
+ich9_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d <== 0x%" PRIx64
+
# pcihp.c
acpi_pci_eject_slot(unsigned bsel, unsigned slot) "bsel: %u slot: %u"
acpi_pci_unplug(int bsel, int slot) "bsel: %d slot: %d"
--
2.25.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 1/4] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
2020-08-18 21:52 ` [RFC PATCH v2 1/4] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb() Julia Suvorova
@ 2020-08-19 3:14 ` Philippe Mathieu-Daudé
2020-08-25 13:14 ` Julia Suvorova
0 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-19 3:14 UTC (permalink / raw)
To: Julia Suvorova, qemu-devel
Cc: Igor Mammedov, Gerd Hoffmann, Michael S. Tsirkin
On 8/18/20 11:52 PM, Julia Suvorova wrote:
> Add trace events similar to piix4_gpe_readb() to check gpe status.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/acpi/ich9.c | 7 ++++++-
> hw/acpi/trace-events | 4 ++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 6a19070cec..a2a1742aa6 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -36,6 +36,7 @@
> #include "hw/acpi/acpi.h"
> #include "hw/acpi/tco.h"
> #include "exec/address-spaces.h"
> +#include "trace.h"
>
> #include "hw/i386/ich9.h"
> #include "hw/mem/pc-dimm.h"
> @@ -59,13 +60,17 @@ static void ich9_pm_update_sci_fn(ACPIREGS *regs)
> static uint64_t ich9_gpe_readb(void *opaque, hwaddr addr, unsigned width)
> {
> ICH9LPCPMRegs *pm = opaque;
> - return acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
> + uint64_t val = acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
> +
> + trace_ich9_gpe_readb(addr, width, val);
> + return val;
> }
>
> static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
> unsigned width)
> {
> ICH9LPCPMRegs *pm = opaque;
> + trace_ich9_gpe_writeb(addr, width, val);
> acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> index afbc77de1c..b9f4827afc 100644
> --- a/hw/acpi/trace-events
> +++ b/hw/acpi/trace-events
> @@ -32,6 +32,10 @@ cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32
> cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
> cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
>
> +# ich9.c
> +ich9_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d ==> 0x%" PRIx64
> +ich9_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d <== 0x%" PRIx64
Nitpick, val could be uint8_t.
Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> +
> # pcihp.c
> acpi_pci_eject_slot(unsigned bsel, unsigned slot) "bsel: %u slot: %u"
> acpi_pci_unplug(int bsel, int slot) "bsel: %d slot: %d"
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 1/4] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
2020-08-19 3:14 ` Philippe Mathieu-Daudé
@ 2020-08-25 13:14 ` Julia Suvorova
0 siblings, 0 replies; 27+ messages in thread
From: Julia Suvorova @ 2020-08-25 13:14 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Igor Mammedov, Michael S. Tsirkin, QEMU Developers, Gerd Hoffmann
On Wed, Aug 19, 2020 at 5:14 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 8/18/20 11:52 PM, Julia Suvorova wrote:
> > Add trace events similar to piix4_gpe_readb() to check gpe status.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/acpi/ich9.c | 7 ++++++-
> > hw/acpi/trace-events | 4 ++++
> > 2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 6a19070cec..a2a1742aa6 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -36,6 +36,7 @@
> > #include "hw/acpi/acpi.h"
> > #include "hw/acpi/tco.h"
> > #include "exec/address-spaces.h"
> > +#include "trace.h"
> >
> > #include "hw/i386/ich9.h"
> > #include "hw/mem/pc-dimm.h"
> > @@ -59,13 +60,17 @@ static void ich9_pm_update_sci_fn(ACPIREGS *regs)
> > static uint64_t ich9_gpe_readb(void *opaque, hwaddr addr, unsigned width)
> > {
> > ICH9LPCPMRegs *pm = opaque;
> > - return acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
> > + uint64_t val = acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
> > +
> > + trace_ich9_gpe_readb(addr, width, val);
> > + return val;
> > }
> >
> > static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
> > unsigned width)
> > {
> > ICH9LPCPMRegs *pm = opaque;
> > + trace_ich9_gpe_writeb(addr, width, val);
> > acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> > acpi_update_sci(&pm->acpi_regs, pm->irq);
> > }
> > diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> > index afbc77de1c..b9f4827afc 100644
> > --- a/hw/acpi/trace-events
> > +++ b/hw/acpi/trace-events
> > @@ -32,6 +32,10 @@ cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32
> > cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
> > cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
> >
> > +# ich9.c
> > +ich9_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d ==> 0x%" PRIx64
> > +ich9_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d <== 0x%" PRIx64
>
> Nitpick, val could be uint8_t.
Since 'val' is an argument and its type is uint64_t it's better to
avoid implicit cast to a smaller type. Also, acpi_gpe_ioport_readb()
does not return uint8_t.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
2020-08-18 21:52 [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 Julia Suvorova
2020-08-18 21:52 ` [RFC PATCH v2 1/4] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb() Julia Suvorova
@ 2020-08-18 21:52 ` Julia Suvorova
2020-08-19 3:21 ` Philippe Mathieu-Daudé
2020-08-21 12:08 ` Igor Mammedov
2020-08-18 21:52 ` [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC Julia Suvorova
` (4 subsequent siblings)
6 siblings, 2 replies; 27+ messages in thread
From: Julia Suvorova @ 2020-08-18 21:52 UTC (permalink / raw)
To: qemu-devel
Cc: Igor Mammedov, Julia Suvorova, Gerd Hoffmann, 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>
---
include/hw/acpi/pcihp.h | 3 ++-
hw/acpi/pcihp.c | 10 ++++++----
hw/acpi/piix4.c | 2 +-
hw/i386/acpi-build.c | 25 ++++++++++++++-----------
4 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 8bc4a4c01d..1e9d246f57 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,
+ bool is_piix4);
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 9e31ab2da4..9a35ed6c83 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -38,7 +38,8 @@
#include "qom/qom-qobject.h"
#include "trace.h"
-#define ACPI_PCIHP_ADDR 0xae00
+#define ACPI_PCIHP_ADDR_PIIX4 0xae00
+#define ACPI_PCIHP_ADDR_Q35 0x0cc4
#define ACPI_PCIHP_SIZE 0x0014
#define PCI_UP_BASE 0x0000
#define PCI_DOWN_BASE 0x0004
@@ -359,12 +360,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,
+ bool is_piix4)
{
s->io_len = ACPI_PCIHP_SIZE;
- s->io_base = ACPI_PCIHP_ADDR;
+ s->io_base = is_piix4 ? ACPI_PCIHP_ADDR_PIIX4 : ACPI_PCIHP_ADDR_Q35;
- 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 cdfa0e2998..1f27bfbd06 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -596,7 +596,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, true);
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 b7bcbbbb2a..f3cd52bd06 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_i386_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 + 0x08), 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 + 0x10), 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,6 @@ 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_piix4_pci0_int(dsdt);
} else {
sb_scope = aml_scope("_SB");
@@ -1526,6 +1527,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
}
}
+ build_i386_pci_hotplug(dsdt, pm->pcihp_io_base);
+
if (pcmc->legacy_cpu_hotplug) {
build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
} else {
@@ -1546,7 +1549,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] 27+ messages in thread* Re: [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
2020-08-18 21:52 ` [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35 Julia Suvorova
@ 2020-08-19 3:21 ` Philippe Mathieu-Daudé
2020-09-16 19:24 ` Julia Suvorova
2020-08-21 12:08 ` Igor Mammedov
1 sibling, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-19 3:21 UTC (permalink / raw)
To: Julia Suvorova, qemu-devel
Cc: Igor Mammedov, Gerd Hoffmann, Michael S. Tsirkin
Hi Julia,
On 8/18/20 11:52 PM, Julia Suvorova wrote:
> 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>
> ---
> include/hw/acpi/pcihp.h | 3 ++-
> hw/acpi/pcihp.c | 10 ++++++----
> hw/acpi/piix4.c | 2 +-
> hw/i386/acpi-build.c | 25 ++++++++++++++-----------
> 4 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 8bc4a4c01d..1e9d246f57 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,
> + bool is_piix4);
>
> 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 9e31ab2da4..9a35ed6c83 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -38,7 +38,8 @@
> #include "qom/qom-qobject.h"
> #include "trace.h"
>
> -#define ACPI_PCIHP_ADDR 0xae00
> +#define ACPI_PCIHP_ADDR_PIIX4 0xae00
> +#define ACPI_PCIHP_ADDR_Q35 0x0cc4
> #define ACPI_PCIHP_SIZE 0x0014
> #define PCI_UP_BASE 0x0000
> #define PCI_DOWN_BASE 0x0004
> @@ -359,12 +360,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,
> + bool is_piix4)
Instead of adding implementation knowledge to this generic function, can
you instead pass it a 'io_base' argument (or 'pcihp_addr')?
> {
> s->io_len = ACPI_PCIHP_SIZE;
> - s->io_base = ACPI_PCIHP_ADDR;
> + s->io_base = is_piix4 ? ACPI_PCIHP_ADDR_PIIX4 : ACPI_PCIHP_ADDR_Q35;
>
> - 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 cdfa0e2998..1f27bfbd06 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -596,7 +596,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, true);
>
> 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 b7bcbbbb2a..f3cd52bd06 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)) {
Different patch?
> 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_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
Being common to 32/64-bit, I'd name that build_x86_pci_hotplug().
> {
> 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 + 0x08), 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 + 0x10), 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,6 @@ 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_piix4_pci0_int(dsdt);
> } else {
> sb_scope = aml_scope("_SB");
> @@ -1526,6 +1527,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> }
> }
>
> + build_i386_pci_hotplug(dsdt, pm->pcihp_io_base);
> +
> if (pcmc->legacy_cpu_hotplug) {
> build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> } else {
> @@ -1546,7 +1549,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) {
Why not directly check 'if (pm->pcihp_bridge_en) {'?
> method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> aml_append(method,
> aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
>
Regards,
Phil.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
2020-08-19 3:21 ` Philippe Mathieu-Daudé
@ 2020-09-16 19:24 ` Julia Suvorova
0 siblings, 0 replies; 27+ messages in thread
From: Julia Suvorova @ 2020-09-16 19:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Igor Mammedov, Michael S. Tsirkin, QEMU Developers, Gerd Hoffmann
On Wed, Aug 19, 2020 at 5:21 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Julia,
>
> On 8/18/20 11:52 PM, Julia Suvorova wrote:
> > 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>
> > ---
> > include/hw/acpi/pcihp.h | 3 ++-
> > hw/acpi/pcihp.c | 10 ++++++----
> > hw/acpi/piix4.c | 2 +-
> > hw/i386/acpi-build.c | 25 ++++++++++++++-----------
> > 4 files changed, 23 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > index 8bc4a4c01d..1e9d246f57 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,
> > + bool is_piix4);
> >
> > 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 9e31ab2da4..9a35ed6c83 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -38,7 +38,8 @@
> > #include "qom/qom-qobject.h"
> > #include "trace.h"
> >
> > -#define ACPI_PCIHP_ADDR 0xae00
> > +#define ACPI_PCIHP_ADDR_PIIX4 0xae00
> > +#define ACPI_PCIHP_ADDR_Q35 0x0cc4
> > #define ACPI_PCIHP_SIZE 0x0014
> > #define PCI_UP_BASE 0x0000
> > #define PCI_DOWN_BASE 0x0004
> > @@ -359,12 +360,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,
> > + bool is_piix4)
>
> Instead of adding implementation knowledge to this generic function, can
> you instead pass it a 'io_base' argument (or 'pcihp_addr')?
Ok.
> > {
> > s->io_len = ACPI_PCIHP_SIZE;
> > - s->io_base = ACPI_PCIHP_ADDR;
> > + s->io_base = is_piix4 ? ACPI_PCIHP_ADDR_PIIX4 : ACPI_PCIHP_ADDR_Q35;
> >
> > - 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 cdfa0e2998..1f27bfbd06 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -596,7 +596,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, true);
> >
> > 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 b7bcbbbb2a..f3cd52bd06 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)) {
>
> Different patch?
PCNT method is part of ACPI hot-plug, and we add it to q35 (pcie).
> > 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_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
>
> Being common to 32/64-bit, I'd name that build_x86_pci_hotplug().
>
> > {
> > 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 + 0x08), 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 + 0x10), 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,6 @@ 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_piix4_pci0_int(dsdt);
> > } else {
> > sb_scope = aml_scope("_SB");
> > @@ -1526,6 +1527,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > }
> > }
> >
> > + build_i386_pci_hotplug(dsdt, pm->pcihp_io_base);
> > +
> > if (pcmc->legacy_cpu_hotplug) {
> > build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> > } else {
> > @@ -1546,7 +1549,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) {
>
> Why not directly check 'if (pm->pcihp_bridge_en) {'?
GPE.1 should be implemented for pci root hotplug on piix4 even if
pcihp_bridge_en is disabled. There will be a bit different logic with
"[PATCH v5 09/11] piix4: don't reserve hw resources when hotplug is
off globally", but it's not merged yet.
> > method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> > aml_append(method,
> > aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
> >
>
> Regards,
>
> Phil.
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
2020-08-18 21:52 ` [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35 Julia Suvorova
2020-08-19 3:21 ` Philippe Mathieu-Daudé
@ 2020-08-21 12:08 ` Igor Mammedov
2020-09-16 19:02 ` Julia Suvorova
1 sibling, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2020-08-21 12:08 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Gerd Hoffmann, qemu-devel, Michael S. Tsirkin
On Tue, 18 Aug 2020 23:52:25 +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.
in addition to comment from Philippe
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> include/hw/acpi/pcihp.h | 3 ++-
> hw/acpi/pcihp.c | 10 ++++++----
> hw/acpi/piix4.c | 2 +-
> hw/i386/acpi-build.c | 25 ++++++++++++++-----------
> 4 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 8bc4a4c01d..1e9d246f57 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,
> + bool is_piix4);
>
> 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 9e31ab2da4..9a35ed6c83 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -38,7 +38,8 @@
> #include "qom/qom-qobject.h"
> #include "trace.h"
>
> -#define ACPI_PCIHP_ADDR 0xae00
> +#define ACPI_PCIHP_ADDR_PIIX4 0xae00
> +#define ACPI_PCIHP_ADDR_Q35 0x0cc4
> #define ACPI_PCIHP_SIZE 0x0014
> #define PCI_UP_BASE 0x0000
> #define PCI_DOWN_BASE 0x0004
> @@ -359,12 +360,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,
> + bool is_piix4)
> {
> s->io_len = ACPI_PCIHP_SIZE;
> - s->io_base = ACPI_PCIHP_ADDR;
> + s->io_base = is_piix4 ? ACPI_PCIHP_ADDR_PIIX4 : ACPI_PCIHP_ADDR_Q35;
>
> - 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 cdfa0e2998..1f27bfbd06 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -596,7 +596,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, true);
>
> 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 b7bcbbbb2a..f3cd52bd06 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)) {
Does pcie bus on hostbridge fall into pci_bus_is_express() categiry or not?
> 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_i386_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 + 0x08), 0x04));
^^^^
how about turning this offset into macro?
> 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 + 0x10), 0x04));
ditto
> 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,6 @@ 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_piix4_pci0_int(dsdt);
> } else {
> sb_scope = aml_scope("_SB");
> @@ -1526,6 +1527,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> }
> }
>
> + build_i386_pci_hotplug(dsdt, pm->pcihp_io_base);
> +
> if (pcmc->legacy_cpu_hotplug) {
> build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> } else {
> @@ -1546,7 +1549,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] 27+ messages in thread* Re: [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
2020-08-21 12:08 ` Igor Mammedov
@ 2020-09-16 19:02 ` Julia Suvorova
0 siblings, 0 replies; 27+ messages in thread
From: Julia Suvorova @ 2020-09-16 19:02 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Gerd Hoffmann, QEMU Developers, Michael S. Tsirkin
On Fri, Aug 21, 2020 at 2:08 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Tue, 18 Aug 2020 23:52:25 +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.
>
> in addition to comment from Philippe
>
>
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > include/hw/acpi/pcihp.h | 3 ++-
> > hw/acpi/pcihp.c | 10 ++++++----
> > hw/acpi/piix4.c | 2 +-
> > hw/i386/acpi-build.c | 25 ++++++++++++++-----------
> > 4 files changed, 23 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > index 8bc4a4c01d..1e9d246f57 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,
> > + bool is_piix4);
> >
> > 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 9e31ab2da4..9a35ed6c83 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -38,7 +38,8 @@
> > #include "qom/qom-qobject.h"
> > #include "trace.h"
> >
> > -#define ACPI_PCIHP_ADDR 0xae00
> > +#define ACPI_PCIHP_ADDR_PIIX4 0xae00
> > +#define ACPI_PCIHP_ADDR_Q35 0x0cc4
> > #define ACPI_PCIHP_SIZE 0x0014
> > #define PCI_UP_BASE 0x0000
> > #define PCI_DOWN_BASE 0x0004
> > @@ -359,12 +360,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,
> > + bool is_piix4)
> > {
> > s->io_len = ACPI_PCIHP_SIZE;
> > - s->io_base = ACPI_PCIHP_ADDR;
> > + s->io_base = is_piix4 ? ACPI_PCIHP_ADDR_PIIX4 : ACPI_PCIHP_ADDR_Q35;
> >
> > - 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 cdfa0e2998..1f27bfbd06 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -596,7 +596,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, true);
> >
> > 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 b7bcbbbb2a..f3cd52bd06 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)) {
> Does pcie bus on hostbridge fall into pci_bus_is_express() categiry or not?
I don't understand why it's there in the first place.
pci_bus_is_root() check is for pxb, but pci_bus_is_express() is
useless because everything is under 'if (pcihp_bridge_en) {', which
means that no pcie bus will get there (before this patch).
> > 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_i386_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 + 0x08), 0x04));
> ^^^^
> how about turning this offset into macro?
>
> > 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 + 0x10), 0x04));
> ditto
>
> > 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,6 @@ 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_piix4_pci0_int(dsdt);
> > } else {
> > sb_scope = aml_scope("_SB");
> > @@ -1526,6 +1527,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > }
> > }
> >
> > + build_i386_pci_hotplug(dsdt, pm->pcihp_io_base);
> > +
> > if (pcmc->legacy_cpu_hotplug) {
> > build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
> > } else {
> > @@ -1546,7 +1549,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] 27+ messages in thread
* [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC
2020-08-18 21:52 [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 Julia Suvorova
2020-08-18 21:52 ` [RFC PATCH v2 1/4] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb() Julia Suvorova
2020-08-18 21:52 ` [RFC PATCH v2 2/4] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35 Julia Suvorova
@ 2020-08-18 21:52 ` Julia Suvorova
2020-08-21 12:13 ` Igor Mammedov
2020-08-18 21:52 ` [RFC PATCH v2 4/4] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
` (3 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Julia Suvorova @ 2020-08-18 21:52 UTC (permalink / raw)
To: qemu-devel
Cc: Igor Mammedov, Julia Suvorova, Gerd Hoffmann, Michael S. Tsirkin
Other methods may be used if the system is capable of this and the _OSC bit
is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
versions will still use PCIe native.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
hw/i386/acpi-build.h | 11 +++++++++++
hw/i386/acpi-build.c | 21 +++++++++++++++------
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 74df5fc612..6f94312c39 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -5,6 +5,17 @@
extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
+/* PCI Firmware Specification 3.2, Table 4-5 */
+typedef enum {
+ ACPI_OSC_NATIVE_HP_EN = 0,
+ ACPI_OSC_SHPC_EN = 1,
+ ACPI_OSC_PME_EN = 2,
+ ACPI_OSC_AER_EN = 3,
+ ACPI_OSC_PCIE_CAP_EN = 4,
+ ACPI_OSC_LTR_EN = 5,
+ ACPI_OSC_ALLONES_INVALID = 6,
+} AcpiOSCField;
+
void acpi_setup(void);
#endif
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f3cd52bd06..c5f4802b8c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1411,7 +1411,7 @@ static void build_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
aml_append(table, scope);
}
-static Aml *build_q35_osc_method(void)
+static Aml *build_q35_osc_method(AcpiPmInfo *pm)
{
Aml *if_ctx;
Aml *if_ctx2;
@@ -1419,6 +1419,7 @@ static Aml *build_q35_osc_method(void)
Aml *method;
Aml *a_cwd1 = aml_name("CDW1");
Aml *a_ctrl = aml_local(0);
+ unsigned osc_ctrl;
method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
@@ -1430,11 +1431,19 @@ static Aml *build_q35_osc_method(void)
aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
+ /* Always allow native PME, AER (depend on PCIE Capability Control) */
+ osc_ctrl = BIT(ACPI_OSC_PME_EN) | BIT(ACPI_OSC_AER_EN) |
+ BIT(ACPI_OSC_PCIE_CAP_EN);
+
/*
- * Always allow native PME, AER (no dependencies)
- * Allow SHPC (PCI bridges can have SHPC controller)
+ * Guests seem to generally prefer native hot-plug control.
+ * Enable it only when we do not use ACPI hot-plug.
*/
- aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
+ if (!pm->pcihp_bridge_en) {
+ osc_ctrl |= BIT(ACPI_OSC_NATIVE_HP_EN) | BIT(ACPI_OSC_SHPC_EN);
+ }
+
+ aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
/* Unknown revision */
@@ -1514,7 +1523,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(1)));
- aml_append(dev, build_q35_osc_method());
+ aml_append(dev, build_q35_osc_method(pm));
aml_append(sb_scope, dev);
aml_append(dsdt, sb_scope);
@@ -1590,7 +1599,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
if (pci_bus_is_express(bus)) {
aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
- aml_append(dev, build_q35_osc_method());
+ aml_append(dev, build_q35_osc_method(pm));
} else {
aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
}
--
2.25.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC
2020-08-18 21:52 ` [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC Julia Suvorova
@ 2020-08-21 12:13 ` Igor Mammedov
2020-09-16 18:03 ` Julia Suvorova
0 siblings, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2020-08-21 12:13 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Gerd Hoffmann, qemu-devel, Michael S. Tsirkin
On Tue, 18 Aug 2020 23:52:26 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> Other methods may be used if the system is capable of this and the _OSC bit
> is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> versions will still use PCIe native.
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> hw/i386/acpi-build.h | 11 +++++++++++
> hw/i386/acpi-build.c | 21 +++++++++++++++------
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 74df5fc612..6f94312c39 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -5,6 +5,17 @@
>
> extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>
> +/* PCI Firmware Specification 3.2, Table 4-5 */
> +typedef enum {
> + ACPI_OSC_NATIVE_HP_EN = 0,
> + ACPI_OSC_SHPC_EN = 1,
> + ACPI_OSC_PME_EN = 2,
> + ACPI_OSC_AER_EN = 3,
> + ACPI_OSC_PCIE_CAP_EN = 4,
> + ACPI_OSC_LTR_EN = 5,
> + ACPI_OSC_ALLONES_INVALID = 6,
> +} AcpiOSCField;
> +
> void acpi_setup(void);
>
> #endif
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f3cd52bd06..c5f4802b8c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1411,7 +1411,7 @@ static void build_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> aml_append(table, scope);
> }
>
> -static Aml *build_q35_osc_method(void)
> +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> {
> Aml *if_ctx;
> Aml *if_ctx2;
> @@ -1419,6 +1419,7 @@ static Aml *build_q35_osc_method(void)
> Aml *method;
> Aml *a_cwd1 = aml_name("CDW1");
> Aml *a_ctrl = aml_local(0);
> + unsigned osc_ctrl;
>
> method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> @@ -1430,11 +1431,19 @@ static Aml *build_q35_osc_method(void)
>
> aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
>
> + /* Always allow native PME, AER (depend on PCIE Capability Control) */
> + osc_ctrl = BIT(ACPI_OSC_PME_EN) | BIT(ACPI_OSC_AER_EN) |
> + BIT(ACPI_OSC_PCIE_CAP_EN);
> +
> /*
> - * Always allow native PME, AER (no dependencies)
> - * Allow SHPC (PCI bridges can have SHPC controller)
> + * Guests seem to generally prefer native hot-plug control.
> + * Enable it only when we do not use ACPI hot-plug.
> */
> - aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> + if (!pm->pcihp_bridge_en) {
> + osc_ctrl |= BIT(ACPI_OSC_NATIVE_HP_EN) | BIT(ACPI_OSC_SHPC_EN);
> + }
ACPI hotplug works only for coldplugged bridges, and native one is used
on hotplugged ones.
Wouldn't that break SHPC/Native hotplug on hotplugged PCI/PCI-E bridge?
> + aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
>
> if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> /* Unknown revision */
> @@ -1514,7 +1523,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> - aml_append(dev, build_q35_osc_method());
> + aml_append(dev, build_q35_osc_method(pm));
> aml_append(sb_scope, dev);
> aml_append(dsdt, sb_scope);
>
> @@ -1590,7 +1599,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> if (pci_bus_is_express(bus)) {
> aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> - aml_append(dev, build_q35_osc_method());
> + aml_append(dev, build_q35_osc_method(pm));
> } else {
> aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> }
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC
2020-08-21 12:13 ` Igor Mammedov
@ 2020-09-16 18:03 ` Julia Suvorova
2020-09-16 19:14 ` Julia Suvorova
0 siblings, 1 reply; 27+ messages in thread
From: Julia Suvorova @ 2020-09-16 18:03 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Gerd Hoffmann, QEMU Developers, Michael S. Tsirkin
On Fri, Aug 21, 2020 at 2:13 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Tue, 18 Aug 2020 23:52:26 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > Other methods may be used if the system is capable of this and the _OSC bit
> > is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> > versions will still use PCIe native.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > hw/i386/acpi-build.h | 11 +++++++++++
> > hw/i386/acpi-build.c | 21 +++++++++++++++------
> > 2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > index 74df5fc612..6f94312c39 100644
> > --- a/hw/i386/acpi-build.h
> > +++ b/hw/i386/acpi-build.h
> > @@ -5,6 +5,17 @@
> >
> > extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> >
> > +/* PCI Firmware Specification 3.2, Table 4-5 */
> > +typedef enum {
> > + ACPI_OSC_NATIVE_HP_EN = 0,
> > + ACPI_OSC_SHPC_EN = 1,
> > + ACPI_OSC_PME_EN = 2,
> > + ACPI_OSC_AER_EN = 3,
> > + ACPI_OSC_PCIE_CAP_EN = 4,
> > + ACPI_OSC_LTR_EN = 5,
> > + ACPI_OSC_ALLONES_INVALID = 6,
> > +} AcpiOSCField;
> > +
> > void acpi_setup(void);
> >
> > #endif
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index f3cd52bd06..c5f4802b8c 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1411,7 +1411,7 @@ static void build_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> > aml_append(table, scope);
> > }
> >
> > -static Aml *build_q35_osc_method(void)
> > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > {
> > Aml *if_ctx;
> > Aml *if_ctx2;
> > @@ -1419,6 +1419,7 @@ static Aml *build_q35_osc_method(void)
> > Aml *method;
> > Aml *a_cwd1 = aml_name("CDW1");
> > Aml *a_ctrl = aml_local(0);
> > + unsigned osc_ctrl;
> >
> > method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> > aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> > @@ -1430,11 +1431,19 @@ static Aml *build_q35_osc_method(void)
> >
> > aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
> >
> > + /* Always allow native PME, AER (depend on PCIE Capability Control) */
> > + osc_ctrl = BIT(ACPI_OSC_PME_EN) | BIT(ACPI_OSC_AER_EN) |
> > + BIT(ACPI_OSC_PCIE_CAP_EN);
> > +
> > /*
> > - * Always allow native PME, AER (no dependencies)
> > - * Allow SHPC (PCI bridges can have SHPC controller)
> > + * Guests seem to generally prefer native hot-plug control.
> > + * Enable it only when we do not use ACPI hot-plug.
> > */
> > - aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > + if (!pm->pcihp_bridge_en) {
> > + osc_ctrl |= BIT(ACPI_OSC_NATIVE_HP_EN) | BIT(ACPI_OSC_SHPC_EN);
> > + }
>
> ACPI hotplug works only for coldplugged bridges, and native one is used
> on hotplugged ones.
> Wouldn't that break SHPC/Native hotplug on hotplugged PCI/PCI-E bridge?
Yes, it would. I'll mention it in the commit message.
> > + aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
> >
> > if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> > /* Unknown revision */
> > @@ -1514,7 +1523,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> > - aml_append(dev, build_q35_osc_method());
> > + aml_append(dev, build_q35_osc_method(pm));
> > aml_append(sb_scope, dev);
> > aml_append(dsdt, sb_scope);
> >
> > @@ -1590,7 +1599,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > if (pci_bus_is_express(bus)) {
> > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > - aml_append(dev, build_q35_osc_method());
> > + aml_append(dev, build_q35_osc_method(pm));
> > } else {
> > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > }
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC
2020-09-16 18:03 ` Julia Suvorova
@ 2020-09-16 19:14 ` Julia Suvorova
2020-09-17 6:01 ` Igor Mammedov
0 siblings, 1 reply; 27+ messages in thread
From: Julia Suvorova @ 2020-09-16 19:14 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Gerd Hoffmann, QEMU Developers, Michael S. Tsirkin
On Wed, Sep 16, 2020 at 8:03 PM Julia Suvorova <jusual@redhat.com> wrote:
>
> On Fri, Aug 21, 2020 at 2:13 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Tue, 18 Aug 2020 23:52:26 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >
> > > Other methods may be used if the system is capable of this and the _OSC bit
> > > is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> > > versions will still use PCIe native.
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > > hw/i386/acpi-build.h | 11 +++++++++++
> > > hw/i386/acpi-build.c | 21 +++++++++++++++------
> > > 2 files changed, 26 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > > index 74df5fc612..6f94312c39 100644
> > > --- a/hw/i386/acpi-build.h
> > > +++ b/hw/i386/acpi-build.h
> > > @@ -5,6 +5,17 @@
> > >
> > > extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> > >
> > > +/* PCI Firmware Specification 3.2, Table 4-5 */
> > > +typedef enum {
> > > + ACPI_OSC_NATIVE_HP_EN = 0,
> > > + ACPI_OSC_SHPC_EN = 1,
> > > + ACPI_OSC_PME_EN = 2,
> > > + ACPI_OSC_AER_EN = 3,
> > > + ACPI_OSC_PCIE_CAP_EN = 4,
> > > + ACPI_OSC_LTR_EN = 5,
> > > + ACPI_OSC_ALLONES_INVALID = 6,
> > > +} AcpiOSCField;
> > > +
> > > void acpi_setup(void);
> > >
> > > #endif
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index f3cd52bd06..c5f4802b8c 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1411,7 +1411,7 @@ static void build_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> > > aml_append(table, scope);
> > > }
> > >
> > > -static Aml *build_q35_osc_method(void)
> > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > > {
> > > Aml *if_ctx;
> > > Aml *if_ctx2;
> > > @@ -1419,6 +1419,7 @@ static Aml *build_q35_osc_method(void)
> > > Aml *method;
> > > Aml *a_cwd1 = aml_name("CDW1");
> > > Aml *a_ctrl = aml_local(0);
> > > + unsigned osc_ctrl;
> > >
> > > method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> > > aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> > > @@ -1430,11 +1431,19 @@ static Aml *build_q35_osc_method(void)
> > >
> > > aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
> > >
> > > + /* Always allow native PME, AER (depend on PCIE Capability Control) */
> > > + osc_ctrl = BIT(ACPI_OSC_PME_EN) | BIT(ACPI_OSC_AER_EN) |
> > > + BIT(ACPI_OSC_PCIE_CAP_EN);
> > > +
> > > /*
> > > - * Always allow native PME, AER (no dependencies)
> > > - * Allow SHPC (PCI bridges can have SHPC controller)
> > > + * Guests seem to generally prefer native hot-plug control.
> > > + * Enable it only when we do not use ACPI hot-plug.
> > > */
> > > - aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > > + if (!pm->pcihp_bridge_en) {
> > > + osc_ctrl |= BIT(ACPI_OSC_NATIVE_HP_EN) | BIT(ACPI_OSC_SHPC_EN);
> > > + }
> >
> > ACPI hotplug works only for coldplugged bridges, and native one is used
> > on hotplugged ones.
> > Wouldn't that break SHPC/Native hotplug on hotplugged PCI/PCI-E bridge?
Wait, what configuration are you talking about exactly?
> > > + aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
> > >
> > > if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> > > /* Unknown revision */
> > > @@ -1514,7 +1523,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> > > - aml_append(dev, build_q35_osc_method());
> > > + aml_append(dev, build_q35_osc_method(pm));
> > > aml_append(sb_scope, dev);
> > > aml_append(dsdt, sb_scope);
> > >
> > > @@ -1590,7 +1599,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > if (pci_bus_is_express(bus)) {
> > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > - aml_append(dev, build_q35_osc_method());
> > > + aml_append(dev, build_q35_osc_method(pm));
> > > } else {
> > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > > }
> >
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC
2020-09-16 19:14 ` Julia Suvorova
@ 2020-09-17 6:01 ` Igor Mammedov
2020-09-17 10:40 ` Julia Suvorova
0 siblings, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2020-09-17 6:01 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Gerd Hoffmann, QEMU Developers, Michael S. Tsirkin
On Wed, 16 Sep 2020 21:14:36 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> On Wed, Sep 16, 2020 at 8:03 PM Julia Suvorova <jusual@redhat.com> wrote:
> >
> > On Fri, Aug 21, 2020 at 2:13 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Tue, 18 Aug 2020 23:52:26 +0200
> > > Julia Suvorova <jusual@redhat.com> wrote:
> > >
> > > > Other methods may be used if the system is capable of this and the _OSC bit
> > > > is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> > > > versions will still use PCIe native.
> > > >
> > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > ---
> > > > hw/i386/acpi-build.h | 11 +++++++++++
> > > > hw/i386/acpi-build.c | 21 +++++++++++++++------
> > > > 2 files changed, 26 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > > > index 74df5fc612..6f94312c39 100644
> > > > --- a/hw/i386/acpi-build.h
> > > > +++ b/hw/i386/acpi-build.h
> > > > @@ -5,6 +5,17 @@
> > > >
> > > > extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> > > >
> > > > +/* PCI Firmware Specification 3.2, Table 4-5 */
> > > > +typedef enum {
> > > > + ACPI_OSC_NATIVE_HP_EN = 0,
> > > > + ACPI_OSC_SHPC_EN = 1,
> > > > + ACPI_OSC_PME_EN = 2,
> > > > + ACPI_OSC_AER_EN = 3,
> > > > + ACPI_OSC_PCIE_CAP_EN = 4,
> > > > + ACPI_OSC_LTR_EN = 5,
> > > > + ACPI_OSC_ALLONES_INVALID = 6,
> > > > +} AcpiOSCField;
> > > > +
> > > > void acpi_setup(void);
> > > >
> > > > #endif
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index f3cd52bd06..c5f4802b8c 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -1411,7 +1411,7 @@ static void build_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> > > > aml_append(table, scope);
> > > > }
> > > >
> > > > -static Aml *build_q35_osc_method(void)
> > > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > > > {
> > > > Aml *if_ctx;
> > > > Aml *if_ctx2;
> > > > @@ -1419,6 +1419,7 @@ static Aml *build_q35_osc_method(void)
> > > > Aml *method;
> > > > Aml *a_cwd1 = aml_name("CDW1");
> > > > Aml *a_ctrl = aml_local(0);
> > > > + unsigned osc_ctrl;
> > > >
> > > > method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> > > > aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> > > > @@ -1430,11 +1431,19 @@ static Aml *build_q35_osc_method(void)
> > > >
> > > > aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
> > > >
> > > > + /* Always allow native PME, AER (depend on PCIE Capability Control) */
> > > > + osc_ctrl = BIT(ACPI_OSC_PME_EN) | BIT(ACPI_OSC_AER_EN) |
> > > > + BIT(ACPI_OSC_PCIE_CAP_EN);
> > > > +
> > > > /*
> > > > - * Always allow native PME, AER (no dependencies)
> > > > - * Allow SHPC (PCI bridges can have SHPC controller)
> > > > + * Guests seem to generally prefer native hot-plug control.
> > > > + * Enable it only when we do not use ACPI hot-plug.
> > > > */
> > > > - aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > > > + if (!pm->pcihp_bridge_en) {
> > > > + osc_ctrl |= BIT(ACPI_OSC_NATIVE_HP_EN) | BIT(ACPI_OSC_SHPC_EN);
> > > > + }
> > >
> > > ACPI hotplug works only for coldplugged bridges, and native one is used
> > > on hotplugged ones.
> > > Wouldn't that break SHPC/Native hotplug on hotplugged PCI/PCI-E bridge?
>
> Wait, what configuration are you talking about exactly?
Currently on piix4, we have ACPI and native hotplug working simultaneously
the former works on cold-plugged bridges and if you hotplug another bridge,
that one will use native method.
With above hunk it probably will break.
>
> > > > + aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
> > > >
> > > > if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> > > > /* Unknown revision */
> > > > @@ -1514,7 +1523,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > > aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > > aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> > > > - aml_append(dev, build_q35_osc_method());
> > > > + aml_append(dev, build_q35_osc_method(pm));
> > > > aml_append(sb_scope, dev);
> > > > aml_append(dsdt, sb_scope);
> > > >
> > > > @@ -1590,7 +1599,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > if (pci_bus_is_express(bus)) {
> > > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > > - aml_append(dev, build_q35_osc_method());
> > > > + aml_append(dev, build_q35_osc_method(pm));
> > > > } else {
> > > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > > > }
> > >
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC
2020-09-17 6:01 ` Igor Mammedov
@ 2020-09-17 10:40 ` Julia Suvorova
2020-09-18 6:50 ` Igor Mammedov
0 siblings, 1 reply; 27+ messages in thread
From: Julia Suvorova @ 2020-09-17 10:40 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Gerd Hoffmann, QEMU Developers, Michael S. Tsirkin
On Thu, Sep 17, 2020 at 8:01 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 16 Sep 2020 21:14:36 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > On Wed, Sep 16, 2020 at 8:03 PM Julia Suvorova <jusual@redhat.com> wrote:
> > >
> > > On Fri, Aug 21, 2020 at 2:13 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > >
> > > > On Tue, 18 Aug 2020 23:52:26 +0200
> > > > Julia Suvorova <jusual@redhat.com> wrote:
> > > >
> > > > > Other methods may be used if the system is capable of this and the _OSC bit
> > > > > is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> > > > > versions will still use PCIe native.
> > > > >
> > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > > ---
> > > > > hw/i386/acpi-build.h | 11 +++++++++++
> > > > > hw/i386/acpi-build.c | 21 +++++++++++++++------
> > > > > 2 files changed, 26 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > > > > index 74df5fc612..6f94312c39 100644
> > > > > --- a/hw/i386/acpi-build.h
> > > > > +++ b/hw/i386/acpi-build.h
> > > > > @@ -5,6 +5,17 @@
> > > > >
> > > > > extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> > > > >
> > > > > +/* PCI Firmware Specification 3.2, Table 4-5 */
> > > > > +typedef enum {
> > > > > + ACPI_OSC_NATIVE_HP_EN = 0,
> > > > > + ACPI_OSC_SHPC_EN = 1,
> > > > > + ACPI_OSC_PME_EN = 2,
> > > > > + ACPI_OSC_AER_EN = 3,
> > > > > + ACPI_OSC_PCIE_CAP_EN = 4,
> > > > > + ACPI_OSC_LTR_EN = 5,
> > > > > + ACPI_OSC_ALLONES_INVALID = 6,
> > > > > +} AcpiOSCField;
> > > > > +
> > > > > void acpi_setup(void);
> > > > >
> > > > > #endif
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index f3cd52bd06..c5f4802b8c 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -1411,7 +1411,7 @@ static void build_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> > > > > aml_append(table, scope);
> > > > > }
> > > > >
> > > > > -static Aml *build_q35_osc_method(void)
> > > > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > > > > {
> > > > > Aml *if_ctx;
> > > > > Aml *if_ctx2;
> > > > > @@ -1419,6 +1419,7 @@ static Aml *build_q35_osc_method(void)
> > > > > Aml *method;
> > > > > Aml *a_cwd1 = aml_name("CDW1");
> > > > > Aml *a_ctrl = aml_local(0);
> > > > > + unsigned osc_ctrl;
> > > > >
> > > > > method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> > > > > aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> > > > > @@ -1430,11 +1431,19 @@ static Aml *build_q35_osc_method(void)
> > > > >
> > > > > aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
> > > > >
> > > > > + /* Always allow native PME, AER (depend on PCIE Capability Control) */
> > > > > + osc_ctrl = BIT(ACPI_OSC_PME_EN) | BIT(ACPI_OSC_AER_EN) |
> > > > > + BIT(ACPI_OSC_PCIE_CAP_EN);
> > > > > +
> > > > > /*
> > > > > - * Always allow native PME, AER (no dependencies)
> > > > > - * Allow SHPC (PCI bridges can have SHPC controller)
> > > > > + * Guests seem to generally prefer native hot-plug control.
> > > > > + * Enable it only when we do not use ACPI hot-plug.
> > > > > */
> > > > > - aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > > > > + if (!pm->pcihp_bridge_en) {
> > > > > + osc_ctrl |= BIT(ACPI_OSC_NATIVE_HP_EN) | BIT(ACPI_OSC_SHPC_EN);
> > > > > + }
> > > >
> > > > ACPI hotplug works only for coldplugged bridges, and native one is used
> > > > on hotplugged ones.
> > > > Wouldn't that break SHPC/Native hotplug on hotplugged PCI/PCI-E bridge?
> >
> > Wait, what configuration are you talking about exactly?
> Currently on piix4, we have ACPI and native hotplug working simultaneously
> the former works on cold-plugged bridges and if you hotplug another bridge,
> that one will use native method.
> With above hunk it probably will break.
Ok, I will add a check for piix4.
> >
> > > > > + aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
> > > > >
> > > > > if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> > > > > /* Unknown revision */
> > > > > @@ -1514,7 +1523,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > > > aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > > > aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> > > > > - aml_append(dev, build_q35_osc_method());
> > > > > + aml_append(dev, build_q35_osc_method(pm));
> > > > > aml_append(sb_scope, dev);
> > > > > aml_append(dsdt, sb_scope);
> > > > >
> > > > > @@ -1590,7 +1599,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > > if (pci_bus_is_express(bus)) {
> > > > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > > > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > > > - aml_append(dev, build_q35_osc_method());
> > > > > + aml_append(dev, build_q35_osc_method(pm));
> > > > > } else {
> > > > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > > > > }
> > > >
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC
2020-09-17 10:40 ` Julia Suvorova
@ 2020-09-18 6:50 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2020-09-18 6:50 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Gerd Hoffmann, QEMU Developers, Michael S. Tsirkin
On Thu, 17 Sep 2020 12:40:16 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> On Thu, Sep 17, 2020 at 8:01 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 16 Sep 2020 21:14:36 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >
> > > On Wed, Sep 16, 2020 at 8:03 PM Julia Suvorova <jusual@redhat.com> wrote:
> > > >
> > > > On Fri, Aug 21, 2020 at 2:13 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > > >
> > > > > On Tue, 18 Aug 2020 23:52:26 +0200
> > > > > Julia Suvorova <jusual@redhat.com> wrote:
> > > > >
> > > > > > Other methods may be used if the system is capable of this and the _OSC bit
> > > > > > is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> > > > > > versions will still use PCIe native.
> > > > > >
> > > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > > > ---
> > > > > > hw/i386/acpi-build.h | 11 +++++++++++
> > > > > > hw/i386/acpi-build.c | 21 +++++++++++++++------
> > > > > > 2 files changed, 26 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > > > > > index 74df5fc612..6f94312c39 100644
> > > > > > --- a/hw/i386/acpi-build.h
> > > > > > +++ b/hw/i386/acpi-build.h
> > > > > > @@ -5,6 +5,17 @@
> > > > > >
> > > > > > extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
> > > > > >
> > > > > > +/* PCI Firmware Specification 3.2, Table 4-5 */
> > > > > > +typedef enum {
> > > > > > + ACPI_OSC_NATIVE_HP_EN = 0,
> > > > > > + ACPI_OSC_SHPC_EN = 1,
> > > > > > + ACPI_OSC_PME_EN = 2,
> > > > > > + ACPI_OSC_AER_EN = 3,
> > > > > > + ACPI_OSC_PCIE_CAP_EN = 4,
> > > > > > + ACPI_OSC_LTR_EN = 5,
> > > > > > + ACPI_OSC_ALLONES_INVALID = 6,
> > > > > > +} AcpiOSCField;
> > > > > > +
> > > > > > void acpi_setup(void);
> > > > > >
> > > > > > #endif
> > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > index f3cd52bd06..c5f4802b8c 100644
> > > > > > --- a/hw/i386/acpi-build.c
> > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > @@ -1411,7 +1411,7 @@ static void build_i386_pci_hotplug(Aml *table, uint64_t pcihp_addr)
> > > > > > aml_append(table, scope);
> > > > > > }
> > > > > >
> > > > > > -static Aml *build_q35_osc_method(void)
> > > > > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > > > > > {
> > > > > > Aml *if_ctx;
> > > > > > Aml *if_ctx2;
> > > > > > @@ -1419,6 +1419,7 @@ static Aml *build_q35_osc_method(void)
> > > > > > Aml *method;
> > > > > > Aml *a_cwd1 = aml_name("CDW1");
> > > > > > Aml *a_ctrl = aml_local(0);
> > > > > > + unsigned osc_ctrl;
> > > > > >
> > > > > > method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> > > > > > aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> > > > > > @@ -1430,11 +1431,19 @@ static Aml *build_q35_osc_method(void)
> > > > > >
> > > > > > aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl));
> > > > > >
> > > > > > + /* Always allow native PME, AER (depend on PCIE Capability Control) */
> > > > > > + osc_ctrl = BIT(ACPI_OSC_PME_EN) | BIT(ACPI_OSC_AER_EN) |
> > > > > > + BIT(ACPI_OSC_PCIE_CAP_EN);
> > > > > > +
> > > > > > /*
> > > > > > - * Always allow native PME, AER (no dependencies)
> > > > > > - * Allow SHPC (PCI bridges can have SHPC controller)
> > > > > > + * Guests seem to generally prefer native hot-plug control.
> > > > > > + * Enable it only when we do not use ACPI hot-plug.
> > > > > > */
> > > > > > - aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > > > > > + if (!pm->pcihp_bridge_en) {
> > > > > > + osc_ctrl |= BIT(ACPI_OSC_NATIVE_HP_EN) | BIT(ACPI_OSC_SHPC_EN);
> > > > > > + }
> > > > >
> > > > > ACPI hotplug works only for coldplugged bridges, and native one is used
> > > > > on hotplugged ones.
> > > > > Wouldn't that break SHPC/Native hotplug on hotplugged PCI/PCI-E bridge?
> > >
> > > Wait, what configuration are you talking about exactly?
> > Currently on piix4, we have ACPI and native hotplug working simultaneously
> > the former works on cold-plugged bridges and if you hotplug another bridge,
> > that one will use native method.
> > With above hunk it probably will break.
>
> Ok, I will add a check for piix4.
can we have the same behavior for q35 as well?
i.e.
could-plugged => ACPI hotplug
hotplugged ports, bridges, whatnot => native hotplug
>
> > >
> > > > > > + aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
> > > > > >
> > > > > > if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> > > > > > /* Unknown revision */
> > > > > > @@ -1514,7 +1523,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > > > > aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > > > > aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> > > > > > - aml_append(dev, build_q35_osc_method());
> > > > > > + aml_append(dev, build_q35_osc_method(pm));
> > > > > > aml_append(sb_scope, dev);
> > > > > > aml_append(dsdt, sb_scope);
> > > > > >
> > > > > > @@ -1590,7 +1599,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > > > if (pci_bus_is_express(bus)) {
> > > > > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > > > > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > > > > - aml_append(dev, build_q35_osc_method());
> > > > > > + aml_append(dev, build_q35_osc_method(pm));
> > > > > > } else {
> > > > > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > > > > > }
> > > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH v2 4/4] hw/acpi/ich9: Enable ACPI PCI hot-plug
2020-08-18 21:52 [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 Julia Suvorova
` (2 preceding siblings ...)
2020-08-18 21:52 ` [RFC PATCH v2 3/4] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC Julia Suvorova
@ 2020-08-18 21:52 ` Julia Suvorova
2020-08-21 12:24 ` Igor Mammedov
2020-08-18 22:29 ` [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 no-reply
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Julia Suvorova @ 2020-08-18 21:52 UTC (permalink / raw)
To: qemu-devel
Cc: Igor Mammedov, Julia Suvorova, Gerd Hoffmann, Michael S. Tsirkin
Add acpi_pcihp to ich9_pm and use ACPI PCI hot-plug by default.
Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
Note: New pc_compats are usually added shortly after release.
I will switch to pc_compat_5_1 when it becomes available.
hw/i386/acpi-build.h | 1 +
include/hw/acpi/ich9.h | 3 +++
hw/acpi/ich9.c | 45 ++++++++++++++++++++++++++++++++++++++++++
hw/acpi/pcihp.c | 5 ++++-
hw/i386/acpi-build.c | 2 +-
hw/i386/pc.c | 1 +
6 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 6f94312c39..f0bb080018 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -17,5 +17,6 @@ typedef enum {
} AcpiOSCField;
void acpi_setup(void);
+Object *acpi_get_i386_pci_host(void);
#endif
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 28a53181cb..9947085e9c 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"
@@ -53,6 +54,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 a2a1742aa6..fde86d12ae 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -265,6 +265,7 @@ static void pm_reset(void *opaque)
}
pm->smi_en_wmask = ~0;
+ acpi_pcihp_reset(&pm->acpi_pci_hotplug);
acpi_update_sci(&pm->acpi_regs, pm->irq);
}
@@ -303,6 +304,17 @@ 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, false);
+
+ 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;
@@ -374,6 +386,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;
@@ -382,6 +408,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 = true;
object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
&pm->pm_io_base, OBJ_PROP_FLAG_READ);
@@ -405,6 +432,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,
@@ -412,6 +442,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,
@@ -437,6 +472,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)));
@@ -457,6 +495,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)));
@@ -474,6 +516,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 9a35ed6c83..46ebd1bb15 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"
@@ -90,6 +92,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;
@@ -98,7 +101,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 c5f4802b8c..8cecdf722f 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;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 47c5ca3e34..175e6911a1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -98,6 +98,7 @@
#include "trace.h"
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] 27+ messages in thread* Re: [RFC PATCH v2 4/4] hw/acpi/ich9: Enable ACPI PCI hot-plug
2020-08-18 21:52 ` [RFC PATCH v2 4/4] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
@ 2020-08-21 12:24 ` Igor Mammedov
2020-09-16 19:28 ` Julia Suvorova
0 siblings, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2020-08-21 12:24 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Michael S. Tsirkin, qemu-devel, Gerd Hoffmann
On Tue, 18 Aug 2020 23:52:27 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> Add acpi_pcihp to ich9_pm and use ACPI PCI hot-plug by default.
I don't see anything related to migrating hotplug state within series,
see VMSTATE_PCI_HOTPLUG for example.
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> Note: New pc_compats are usually added shortly after release.
> I will switch to pc_compat_5_1 when it becomes available.
>
> hw/i386/acpi-build.h | 1 +
> include/hw/acpi/ich9.h | 3 +++
> hw/acpi/ich9.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> hw/acpi/pcihp.c | 5 ++++-
> hw/i386/acpi-build.c | 2 +-
> hw/i386/pc.c | 1 +
> 6 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 6f94312c39..f0bb080018 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -17,5 +17,6 @@ typedef enum {
> } AcpiOSCField;
>
> void acpi_setup(void);
> +Object *acpi_get_i386_pci_host(void);
>
> #endif
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 28a53181cb..9947085e9c 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"
> @@ -53,6 +54,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 a2a1742aa6..fde86d12ae 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -265,6 +265,7 @@ static void pm_reset(void *opaque)
> }
> pm->smi_en_wmask = ~0;
>
> + acpi_pcihp_reset(&pm->acpi_pci_hotplug);
> acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
>
> @@ -303,6 +304,17 @@ 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, false);
> +
> + 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;
> @@ -374,6 +386,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;
> @@ -382,6 +408,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 = true;
I'd prefer default to be false, and managment turn it on when creating
config for Windows.
i.e. if users need it, they should enable it explicitly so overhead it produces
won't affect other configs.
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> @@ -405,6 +432,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,
> @@ -412,6 +442,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,
> @@ -437,6 +472,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)));
> @@ -457,6 +495,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)));
> @@ -474,6 +516,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 9a35ed6c83..46ebd1bb15 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"
> @@ -90,6 +92,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;
>
> @@ -98,7 +101,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 c5f4802b8c..8cecdf722f 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;
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 47c5ca3e34..175e6911a1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -98,6 +98,7 @@
> #include "trace.h"
>
> 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] 27+ messages in thread* Re: [RFC PATCH v2 4/4] hw/acpi/ich9: Enable ACPI PCI hot-plug
2020-08-21 12:24 ` Igor Mammedov
@ 2020-09-16 19:28 ` Julia Suvorova
0 siblings, 0 replies; 27+ messages in thread
From: Julia Suvorova @ 2020-09-16 19:28 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Michael S. Tsirkin, QEMU Developers, Gerd Hoffmann
On Fri, Aug 21, 2020 at 2:24 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Tue, 18 Aug 2020 23:52:27 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > Add acpi_pcihp to ich9_pm and use ACPI PCI hot-plug by default.
>
> I don't see anything related to migrating hotplug state within series,
>
> see VMSTATE_PCI_HOTPLUG for example.
Ok, will do.
>
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > Note: New pc_compats are usually added shortly after release.
> > I will switch to pc_compat_5_1 when it becomes available.
> >
> > hw/i386/acpi-build.h | 1 +
> > include/hw/acpi/ich9.h | 3 +++
> > hw/acpi/ich9.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> > hw/acpi/pcihp.c | 5 ++++-
> > hw/i386/acpi-build.c | 2 +-
> > hw/i386/pc.c | 1 +
> > 6 files changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > index 6f94312c39..f0bb080018 100644
> > --- a/hw/i386/acpi-build.h
> > +++ b/hw/i386/acpi-build.h
> > @@ -17,5 +17,6 @@ typedef enum {
> > } AcpiOSCField;
> >
> > void acpi_setup(void);
> > +Object *acpi_get_i386_pci_host(void);
> >
> > #endif
> > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > index 28a53181cb..9947085e9c 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"
> > @@ -53,6 +54,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 a2a1742aa6..fde86d12ae 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -265,6 +265,7 @@ static void pm_reset(void *opaque)
> > }
> > pm->smi_en_wmask = ~0;
> >
> > + acpi_pcihp_reset(&pm->acpi_pci_hotplug);
> > acpi_update_sci(&pm->acpi_regs, pm->irq);
> > }
> >
> > @@ -303,6 +304,17 @@ 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, false);
> > +
> > + 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;
> > @@ -374,6 +386,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;
> > @@ -382,6 +408,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 = true;
>
> I'd prefer default to be false, and managment turn it on when creating
> config for Windows.
>
> i.e. if users need it, they should enable it explicitly so overhead it produces
> won't affect other configs.
>
> > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> > &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> > @@ -405,6 +432,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,
> > @@ -412,6 +442,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,
> > @@ -437,6 +472,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)));
> > @@ -457,6 +495,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)));
> > @@ -474,6 +516,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 9a35ed6c83..46ebd1bb15 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"
> > @@ -90,6 +92,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;
> >
> > @@ -98,7 +101,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 c5f4802b8c..8cecdf722f 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;
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 47c5ca3e34..175e6911a1 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -98,6 +98,7 @@
> > #include "trace.h"
> >
> > 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] 27+ messages in thread
* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-18 21:52 [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 Julia Suvorova
` (3 preceding siblings ...)
2020-08-18 21:52 ` [RFC PATCH v2 4/4] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
@ 2020-08-18 22:29 ` no-reply
2020-08-21 3:41 ` Michael S. Tsirkin
2020-08-21 10:30 ` Igor Mammedov
6 siblings, 0 replies; 27+ messages in thread
From: no-reply @ 2020-08-18 22:29 UTC (permalink / raw)
To: jusual; +Cc: imammedo, mst, jusual, qemu-devel, kraxel
Patchew URL: https://patchew.org/QEMU/20200818215227.181654-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 ===
acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-BFBCP0], Expected [aml:tests/data/acpi/pc/DSDT].
See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and re-run tests with V=1 environment variable set**
ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:494:test_acpi_asl: assertion failed: (all_tables_match)
ERROR bios-tables-test - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:494:test_acpi_asl: assertion failed: (all_tables_match)
make: *** [check-qtest-x86_64] Error 1
make: *** Waiting for unfinished jobs....
qemu-system-aarch64: -accel kvm: invalid accelerator kvm
qemu-system-aarch64: falling back to tcg
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=89a74ccc1bb04197855ac348bd5b106a', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-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-0hn12f1e/src/docker-src.2020-08-18-18.16.37.21604:/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=89a74ccc1bb04197855ac348bd5b106a
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-0hn12f1e/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 12m47.487s
user 0m8.668s
The full log is available at
http://patchew.org/logs/20200818215227.181654-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] 27+ messages in thread* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-18 21:52 [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 Julia Suvorova
` (4 preceding siblings ...)
2020-08-18 22:29 ` [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 no-reply
@ 2020-08-21 3:41 ` Michael S. Tsirkin
2020-08-21 10:30 ` Igor Mammedov
6 siblings, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2020-08-21 3:41 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Igor Mammedov, qemu-devel, Gerd Hoffmann
On Tue, Aug 18, 2020 at 11:52:23PM +0200, Julia Suvorova wrote:
> PCIe native hot-plug has numerous problems with racing events and unpredictable
> guest behaviour (Windows). Switching to ACPI hot-plug for now.
>
> Tested on RHEL 8 and Windows 2019.
> pxb-pcie is not yet supported.
Julia, pls update expected acpi tables, see tests/qtest/bios-tables-test.c
> 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 (4):
> hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
> hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
> hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC
> in _OSC
> hw/acpi/ich9: Enable ACPI PCI hot-plug
>
> hw/i386/acpi-build.h | 12 ++++++++++
> include/hw/acpi/ich9.h | 3 +++
> include/hw/acpi/pcihp.h | 3 ++-
> hw/acpi/ich9.c | 52 ++++++++++++++++++++++++++++++++++++++++-
> hw/acpi/pcihp.c | 15 ++++++++----
> hw/acpi/piix4.c | 2 +-
> hw/i386/acpi-build.c | 48 +++++++++++++++++++++++--------------
> hw/i386/pc.c | 1 +
> hw/acpi/trace-events | 4 ++++
> 9 files changed, 114 insertions(+), 26 deletions(-)
>
> --
> 2.25.4
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-18 21:52 [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35 Julia Suvorova
` (5 preceding siblings ...)
2020-08-21 3:41 ` Michael S. Tsirkin
@ 2020-08-21 10:30 ` Igor Mammedov
2020-08-21 12:29 ` Igor Mammedov
2020-08-22 14:25 ` Laszlo Ersek
6 siblings, 2 replies; 27+ messages in thread
From: Igor Mammedov @ 2020-08-21 10:30 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Michael S. Tsirkin, qemu-devel, Gerd Hoffmann
On Tue, 18 Aug 2020 23:52:23 +0200
Julia Suvorova <jusual@redhat.com> wrote:
> PCIe native hot-plug has numerous problems with racing events and unpredictable
> guest behaviour (Windows).
Documenting these misterious problems I've asked for in previous review
hasn't been addressed.
Pls see v1 for comments and add requested info into cover letter at least
or in a commit message.
> Switching to ACPI hot-plug for now.
>
> Tested on RHEL 8 and Windows 2019.
> pxb-pcie is not yet supported.
>
> 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 (4):
> hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
> hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
> hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC
> in _OSC
> hw/acpi/ich9: Enable ACPI PCI hot-plug
>
> hw/i386/acpi-build.h | 12 ++++++++++
> include/hw/acpi/ich9.h | 3 +++
> include/hw/acpi/pcihp.h | 3 ++-
> hw/acpi/ich9.c | 52 ++++++++++++++++++++++++++++++++++++++++-
> hw/acpi/pcihp.c | 15 ++++++++----
> hw/acpi/piix4.c | 2 +-
> hw/i386/acpi-build.c | 48 +++++++++++++++++++++++--------------
> hw/i386/pc.c | 1 +
> hw/acpi/trace-events | 4 ++++
> 9 files changed, 114 insertions(+), 26 deletions(-)
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-21 10:30 ` Igor Mammedov
@ 2020-08-21 12:29 ` Igor Mammedov
2020-08-22 14:25 ` Laszlo Ersek
1 sibling, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2020-08-21 12:29 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Gerd Hoffmann, qemu-devel, Michael S. Tsirkin
On Fri, 21 Aug 2020 12:30:07 +0200
Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 18 Aug 2020 23:52:23 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > PCIe native hot-plug has numerous problems with racing events and unpredictable
> > guest behaviour (Windows).
> Documenting these misterious problems I've asked for in previous review
> hasn't been addressed.
> Pls see v1 for comments and add requested info into cover letter at least
> or in a commit message.
>
>
> > Switching to ACPI hot-plug for now.
in addition to above it looks like the way it's implemented,
it's either all on or all off.
Don't we want to have it per bridge?
What about per port that's possible with native.
We also should document (other than in C) expectations
toward this feature.
> >
> > Tested on RHEL 8 and Windows 2019.
> > pxb-pcie is not yet supported.
> >
> > 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 (4):
> > hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
> > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
> > hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC
> > in _OSC
> > hw/acpi/ich9: Enable ACPI PCI hot-plug
> >
> > hw/i386/acpi-build.h | 12 ++++++++++
> > include/hw/acpi/ich9.h | 3 +++
> > include/hw/acpi/pcihp.h | 3 ++-
> > hw/acpi/ich9.c | 52 ++++++++++++++++++++++++++++++++++++++++-
> > hw/acpi/pcihp.c | 15 ++++++++----
> > hw/acpi/piix4.c | 2 +-
> > hw/i386/acpi-build.c | 48 +++++++++++++++++++++++--------------
> > hw/i386/pc.c | 1 +
> > hw/acpi/trace-events | 4 ++++
> > 9 files changed, 114 insertions(+), 26 deletions(-)
> >
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-21 10:30 ` Igor Mammedov
2020-08-21 12:29 ` Igor Mammedov
@ 2020-08-22 14:25 ` Laszlo Ersek
2020-08-24 11:35 ` Igor Mammedov
1 sibling, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-08-22 14:25 UTC (permalink / raw)
To: Igor Mammedov, Julia Suvorova
Cc: Daniel P. Berrange, Michael S. Tsirkin, qemu-devel,
Gerd Hoffmann, Laine Stump
+Marcel, Laine, Daniel
On 08/21/20 12:30, Igor Mammedov wrote:
> On Tue, 18 Aug 2020 23:52:23 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
>> PCIe native hot-plug has numerous problems with racing events and
>> unpredictable guest behaviour (Windows).
> Documenting these misterious problems I've asked for in previous
> review hasn't been addressed.
> Pls see v1 for comments and add requested info into cover letter at
> least or in a commit message.
Igor, I assume you are referring to
http://mid.mail-archive.com/20200715153321.3495e62d@redhat.com
and I couldn't agree more.
I'd like to understand the specific motivation for this patch series.
- I'm very concerned that it could regress various hotplug scenarios
with at least OVMF.
So minimally I'm hoping that the work is being meticulously tested with
OVMF.
- I don't recall testing native PCIe hot-*unplug*, but we had repeatedly
tested native PCIe plug with both Linux and Windows guests, and in the
end, it worked fine. (I recall working with at least Marcel on that; one
historical reference I can find now is
<https://bugzilla.tianocore.org/show_bug.cgi?id=75>.)
I remember users confirming that native PCIe hotplug worked with
assigned physical devices even (e.g. GPUs), assuming they made use of
the resource reservation capability (e.g. they'd reserve large MMIO64
areas during initial enumeration).
- I seem to remember that we had tested hotplug on extra root bridges
(PXB) too; regressing that -- per the pxb-pcie mention in the blurb,
quoted below -- wouldn't be great. At least, please don't flip the big
switch so soon (IIUC, there is a big switch being proposed).
- The documentation at "docs/pcie.txt" and "docs/pcie_pci_bridge.txt" is
chock-full of hotplug references; we had spent days if not weeks for
writing and reviewing those. I hope it's being evaluated how much of
that is going to need an update.
In particular, do we know how this work is going to affect the resource
reservation capability?
$ qemu-system-x86_64 -device pcie-root-port,\? | grep reserve
bus-reserve=<uint32> - (default: 4294967295)
io-reserve=<size> - (default: 18446744073709551615)
mem-reserve=<size> - (default: 18446744073709551615)
pref32-reserve=<size> - (default: 18446744073709551615)
pref64-reserve=<size> - (default: 18446744073709551615)
The OVMF-side code (OvmfPkg/PciHotPlugInitDxe) was tough to write. As
far as I remember, especially commit fe4049471bdf
("OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation
hints", 2017-10-03) had taken a lot of navel-gazing. So the best answer
I'm looking for here is "this series does not affect resource
reservation at all".
- If my message is suggesting that I'm alarmed: that's an
understatement. This stuff is a mine-field. A good example is Gerd's
(correct!) response "Oh no, please don't" to Igor's question in the v1
thread, as to whether the piix4 IO port range could be reused:
http://mid.mail-archive.com/20200715065751.ogchtdqmnn7cxsyi@sirius.home.kraxel.org
That kind of "reuse" would have been a catastrophe, because for PCI IO
port aperture, OVMF uses [0xC000..0xFFFF] on i440fx, but
[0x6000..0xFFFF] on q35:
commit bba734ab4c7c9b4386d39420983bf61484f65dda
Author: Laszlo Ersek <lersek@redhat.com>
Date: Mon May 9 22:54:36 2016 +0200
OvmfPkg/PlatformPei: provide 10 * 4KB of PCI IO Port space on Q35
This can accommodate 10 bridges (including root bridges, PCIe upstream and
downstream ports, etc -- see
<https://bugzilla.redhat.com/show_bug.cgi?id=1333238#c12> for more
details).
10 is not a whole lot, but closer to the architectural limit of 15 than
our current 4, so it can be considered a stop-gap solution until all
guests manage to migrate to virtio-1.0, and no longer need PCI IO BARs
behind PCIe downstream ports.
Cc: Gabriel Somlo <somlo@cmu.edu>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1333238
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Gabriel Somlo <somlo@cmu.edu>
- If native PCIe hot-unplug is not working well (or at all) right now,
then I guess I can't just summarily say "we had better drop this like
hot potato".
But then, if we are committed to *juggling* that potato, we should at
least document the use case / motivation / current issues meticulously,
please. Do we have a public BZ at least?
- The other work, with regard to *disabling* unplug, which seems to be
progressing in parallel, is similarly in need of a good explanation, in
my opinion:
http://mid.mail-archive.com/20200820092157.17792-1-ani@anisinha.ca
Yes, I have read Laine's long email, linked from the QEMU cover letter:
https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
The whole use case "prevent guest admins from unplugging virtual
devices" still doesn't make any sense to me. How is "some cloud admins
don't want that" acceptable at face value, without further explanation?
Thanks
Laszlo
>
>
>> Switching to ACPI hot-plug for now.
>>
>> Tested on RHEL 8 and Windows 2019.
>> pxb-pcie is not yet supported.
>>
>> 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 (4):
>> hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
>> hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
>> hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC
>> in _OSC
>> hw/acpi/ich9: Enable ACPI PCI hot-plug
>>
>> hw/i386/acpi-build.h | 12 ++++++++++
>> include/hw/acpi/ich9.h | 3 +++
>> include/hw/acpi/pcihp.h | 3 ++-
>> hw/acpi/ich9.c | 52 ++++++++++++++++++++++++++++++++++++++++-
>> hw/acpi/pcihp.c | 15 ++++++++----
>> hw/acpi/piix4.c | 2 +-
>> hw/i386/acpi-build.c | 48 +++++++++++++++++++++++--------------
>> hw/i386/pc.c | 1 +
>> hw/acpi/trace-events | 4 ++++
>> 9 files changed, 114 insertions(+), 26 deletions(-)
>>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-22 14:25 ` Laszlo Ersek
@ 2020-08-24 11:35 ` Igor Mammedov
2020-08-24 11:51 ` Ani Sinha
0 siblings, 1 reply; 27+ messages in thread
From: Igor Mammedov @ 2020-08-24 11:35 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Daniel P. Berrange, Michael S. Tsirkin, Julia Suvorova,
qemu-devel, Gerd Hoffmann, Laine Stump, Ani Sinha
On Sat, 22 Aug 2020 16:25:55 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
> +Marcel, Laine, Daniel
>
> On 08/21/20 12:30, Igor Mammedov wrote:
> > On Tue, 18 Aug 2020 23:52:23 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >
> >> PCIe native hot-plug has numerous problems with racing events and
> >> unpredictable guest behaviour (Windows).
> > Documenting these misterious problems I've asked for in previous
> > review hasn't been addressed.
> > Pls see v1 for comments and add requested info into cover letter at
> > least or in a commit message.
>
> Igor, I assume you are referring to
>
> http://mid.mail-archive.com/20200715153321.3495e62d@redhat.com
>
> and I couldn't agree more.
>
> I'd like to understand the specific motivation for this patch series.
>
> - I'm very concerned that it could regress various hotplug scenarios
> with at least OVMF.
>
> So minimally I'm hoping that the work is being meticulously tested with
> OVMF.
>
> - I don't recall testing native PCIe hot-*unplug*, but we had repeatedly
> tested native PCIe plug with both Linux and Windows guests, and in the
> end, it worked fine. (I recall working with at least Marcel on that; one
> historical reference I can find now is
> <https://bugzilla.tianocore.org/show_bug.cgi?id=75>.)
>
> I remember users confirming that native PCIe hotplug worked with
> assigned physical devices even (e.g. GPUs), assuming they made use of
> the resource reservation capability (e.g. they'd reserve large MMIO64
> areas during initial enumeration).
>
> - I seem to remember that we had tested hotplug on extra root bridges
> (PXB) too; regressing that -- per the pxb-pcie mention in the blurb,
> quoted below -- wouldn't be great. At least, please don't flip the big
> switch so soon (IIUC, there is a big switch being proposed).
I'm suggesting to make ACPI hotplug on q35 opt-in,
becasue it's only Windows guests that doesn't work well with unplug.
Linux guests seems to be just fine with native hotplug.
> - The documentation at "docs/pcie.txt" and "docs/pcie_pci_bridge.txt" is
> chock-full of hotplug references; we had spent days if not weeks for
> writing and reviewing those. I hope it's being evaluated how much of
> that is going to need an update.
>
> In particular, do we know how this work is going to affect the resource
> reservation capability?
My hunch is that should not be affected (but I will not bet on it).
ACPI hotplug just changes route hotplug event is delivered, and unplug
happens via ACPI as well. That works around drivers offlining/onlining
devices in rapid succession during native unplug (that's quite crude
justification).
So I'd like reasons to be well documented, including what ideas were
tried to fix or workaround those issues (beside ACPI one), so the next
time we look at it we don't have to start from ground up.
> $ qemu-system-x86_64 -device pcie-root-port,\? | grep reserve
> bus-reserve=<uint32> - (default: 4294967295)
> io-reserve=<size> - (default: 18446744073709551615)
> mem-reserve=<size> - (default: 18446744073709551615)
> pref32-reserve=<size> - (default: 18446744073709551615)
> pref64-reserve=<size> - (default: 18446744073709551615)
>
> The OVMF-side code (OvmfPkg/PciHotPlugInitDxe) was tough to write. As
> far as I remember, especially commit fe4049471bdf
> ("OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation
> hints", 2017-10-03) had taken a lot of navel-gazing. So the best answer
> I'm looking for here is "this series does not affect resource
> reservation at all".
>
> - If my message is suggesting that I'm alarmed: that's an
> understatement. This stuff is a mine-field. A good example is Gerd's
> (correct!) response "Oh no, please don't" to Igor's question in the v1
> thread, as to whether the piix4 IO port range could be reused:
>
> http://mid.mail-archive.com/20200715065751.ogchtdqmnn7cxsyi@sirius.home.kraxel.org
>
> That kind of "reuse" would have been a catastrophe, because for PCI IO
> port aperture, OVMF uses [0xC000..0xFFFF] on i440fx, but
> [0x6000..0xFFFF] on q35:
>
> commit bba734ab4c7c9b4386d39420983bf61484f65dda
> Author: Laszlo Ersek <lersek@redhat.com>
> Date: Mon May 9 22:54:36 2016 +0200
>
> OvmfPkg/PlatformPei: provide 10 * 4KB of PCI IO Port space on Q35
>
> This can accommodate 10 bridges (including root bridges, PCIe upstream and
> downstream ports, etc -- see
> <https://bugzilla.redhat.com/show_bug.cgi?id=1333238#c12> for more
> details).
>
> 10 is not a whole lot, but closer to the architectural limit of 15 than
> our current 4, so it can be considered a stop-gap solution until all
> guests manage to migrate to virtio-1.0, and no longer need PCI IO BARs
> behind PCIe downstream ports.
>
> Cc: Gabriel Somlo <somlo@cmu.edu>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1333238
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> Tested-by: Gabriel Somlo <somlo@cmu.edu>
>
> - If native PCIe hot-unplug is not working well (or at all) right now,
> then I guess I can't just summarily say "we had better drop this like
> hot potato".
>
> But then, if we are committed to *juggling* that potato, we should at
> least document the use case / motivation / current issues meticulously,
> please. Do we have a public BZ at least?
>
> - The other work, with regard to *disabling* unplug, which seems to be
> progressing in parallel, is similarly in need of a good explanation, in
> my opinion:
>
> http://mid.mail-archive.com/20200820092157.17792-1-ani@anisinha.ca
>
> Yes, I have read Laine's long email, linked from the QEMU cover letter:
>
> https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
>
> The whole use case "prevent guest admins from unplugging virtual
> devices" still doesn't make any sense to me. How is "some cloud admins
> don't want that" acceptable at face value, without further explanation?
My take on it that, Windows always exposes unplug icon, and lets VM users
to unplug PCI devices. Notably, user is able to click away the only NIC
VM was configured with.
Unfortunatly the 'feature' can't be fixed on guest side, that's why
hypervisors implement such hack to disable ACPI hotplug. Which I guess
is backed by demand from users deploying Windows virtual desktops.
PS:
I'd made PCI hotplug opt-in, since not everyone needs it.
But that ship sailed long ago.
>
> Thanks
> Laszlo
>
> >
> >
> >> Switching to ACPI hot-plug for now.
> >>
> >> Tested on RHEL 8 and Windows 2019.
> >> pxb-pcie is not yet supported.
> >>
> >> 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 (4):
> >> hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
> >> hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
> >> hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC
> >> in _OSC
> >> hw/acpi/ich9: Enable ACPI PCI hot-plug
> >>
> >> hw/i386/acpi-build.h | 12 ++++++++++
> >> include/hw/acpi/ich9.h | 3 +++
> >> include/hw/acpi/pcihp.h | 3 ++-
> >> hw/acpi/ich9.c | 52 ++++++++++++++++++++++++++++++++++++++++-
> >> hw/acpi/pcihp.c | 15 ++++++++----
> >> hw/acpi/piix4.c | 2 +-
> >> hw/i386/acpi-build.c | 48 +++++++++++++++++++++++--------------
> >> hw/i386/pc.c | 1 +
> >> hw/acpi/trace-events | 4 ++++
> >> 9 files changed, 114 insertions(+), 26 deletions(-)
> >>
> >
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-24 11:35 ` Igor Mammedov
@ 2020-08-24 11:51 ` Ani Sinha
2020-08-24 15:03 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Ani Sinha @ 2020-08-24 11:51 UTC (permalink / raw)
To: Igor Mammedov
Cc: Daniel P. Berrange, Laine Stump, Michael S. Tsirkin,
Julia Suvorova, qemu-devel, Gerd Hoffmann, Laszlo Ersek
On Mon, Aug 24, 2020 at 5:06 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Sat, 22 Aug 2020 16:25:55 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
>
> > +Marcel, Laine, Daniel
> >
> > On 08/21/20 12:30, Igor Mammedov wrote:
> > > On Tue, 18 Aug 2020 23:52:23 +0200
> > > Julia Suvorova <jusual@redhat.com> wrote:
> > >
> > >> PCIe native hot-plug has numerous problems with racing events and
> > >> unpredictable guest behaviour (Windows).
> > > Documenting these misterious problems I've asked for in previous
> > > review hasn't been addressed.
> > > Pls see v1 for comments and add requested info into cover letter at
> > > least or in a commit message.
> >
> > Igor, I assume you are referring to
> >
> > http://mid.mail-archive.com/20200715153321.3495e62d@redhat.com
> >
> > and I couldn't agree more.
> >
> > I'd like to understand the specific motivation for this patch series.
> >
> > - I'm very concerned that it could regress various hotplug scenarios
> > with at least OVMF.
> >
> > So minimally I'm hoping that the work is being meticulously tested with
> > OVMF.
> >
> > - I don't recall testing native PCIe hot-*unplug*, but we had repeatedly
> > tested native PCIe plug with both Linux and Windows guests, and in the
> > end, it worked fine. (I recall working with at least Marcel on that; one
> > historical reference I can find now is
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=75>.)
> >
> > I remember users confirming that native PCIe hotplug worked with
> > assigned physical devices even (e.g. GPUs), assuming they made use of
> > the resource reservation capability (e.g. they'd reserve large MMIO64
> > areas during initial enumeration).
> >
> > - I seem to remember that we had tested hotplug on extra root bridges
> > (PXB) too; regressing that -- per the pxb-pcie mention in the blurb,
> > quoted below -- wouldn't be great. At least, please don't flip the big
> > switch so soon (IIUC, there is a big switch being proposed).
>
> I'm suggesting to make ACPI hotplug on q35 opt-in,
> becasue it's only Windows guests that doesn't work well with unplug.
> Linux guests seems to be just fine with native hotplug.
>
> > - The documentation at "docs/pcie.txt" and "docs/pcie_pci_bridge.txt" is
> > chock-full of hotplug references; we had spent days if not weeks for
> > writing and reviewing those. I hope it's being evaluated how much of
> > that is going to need an update.
> >
> > In particular, do we know how this work is going to affect the resource
> > reservation capability?
> My hunch is that should not be affected (but I will not bet on it).
> ACPI hotplug just changes route hotplug event is delivered, and unplug
> happens via ACPI as well. That works around drivers offlining/onlining
> devices in rapid succession during native unplug (that's quite crude
> justification).
>
> So I'd like reasons to be well documented, including what ideas were
> tried to fix or workaround those issues (beside ACPI one), so the next
> time we look at it we don't have to start from ground up.
>
>
> > $ qemu-system-x86_64 -device pcie-root-port,\? | grep reserve
> > bus-reserve=<uint32> - (default: 4294967295)
> > io-reserve=<size> - (default: 18446744073709551615)
> > mem-reserve=<size> - (default: 18446744073709551615)
> > pref32-reserve=<size> - (default: 18446744073709551615)
> > pref64-reserve=<size> - (default: 18446744073709551615)
> >
> > The OVMF-side code (OvmfPkg/PciHotPlugInitDxe) was tough to write. As
> > far as I remember, especially commit fe4049471bdf
> > ("OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation
> > hints", 2017-10-03) had taken a lot of navel-gazing. So the best answer
> > I'm looking for here is "this series does not affect resource
> > reservation at all".
> >
> > - If my message is suggesting that I'm alarmed: that's an
> > understatement. This stuff is a mine-field. A good example is Gerd's
> > (correct!) response "Oh no, please don't" to Igor's question in the v1
> > thread, as to whether the piix4 IO port range could be reused:
> >
> > http://mid.mail-archive.com/20200715065751.ogchtdqmnn7cxsyi@sirius.home.kraxel.org
> >
> > That kind of "reuse" would have been a catastrophe, because for PCI IO
> > port aperture, OVMF uses [0xC000..0xFFFF] on i440fx, but
> > [0x6000..0xFFFF] on q35:
> >
> > commit bba734ab4c7c9b4386d39420983bf61484f65dda
> > Author: Laszlo Ersek <lersek@redhat.com>
> > Date: Mon May 9 22:54:36 2016 +0200
> >
> > OvmfPkg/PlatformPei: provide 10 * 4KB of PCI IO Port space on Q35
> >
> > This can accommodate 10 bridges (including root bridges, PCIe upstream and
> > downstream ports, etc -- see
> > <https://bugzilla.redhat.com/show_bug.cgi?id=1333238#c12> for more
> > details).
> >
> > 10 is not a whole lot, but closer to the architectural limit of 15 than
> > our current 4, so it can be considered a stop-gap solution until all
> > guests manage to migrate to virtio-1.0, and no longer need PCI IO BARs
> > behind PCIe downstream ports.
> >
> > Cc: Gabriel Somlo <somlo@cmu.edu>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1333238
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> > Tested-by: Gabriel Somlo <somlo@cmu.edu>
> >
> > - If native PCIe hot-unplug is not working well (or at all) right now,
> > then I guess I can't just summarily say "we had better drop this like
> > hot potato".
> >
> > But then, if we are committed to *juggling* that potato, we should at
> > least document the use case / motivation / current issues meticulously,
> > please. Do we have a public BZ at least?
> >
> > - The other work, with regard to *disabling* unplug, which seems to be
> > progressing in parallel, is similarly in need of a good explanation, in
> > my opinion:
> >
> > http://mid.mail-archive.com/20200820092157.17792-1-ani@anisinha.ca
> >
> > Yes, I have read Laine's long email, linked from the QEMU cover letter:
> >
> > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> >
> > The whole use case "prevent guest admins from unplugging virtual
> > devices" still doesn't make any sense to me. How is "some cloud admins
> > don't want that" acceptable at face value, without further explanation?
> My take on it that, Windows always exposes unplug icon, and lets VM users
> to unplug PCI devices. Notably, user is able to click away the only NIC
> VM was configured with.
Correct. Also sometimes the admins may not want some other PCI devices
to be hot unpluggable such as the balloon device.
>
> Unfortunatly the 'feature' can't be fixed on guest side,
It can be using driver hacks but they are very operating system
specific and also needs to be applied per VM everytime they are
powered on.
that's why
> hypervisors implement such hack to disable ACPI hotplug. Which I guess
> is backed by demand from users deploying Windows virtual desktops.
>
> PS:
> I'd made PCI hotplug opt-in, since not everyone needs it.
> But that ship sailed long ago.
>
>
>
> >
> > Thanks
> > Laszlo
> >
> > >
> > >
> > >> Switching to ACPI hot-plug for now.
> > >>
> > >> Tested on RHEL 8 and Windows 2019.
> > >> pxb-pcie is not yet supported.
> > >>
> > >> 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 (4):
> > >> hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
> > >> hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
> > >> hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC
> > >> in _OSC
> > >> hw/acpi/ich9: Enable ACPI PCI hot-plug
> > >>
> > >> hw/i386/acpi-build.h | 12 ++++++++++
> > >> include/hw/acpi/ich9.h | 3 +++
> > >> include/hw/acpi/pcihp.h | 3 ++-
> > >> hw/acpi/ich9.c | 52 ++++++++++++++++++++++++++++++++++++++++-
> > >> hw/acpi/pcihp.c | 15 ++++++++----
> > >> hw/acpi/piix4.c | 2 +-
> > >> hw/i386/acpi-build.c | 48 +++++++++++++++++++++++--------------
> > >> hw/i386/pc.c | 1 +
> > >> hw/acpi/trace-events | 4 ++++
> > >> 9 files changed, 114 insertions(+), 26 deletions(-)
> > >>
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [RFC PATCH v2 0/4] Use ACPI PCI hot-plug for q35
2020-08-24 11:51 ` Ani Sinha
@ 2020-08-24 15:03 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-08-24 15:03 UTC (permalink / raw)
To: Ani Sinha, Igor Mammedov
Cc: Daniel P. Berrange, Michael S. Tsirkin, Julia Suvorova,
qemu-devel, Gerd Hoffmann, Laine Stump
On 08/24/20 13:51, Ani Sinha wrote:
> On Mon, Aug 24, 2020 at 5:06 PM Igor Mammedov <imammedo@redhat.com> wrote:
>>
>> On Sat, 22 Aug 2020 16:25:55 +0200
>> Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>> +Marcel, Laine, Daniel
>>>
>>> On 08/21/20 12:30, Igor Mammedov wrote:
>>>> On Tue, 18 Aug 2020 23:52:23 +0200
>>>> Julia Suvorova <jusual@redhat.com> wrote:
>>>>
>>>>> PCIe native hot-plug has numerous problems with racing events and
>>>>> unpredictable guest behaviour (Windows).
>>>> Documenting these misterious problems I've asked for in previous
>>>> review hasn't been addressed.
>>>> Pls see v1 for comments and add requested info into cover letter at
>>>> least or in a commit message.
>>>
>>> Igor, I assume you are referring to
>>>
>>> http://mid.mail-archive.com/20200715153321.3495e62d@redhat.com
>>>
>>> and I couldn't agree more.
>>>
>>> I'd like to understand the specific motivation for this patch series.
>>>
>>> - I'm very concerned that it could regress various hotplug scenarios
>>> with at least OVMF.
>>>
>>> So minimally I'm hoping that the work is being meticulously tested with
>>> OVMF.
>>>
>>> - I don't recall testing native PCIe hot-*unplug*, but we had repeatedly
>>> tested native PCIe plug with both Linux and Windows guests, and in the
>>> end, it worked fine. (I recall working with at least Marcel on that; one
>>> historical reference I can find now is
>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=75>.)
>>>
>>> I remember users confirming that native PCIe hotplug worked with
>>> assigned physical devices even (e.g. GPUs), assuming they made use of
>>> the resource reservation capability (e.g. they'd reserve large MMIO64
>>> areas during initial enumeration).
>>>
>>> - I seem to remember that we had tested hotplug on extra root bridges
>>> (PXB) too; regressing that -- per the pxb-pcie mention in the blurb,
>>> quoted below -- wouldn't be great. At least, please don't flip the big
>>> switch so soon (IIUC, there is a big switch being proposed).
>>
>> I'm suggesting to make ACPI hotplug on q35 opt-in,
>> becasue it's only Windows guests that doesn't work well with unplug.
>> Linux guests seems to be just fine with native hotplug.
>>
>>> - The documentation at "docs/pcie.txt" and "docs/pcie_pci_bridge.txt" is
>>> chock-full of hotplug references; we had spent days if not weeks for
>>> writing and reviewing those. I hope it's being evaluated how much of
>>> that is going to need an update.
>>>
>>> In particular, do we know how this work is going to affect the resource
>>> reservation capability?
>> My hunch is that should not be affected (but I will not bet on it).
>> ACPI hotplug just changes route hotplug event is delivered, and unplug
>> happens via ACPI as well. That works around drivers offlining/onlining
>> devices in rapid succession during native unplug (that's quite crude
>> justification).
>>
>> So I'd like reasons to be well documented, including what ideas were
>> tried to fix or workaround those issues (beside ACPI one), so the next
>> time we look at it we don't have to start from ground up.
>>
>>
>>> $ qemu-system-x86_64 -device pcie-root-port,\? | grep reserve
>>> bus-reserve=<uint32> - (default: 4294967295)
>>> io-reserve=<size> - (default: 18446744073709551615)
>>> mem-reserve=<size> - (default: 18446744073709551615)
>>> pref32-reserve=<size> - (default: 18446744073709551615)
>>> pref64-reserve=<size> - (default: 18446744073709551615)
>>>
>>> The OVMF-side code (OvmfPkg/PciHotPlugInitDxe) was tough to write. As
>>> far as I remember, especially commit fe4049471bdf
>>> ("OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation
>>> hints", 2017-10-03) had taken a lot of navel-gazing. So the best answer
>>> I'm looking for here is "this series does not affect resource
>>> reservation at all".
>>>
>>> - If my message is suggesting that I'm alarmed: that's an
>>> understatement. This stuff is a mine-field. A good example is Gerd's
>>> (correct!) response "Oh no, please don't" to Igor's question in the v1
>>> thread, as to whether the piix4 IO port range could be reused:
>>>
>>> http://mid.mail-archive.com/20200715065751.ogchtdqmnn7cxsyi@sirius.home.kraxel.org
>>>
>>> That kind of "reuse" would have been a catastrophe, because for PCI IO
>>> port aperture, OVMF uses [0xC000..0xFFFF] on i440fx, but
>>> [0x6000..0xFFFF] on q35:
>>>
>>> commit bba734ab4c7c9b4386d39420983bf61484f65dda
>>> Author: Laszlo Ersek <lersek@redhat.com>
>>> Date: Mon May 9 22:54:36 2016 +0200
>>>
>>> OvmfPkg/PlatformPei: provide 10 * 4KB of PCI IO Port space on Q35
>>>
>>> This can accommodate 10 bridges (including root bridges, PCIe upstream and
>>> downstream ports, etc -- see
>>> <https://bugzilla.redhat.com/show_bug.cgi?id=1333238#c12> for more
>>> details).
>>>
>>> 10 is not a whole lot, but closer to the architectural limit of 15 than
>>> our current 4, so it can be considered a stop-gap solution until all
>>> guests manage to migrate to virtio-1.0, and no longer need PCI IO BARs
>>> behind PCIe downstream ports.
>>>
>>> Cc: Gabriel Somlo <somlo@cmu.edu>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1333238
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>>> Tested-by: Gabriel Somlo <somlo@cmu.edu>
>>>
>>> - If native PCIe hot-unplug is not working well (or at all) right now,
>>> then I guess I can't just summarily say "we had better drop this like
>>> hot potato".
>>>
>>> But then, if we are committed to *juggling* that potato, we should at
>>> least document the use case / motivation / current issues meticulously,
>>> please. Do we have a public BZ at least?
>>>
>>> - The other work, with regard to *disabling* unplug, which seems to be
>>> progressing in parallel, is similarly in need of a good explanation, in
>>> my opinion:
>>>
>>> http://mid.mail-archive.com/20200820092157.17792-1-ani@anisinha.ca
>>>
>>> Yes, I have read Laine's long email, linked from the QEMU cover letter:
>>>
>>> https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
>>>
>>> The whole use case "prevent guest admins from unplugging virtual
>>> devices" still doesn't make any sense to me. How is "some cloud admins
>>> don't want that" acceptable at face value, without further explanation?
>> My take on it that, Windows always exposes unplug icon, and lets VM users
>> to unplug PCI devices. Notably, user is able to click away the only NIC
>> VM was configured with.
>
> Correct. Also sometimes the admins may not want some other PCI devices
> to be hot unpluggable such as the balloon device.
>
>>
>> Unfortunatly the 'feature' can't be fixed on guest side,
>
> It can be using driver hacks but they are very operating system
> specific and also needs to be applied per VM everytime they are
> powered on.
>
> that's why
>> hypervisors implement such hack to disable ACPI hotplug. Which I guess
>> is backed by demand from users deploying Windows virtual desktops.
>>
>> PS:
>> I'd made PCI hotplug opt-in, since not everyone needs it.
>> But that ship sailed long ago.
Thank you both for explaining.
All of these use cases seems justified to me.
Given that they are basically quirks, for addressing guest OS specific
peculiarities, changing machine type defaults does not seem warranted.
In my opinion, all of these bits should be opt-in. If we need to capture
permanent recommendations, we can always document them, and/or libosinfo
can expose them machine-readably.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread