* [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35
@ 2013-12-13 16:22 Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 01/11] acpi: piix4: remove not needed GPE0 mask Igor Mammedov
` (10 more replies)
0 siblings, 11 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
Tested with RHEL6 & WS2012R2
git tree for testing:
https://github.com/imammedo/qemu/commits/q35_cpu_hp_v1
Igor Mammedov (11):
acpi: piix4: remove not needed GPE0 mask
acpi: factor out common pm_update_sci() into acpi core
acpi: factor out common cpu hotplug code for PIIX4/Q35
acpi/piix4: add readonly "cpu-hotplug-io-base" property
acpi: ich9: allow guest to clear SCI rised by GPE
acpi/ich9: add CPU hotplug handling to Q35 machine
ACPI: Q35 DSDT: fix CPU hotplug GPE0.2 handler
ACPI/DSDT-CPU: cleanup bogus comment
ACPI: move PRST OperationRegion into SSDT
ACPI: set CPU hotplug io base dynamically
ACPI: update ssdt-misc.hex.generated acpi-dsdt.hex.generated
q35-acpi-dsdt.hex.generated
hw/acpi/Makefile.objs | 2 +-
hw/acpi/core.c | 18 +++
hw/acpi/hotplug.c | 65 +++++++++
hw/acpi/ich9.c | 49 ++++---
hw/acpi/piix4.c | 114 +++-------------
hw/i386/acpi-build.c | 6 +
hw/i386/acpi-dsdt-cpu-hotplug.dsl | 40 +-----
hw/i386/acpi-dsdt.dsl | 2 +-
hw/i386/acpi-dsdt.hex.generated | 152 ++-------------------
hw/i386/q35-acpi-dsdt.dsl | 6 +-
hw/i386/q35-acpi-dsdt.hex.generated | 168 +++--------------------
hw/i386/ssdt-misc.dsl | 66 +++++++++
hw/i386/ssdt-misc.hex.generated | 262 +++++++++++++++++++++++++++++++++++-
include/hw/acpi/acpi.h | 8 ++
include/hw/acpi/hotplug.h | 32 +++++
include/hw/acpi/ich9.h | 4 +
16 files changed, 545 insertions(+), 449 deletions(-)
create mode 100644 hw/acpi/hotplug.c
create mode 100644 include/hw/acpi/hotplug.h
--
1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 01/11] acpi: piix4: remove not needed GPE0 mask
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-19 14:16 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 02/11] acpi: factor out common pm_update_sci() into acpi core Igor Mammedov
` (9 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
Hardcoded GPE0 mask isn't really needed. Since GPE0_STS initialized
with all bits cleared and only QEMU itself can set bits there (i.e.
guest can only clear bits in it). So guest can't triger SCI
by setting _STS & _EN bits and there is not reason to mask out not
supported _STS bits since they shouldn't be set by QEMU in the first
place.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/piix4.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 93849c8..b4caeab 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -122,8 +122,7 @@ static void pm_update_sci(PIIX4PMState *s)
ACPI_BITMASK_POWER_BUTTON_ENABLE |
ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
- (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
- (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
+ ((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) != 0);
qemu_set_irq(s->irq, sci_level);
/* schedule a timer interruption if needed */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 02/11] acpi: factor out common pm_update_sci() into acpi core
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 01/11] acpi: piix4: remove not needed GPE0 mask Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-19 14:16 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35 Igor Mammedov
` (8 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
... and rename it into acpi_update_sci() since it changes
SCI on only on PM registers status.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
---
hw/acpi/core.c | 18 ++++++++++++++++++
hw/acpi/ich9.c | 23 ++---------------------
hw/acpi/piix4.c | 26 ++++----------------------
include/hw/acpi/acpi.h | 8 ++++++++
4 files changed, 32 insertions(+), 43 deletions(-)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 58308a3..79414b4 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -662,3 +662,21 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
return val;
}
+
+void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
+{
+ int sci_level, pm1a_sts;
+
+ pm1a_sts = acpi_pm1_evt_get_sts(regs);
+
+ sci_level = ((pm1a_sts &
+ regs->pm1.evt.en & ACPI_BITMASK_PM1_COMMON_ENABLED) != 0) ||
+ ((regs->gpe.sts[0] & regs->gpe.en[0]) != 0);
+
+ qemu_set_irq(irq, sci_level);
+
+ /* schedule a timer interruption if needed */
+ acpi_pm_tmr_update(regs,
+ (regs->pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
+ !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
+}
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 7e0429e..dcdef7c 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -44,29 +44,10 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
#define ICH9_DEBUG(fmt, ...) do { } while (0)
#endif
-static void pm_update_sci(ICH9LPCPMRegs *pm)
-{
- int sci_level, pm1a_sts;
-
- pm1a_sts = acpi_pm1_evt_get_sts(&pm->acpi_regs);
-
- sci_level = (((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
- (ACPI_BITMASK_RT_CLOCK_ENABLE |
- ACPI_BITMASK_POWER_BUTTON_ENABLE |
- ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
- ACPI_BITMASK_TIMER_ENABLE)) != 0);
- qemu_set_irq(pm->irq, sci_level);
-
- /* schedule a timer interruption if needed */
- acpi_pm_tmr_update(&pm->acpi_regs,
- (pm->acpi_regs.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
- !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
-}
-
static void ich9_pm_update_sci_fn(ACPIREGS *regs)
{
ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
- pm_update_sci(pm);
+ acpi_update_sci(&pm->acpi_regs, pm->irq);
}
static uint64_t ich9_gpe_readb(void *opaque, hwaddr addr, unsigned width)
@@ -193,7 +174,7 @@ static void pm_reset(void *opaque)
pm->smi_en |= ICH9_PMIO_SMI_EN_APMC_EN;
}
- pm_update_sci(pm);
+ acpi_update_sci(&pm->acpi_regs, pm->irq);
}
static void pm_powerdown_req(Notifier *n, void *opaque)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b4caeab..b6b97ce 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -112,28 +112,10 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
#define ACPI_ENABLE 0xf1
#define ACPI_DISABLE 0xf0
-static void pm_update_sci(PIIX4PMState *s)
-{
- int sci_level, pmsts;
-
- pmsts = acpi_pm1_evt_get_sts(&s->ar);
- sci_level = (((pmsts & s->ar.pm1.evt.en) &
- (ACPI_BITMASK_RT_CLOCK_ENABLE |
- ACPI_BITMASK_POWER_BUTTON_ENABLE |
- ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
- ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
- ((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) != 0);
-
- qemu_set_irq(s->irq, sci_level);
- /* schedule a timer interruption if needed */
- acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
- !(pmsts & ACPI_BITMASK_TIMER_STATUS));
-}
-
static void pm_tmr_timer(ACPIREGS *ar)
{
PIIX4PMState *s = container_of(ar, PIIX4PMState, ar);
- pm_update_sci(s);
+ acpi_update_sci(&s->ar, s->irq);
}
static void apm_ctrl_changed(uint32_t val, void *arg)
@@ -577,7 +559,7 @@ static void gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
PIIX4PMState *s = opaque;
acpi_gpe_ioport_writeb(&s->ar, addr, val);
- pm_update_sci(s);
+ acpi_update_sci(&s->ar, s->irq);
PIIX4_DPRINTF("gpe write %" HWADDR_PRIx " <== %" PRIu64 "\n", addr, val);
}
@@ -693,7 +675,7 @@ static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
} else {
g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
}
- pm_update_sci(s);
+ acpi_update_sci(&s->ar, s->irq);
}
static void piix4_cpu_added_req(Notifier *n, void *opaque)
@@ -767,7 +749,7 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
disable_device(s, slot);
}
- pm_update_sci(s);
+ acpi_update_sci(&s->ar, s->irq);
return 0;
}
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 6bbcb17..3e53297 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -69,6 +69,12 @@
#define ACPI_BITMASK_RT_CLOCK_ENABLE 0x0400
#define ACPI_BITMASK_PCIEXP_WAKE_DISABLE 0x4000 /* ACPI 3.0 */
+#define ACPI_BITMASK_PM1_COMMON_ENABLED ( \
+ ACPI_BITMASK_RT_CLOCK_ENABLE | \
+ ACPI_BITMASK_POWER_BUTTON_ENABLE | \
+ ACPI_BITMASK_GLOBAL_LOCK_ENABLE | \
+ ACPI_BITMASK_TIMER_ENABLE)
+
/* PM1x_CNT */
#define ACPI_BITMASK_SCI_ENABLE 0x0001
#define ACPI_BITMASK_BUS_MASTER_RLD 0x0002
@@ -160,6 +166,8 @@ void acpi_gpe_reset(ACPIREGS *ar);
void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
+void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
+
/* acpi.c */
extern int acpi_enabled;
extern char unsigned *acpi_tables;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 01/11] acpi: piix4: remove not needed GPE0 mask Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 02/11] acpi: factor out common pm_update_sci() into acpi core Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-19 14:14 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 04/11] acpi/piix4: add readonly "cpu-hotplug-io-base" property Igor Mammedov
` (7 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
.. so it could be used for adding CPU hotplug to Q35 machine
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/Makefile.objs | 2 +-
hw/acpi/hotplug.c | 65 +++++++++++++++++++++++++++++++++++++++
hw/acpi/piix4.c | 78 +++++------------------------------------------
include/hw/acpi/hotplug.h | 31 +++++++++++++++++++
4 files changed, 104 insertions(+), 72 deletions(-)
create mode 100644 hw/acpi/hotplug.c
create mode 100644 include/hw/acpi/hotplug.h
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index a0b63b5..41dec06 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -1,2 +1,2 @@
-common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o
+common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o hotplug.o
diff --git a/hw/acpi/hotplug.c b/hw/acpi/hotplug.c
new file mode 100644
index 0000000..fcc0988
--- /dev/null
+++ b/hw/acpi/hotplug.c
@@ -0,0 +1,65 @@
+/*
+ * QEMU ACPI hotplug utilities
+ *
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Authors:
+ * Igor Mammedov <imammedo@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "hw/hw.h"
+#include "hw/acpi/hotplug.h"
+
+static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
+{
+ AcpiCPUHotplug *cpus = opaque;
+ uint64_t val = cpus->sts[addr];
+
+ return val;
+}
+
+static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned int size)
+{
+ /* TODO: implement VCPU removal on guest signal that CPU can be removed */
+}
+
+static const MemoryRegionOps acpi_cpu_hotplug_ops = {
+ .read = cpu_status_read,
+ .write = cpu_status_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 1,
+ },
+};
+
+void acpi_hotplug_cpu_add(ACPIGPE *gpe, AcpiCPUHotplug *g, CPUState *cpu)
+{
+ CPUClass *k = CPU_GET_CLASS(cpu);
+ int64_t cpu_id;
+
+ *gpe->sts = *gpe->sts | ACPI_CPU_HOTPLUG_STATUS;
+ cpu_id = k->get_arch_id(CPU(cpu));
+ g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
+}
+
+void acpi_hotplug_cpu_init(MemoryRegion *parent, Object *owner,
+ AcpiCPUHotplug *gpe_cpu, uint16_t base)
+{
+ CPUState *cpu;
+
+ CPU_FOREACH(cpu) {
+ CPUClass *cc = CPU_GET_CLASS(cpu);
+ int64_t id = cc->get_arch_id(cpu);
+
+ g_assert((id / 8) < ACPI_GPE_PROC_LEN);
+ gpe_cpu->sts[id / 8] |= (1 << (id % 8));
+ }
+ gpe_cpu->io_base = base;
+ memory_region_init_io(&gpe_cpu->io, owner, &acpi_cpu_hotplug_ops,
+ gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
+ memory_region_add_subregion(parent, gpe_cpu->io_base, &gpe_cpu->io);
+}
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b6b97ce..8ab85b2 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -30,6 +30,7 @@
#include "hw/nvram/fw_cfg.h"
#include "exec/address-spaces.h"
#include "hw/acpi/piix4.h"
+#include "hw/acpi/hotplug.h"
//#define DEBUG
@@ -50,20 +51,14 @@
#define PCI_RMV_BASE 0xae0c
#define PIIX4_PROC_BASE 0xaf00
-#define PIIX4_PROC_LEN 32
#define PIIX4_PCI_HOTPLUG_STATUS 2
-#define PIIX4_CPU_HOTPLUG_STATUS 4
struct pci_status {
uint32_t up; /* deprecated, maintained for migration compatibility */
uint32_t down;
};
-typedef struct CPUStatus {
- uint8_t sts[PIIX4_PROC_LEN];
-} CPUStatus;
-
typedef struct PIIX4PMState {
/*< private >*/
PCIDevice parent_obj;
@@ -74,7 +69,6 @@ typedef struct PIIX4PMState {
MemoryRegion io_gpe;
MemoryRegion io_pci;
- MemoryRegion io_cpu;
ACPIREGS ar;
APMState apm;
@@ -97,7 +91,7 @@ typedef struct PIIX4PMState {
uint8_t disable_s4;
uint8_t s4_val;
- CPUStatus gpe_cpu;
+ AcpiCPUHotplug gpe_cpu;
Notifier cpu_added_notifier;
} PIIX4PMState;
@@ -628,61 +622,13 @@ static const MemoryRegionOps piix4_pci_ops = {
},
};
-static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
-{
- PIIX4PMState *s = opaque;
- CPUStatus *cpus = &s->gpe_cpu;
- uint64_t val = cpus->sts[addr];
-
- return val;
-}
-
-static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
- unsigned int size)
-{
- /* TODO: implement VCPU removal on guest signal that CPU can be removed */
-}
-
-static const MemoryRegionOps cpu_hotplug_ops = {
- .read = cpu_status_read,
- .write = cpu_status_write,
- .endianness = DEVICE_LITTLE_ENDIAN,
- .valid = {
- .min_access_size = 1,
- .max_access_size = 1,
- },
-};
-
-typedef enum {
- PLUG,
- UNPLUG,
-} HotplugEventType;
-
-static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
- HotplugEventType action)
-{
- CPUStatus *g = &s->gpe_cpu;
- ACPIGPE *gpe = &s->ar.gpe;
- CPUClass *k = CPU_GET_CLASS(cpu);
- int64_t cpu_id;
-
- assert(s != NULL);
-
- *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
- cpu_id = k->get_arch_id(CPU(cpu));
- if (action == PLUG) {
- g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
- } else {
- g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
- }
- acpi_update_sci(&s->ar, s->irq);
-}
-
static void piix4_cpu_added_req(Notifier *n, void *opaque)
{
PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
- piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
+ assert(s != NULL);
+ acpi_hotplug_cpu_add(&s->ar.gpe, &s->gpe_cpu, CPU(opaque));
+ acpi_update_sci(&s->ar, s->irq);
}
static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
@@ -691,8 +637,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
PCIBus *bus, PIIX4PMState *s)
{
- CPUState *cpu;
-
memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
"acpi-gpe0", GPE_LEN);
memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
@@ -703,16 +647,8 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
&s->io_pci);
pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
- CPU_FOREACH(cpu) {
- CPUClass *cc = CPU_GET_CLASS(cpu);
- int64_t id = cc->get_arch_id(cpu);
-
- g_assert((id / 8) < PIIX4_PROC_LEN);
- s->gpe_cpu.sts[id / 8] |= (1 << (id % 8));
- }
- memory_region_init_io(&s->io_cpu, OBJECT(s), &cpu_hotplug_ops, s,
- "acpi-cpu-hotplug", PIIX4_PROC_LEN);
- memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
+ acpi_hotplug_cpu_init(parent, OBJECT(s), &s->gpe_cpu,
+ PIIX4_PROC_BASE);
s->cpu_added_notifier.notify = piix4_cpu_added_req;
qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
}
diff --git a/include/hw/acpi/hotplug.h b/include/hw/acpi/hotplug.h
new file mode 100644
index 0000000..8bc176b
--- /dev/null
+++ b/include/hw/acpi/hotplug.h
@@ -0,0 +1,31 @@
+/*
+ * QEMU ACPI hotplug utilities
+ *
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Authors:
+ * Igor Mammedov <imammedo@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef ACPI_HOTPLUG_H
+#define ACPI_HOTPLUG_H
+
+#include "hw/acpi/acpi.h"
+
+#define ACPI_CPU_HOTPLUG_STATUS 4
+
+#define ACPI_GPE_PROC_LEN 32
+
+typedef struct AcpiCPUHotplug {
+ MemoryRegion io;
+ uint16_t io_base;
+ uint8_t sts[ACPI_GPE_PROC_LEN];
+} AcpiCPUHotplug;
+
+void acpi_hotplug_cpu_add(ACPIGPE *gpe, AcpiCPUHotplug *g, CPUState *cpu);
+
+void acpi_hotplug_cpu_init(MemoryRegion *parent, Object *owner,
+ AcpiCPUHotplug *gpe_cpu, uint16_t base);
+#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 04/11] acpi/piix4: add readonly "cpu-hotplug-io-base" property
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (2 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35 Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 05/11] acpi: ich9: allow guest to clear SCI rised by GPE Igor Mammedov
` (6 subsequent siblings)
10 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/piix4.c | 11 +++++++++++
include/hw/acpi/hotplug.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 8ab85b2..8db0920 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -31,6 +31,7 @@
#include "exec/address-spaces.h"
#include "hw/acpi/piix4.h"
#include "hw/acpi/hotplug.h"
+#include "qapi/visitor.h"
//#define DEBUG
@@ -384,6 +385,14 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
(memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
}
+static void piix4_get_cpu_io_base(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ PIIX4PMState *s = PIIX4_PM(obj);
+
+ visit_type_uint16(v, &s->gpe_cpu.io_base, name, errp);
+}
+
static void piix4_pm_add_propeties(PIIX4PMState *s)
{
static const uint8_t acpi_enable_cmd = ACPI_ENABLE;
@@ -404,6 +413,8 @@ static void piix4_pm_add_propeties(PIIX4PMState *s)
&sci_int, NULL);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
&s->io_base, NULL);
+ object_property_add(OBJECT(s), ACPI_CPU_HOTPLUG_IO_BASE_PROP, "int",
+ piix4_get_cpu_io_base, NULL, NULL, NULL, NULL);
}
static int piix4_pm_initfn(PCIDevice *dev)
diff --git a/include/hw/acpi/hotplug.h b/include/hw/acpi/hotplug.h
index 8bc176b..c1d82b5 100644
--- a/include/hw/acpi/hotplug.h
+++ b/include/hw/acpi/hotplug.h
@@ -14,6 +14,7 @@
#include "hw/acpi/acpi.h"
+#define ACPI_CPU_HOTPLUG_IO_BASE_PROP "cpu-hotplug-io-base"
#define ACPI_CPU_HOTPLUG_STATUS 4
#define ACPI_GPE_PROC_LEN 32
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 05/11] acpi: ich9: allow guest to clear SCI rised by GPE
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (3 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 04/11] acpi/piix4: add readonly "cpu-hotplug-io-base" property Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-19 14:16 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine Igor Mammedov
` (5 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
it fixes IRQ storm since guest isn't able to lower SCI IRQ
after it has been handled when it clears GPE event.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/ich9.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index dcdef7c..30f0df8 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -61,6 +61,7 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
{
ICH9LPCPMRegs *pm = opaque;
acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
+ acpi_update_sci(&pm->acpi_regs, pm->irq);
}
static const MemoryRegionOps ich9_gpe_ops = {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (4 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 05/11] acpi: ich9: allow guest to clear SCI rised by GPE Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-19 14:18 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 07/11] ACPI: Q35 DSDT: fix CPU hotplug GPE0.2 handler Igor Mammedov
` (4 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
.. including readonly 'cpu-hotplug-io-base' property
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/ich9.c | 27 +++++++++++++++++++++++++++
include/hw/acpi/ich9.h | 4 ++++
2 files changed, 31 insertions(+)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 30f0df8..9c88109 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -44,6 +44,8 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
#define ICH9_DEBUG(fmt, ...) do { } while (0)
#endif
+#define ICH9_PROC_BASE 0xa18
+
static void ich9_pm_update_sci_fn(ACPIREGS *regs)
{
ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
@@ -185,6 +187,15 @@ static void pm_powerdown_req(Notifier *n, void *opaque)
acpi_pm1_evt_power_down(&pm->acpi_regs);
}
+static void ich9_cpu_added_req(Notifier *n, void *opaque)
+{
+ ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
+
+ assert(pm != NULL);
+ acpi_hotplug_cpu_add(&pm->acpi_regs.gpe, &pm->gpe_cpu, CPU(opaque));
+ acpi_update_sci(&pm->acpi_regs, pm->irq);
+}
+
void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
qemu_irq sci_irq)
{
@@ -210,6 +221,11 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
qemu_register_reset(pm_reset, pm);
pm->powerdown_notifier.notify = pm_powerdown_req;
qemu_register_powerdown_notifier(&pm->powerdown_notifier);
+
+ acpi_hotplug_cpu_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
+ &pm->gpe_cpu, pm->gpe_cpu.io_base);
+ pm->cpu_added_notifier.notify = ich9_cpu_added_req;
+ qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
}
static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
@@ -222,6 +238,14 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
visit_type_uint32(v, &value, name, errp);
}
+static void ich9_pm_get_cpu_io_base(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+ visit_type_uint16(v, &s->pm.gpe_cpu.io_base, name, errp);
+}
+
void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
{
static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
@@ -233,4 +257,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
NULL, NULL, pm, NULL);
object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
&gpe0_len, errp);
+ pm->gpe_cpu.io_base = ICH9_PROC_BASE;
+ object_property_add(obj, ACPI_CPU_HOTPLUG_IO_BASE_PROP, "int",
+ ich9_pm_get_cpu_io_base, NULL, NULL, NULL, NULL);
}
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 82fcf9f..c71a9b4 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -22,6 +22,7 @@
#define HW_ACPI_ICH9_H
#include "hw/acpi/acpi.h"
+#include "hw/acpi/hotplug.h"
typedef struct ICH9LPCPMRegs {
/*
@@ -42,6 +43,9 @@ typedef struct ICH9LPCPMRegs {
uint32_t pm_io_base;
Notifier powerdown_notifier;
+
+ AcpiCPUHotplug gpe_cpu;
+ Notifier cpu_added_notifier;
} ICH9LPCPMRegs;
void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 07/11] ACPI: Q35 DSDT: fix CPU hotplug GPE0.2 handler
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (5 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 08/11] ACPI/DSDT-CPU: cleanup bogus comment Igor Mammedov
` (3 subsequent siblings)
10 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
Fix bogus CPU hotplug GPE handler.
Make Q35 CPU hotplug GPE handler match PIIX4 one, since
CPU hotplug event is triggered by GPE0.2 register.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/q35-acpi-dsdt.dsl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 21c89b0..22baa58 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -417,11 +417,11 @@ DefinitionBlock (
Method(_L00) {
}
Method(_L01) {
+ }
+ Method(_E02) {
// CPU hotplug event
\_SB.PRSC()
}
- Method(_L02) {
- }
Method(_L03) {
}
Method(_L04) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 08/11] ACPI/DSDT-CPU: cleanup bogus comment
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (6 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 07/11] ACPI: Q35 DSDT: fix CPU hotplug GPE0.2 handler Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT Igor Mammedov
` (2 subsequent siblings)
10 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/acpi-dsdt-cpu-hotplug.dsl | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
index c96ac42..995b415 100644
--- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
@@ -52,7 +52,6 @@ Scope(\_SB) {
Sleep(200)
}
- /* CPU hotplug notify method */
OperationRegion(PRST, SystemIO, 0xaf00, 32)
Field(PRST, ByteAcc, NoLock, Preserve) {
PRS, 256
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (7 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 08/11] ACPI/DSDT-CPU: cleanup bogus comment Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-16 19:30 ` Michael S. Tsirkin
2013-12-16 19:53 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 10/11] ACPI: set CPU hotplug io base dynamically Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 11/11] ACPI: update ssdt-misc.hex.generated acpi-dsdt.hex.generated q35-acpi-dsdt.hex.generated Igor Mammedov
10 siblings, 2 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
.. and report range used by it to OSPM via _CRS.
PRST is needed in SSDT since its base will depend on
chipset and will be dynamically set by QEMU.
Also move PRSC() method along with PRST since cross
table reference to PRST doesn't work.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
hw/i386/acpi-dsdt.dsl | 2 +-
hw/i386/q35-acpi-dsdt.dsl | 2 +-
hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
4 files changed, 68 insertions(+), 40 deletions(-)
diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
index 995b415..f26f81b 100644
--- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
@@ -20,6 +20,7 @@
Scope(\_SB) {
/* Objects filled in by run-time generated SSDT */
External(NTFY, MethodObj)
+ External(\_SB.CPHD.PRSC, MethodObj)
External(CPON, PkgObj)
/* Methods called by run-time generated SSDT Processor objects */
@@ -51,42 +52,4 @@ Scope(\_SB) {
// _EJ0 method - eject callback
Sleep(200)
}
-
- OperationRegion(PRST, SystemIO, 0xaf00, 32)
- Field(PRST, ByteAcc, NoLock, Preserve) {
- PRS, 256
- }
- Method(PRSC, 0) {
- // Local5 = active cpu bitmap
- Store(PRS, Local5)
- // Local2 = last read byte from bitmap
- Store(Zero, Local2)
- // Local0 = Processor ID / APIC ID iterator
- Store(Zero, Local0)
- While (LLess(Local0, SizeOf(CPON))) {
- // Local1 = CPON flag for this cpu
- Store(DerefOf(Index(CPON, Local0)), Local1)
- If (And(Local0, 0x07)) {
- // Shift down previously read bitmap byte
- ShiftRight(Local2, 1, Local2)
- } Else {
- // Read next byte from cpu bitmap
- Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
- }
- // Local3 = active state for this cpu
- Store(And(Local2, 1), Local3)
-
- If (LNotEqual(Local1, Local3)) {
- // State change - update CPON with new state
- Store(Local3, Index(CPON, Local0))
- // Do CPU notify
- If (LEqual(Local3, 1)) {
- NTFY(Local0, 1)
- } Else {
- NTFY(Local0, 3)
- }
- }
- Increment(Local0)
- }
- }
}
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index 90efce0..fa9f2d4 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -311,7 +311,7 @@ DefinitionBlock (
}
Method(_E02) {
// CPU hotplug event
- \_SB.PRSC()
+ \_SB.CPHD.PRSC()
}
Method(_L03) {
}
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 22baa58..9ccc543 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -420,7 +420,7 @@ DefinitionBlock (
}
Method(_E02) {
// CPU hotplug event
- \_SB.PRSC()
+ \_SB.CPHD.PRSC()
}
Method(_L03) {
}
diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
index a4484b8..ec8893c 100644
--- a/hw/i386/ssdt-misc.dsl
+++ b/hw/i386/ssdt-misc.dsl
@@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
}
}
}
+ Scope(\_SB) {
+ External(NTFY, MethodObj)
+ External(CPON, PkgObj)
+
+ Device(CPHD) {
+ Name(_HID, EISAID("PNP0C08"))
+ Name(CPPL, 32) // cpu-gpe length
+ Name(CPHP, 0xaf00)
+
+ OperationRegion(PRST, SystemIO, CPHP, CPPL)
+ Field(PRST, ByteAcc, NoLock, Preserve) {
+ PRS, 256
+ }
+
+ Method(PRSC, 0) {
+ // Local5 = active cpu bitmap
+ Store(PRS, Local5)
+ // Local2 = last read byte from bitmap
+ Store(Zero, Local2)
+ // Local0 = Processor ID / APIC ID iterator
+ Store(Zero, Local0)
+ While (LLess(Local0, SizeOf(CPON))) {
+ // Local1 = CPON flag for this cpu
+ Store(DerefOf(Index(CPON, Local0)), Local1)
+ If (And(Local0, 0x07)) {
+ // Shift down previously read bitmap byte
+ ShiftRight(Local2, 1, Local2)
+ } Else {
+ // Read next byte from cpu bitmap
+ Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
+ }
+ // Local3 = active state for this cpu
+ Store(And(Local2, 1), Local3)
+
+ If (LNotEqual(Local1, Local3)) {
+ // State change - update CPON with new state
+ Store(Local3, Index(CPON, Local0))
+ // Do CPU notify
+ If (LEqual(Local3, 1)) {
+ NTFY(Local0, 1)
+ } Else {
+ NTFY(Local0, 3)
+ }
+ }
+ Increment(Local0)
+ }
+ }
+
+ /* Leave bit 0 cleared to avoid Windows BSOD */
+ Name(_STA, 0xA)
+
+ Method(_CRS, 0) {
+ Store(ResourceTemplate() {
+ IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
+ }, Local0)
+
+ CreateWordField(Local0, IO._MIN, IOMN)
+ CreateWordField(Local0, IO._MAX, IOMX)
+
+ Store(CPHP, IOMN)
+ Subtract(Add(CPHP, CPPL), 1, IOMX)
+ Return(Local0)
+ }
+ } // Device(CPHD)
+ } // Scope(\_SB)
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 10/11] ACPI: set CPU hotplug io base dynamically
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (8 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 11/11] ACPI: update ssdt-misc.hex.generated acpi-dsdt.hex.generated q35-acpi-dsdt.hex.generated Igor Mammedov
10 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/acpi-build.c | 6 ++++++
hw/i386/ssdt-misc.dsl | 1 +
2 files changed, 7 insertions(+)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index befc39f..bfd34d7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -68,6 +68,7 @@ typedef struct AcpiPmInfo {
uint32_t gpe0_blk;
uint32_t gpe0_blk_len;
uint32_t io_base;
+ uint16_t io_cpu_base;
} AcpiPmInfo;
typedef struct AcpiMiscInfo {
@@ -169,6 +170,9 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
NULL);
pm->gpe0_blk_len = object_property_get_int(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
NULL);
+ pm->io_cpu_base = object_property_get_int(obj,
+ ACPI_CPU_HOTPLUG_IO_BASE_PROP,
+ NULL);
}
static void acpi_get_hotplug_info(AcpiMiscInfo *misc)
@@ -712,6 +716,8 @@ build_ssdt(GArray *table_data, GArray *linker,
*(uint16_t *)(ssdt_ptr + *ssdt_isa_pest) =
cpu_to_le16(misc->pvpanic_port);
+ *(uint16_t *)(ssdt_ptr + *ssdt_cpugpe_port) =
+ cpu_to_le16(pm->io_cpu_base);
{
GArray *sb_scope = build_alloc_array();
diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
index ec8893c..bb75b99 100644
--- a/hw/i386/ssdt-misc.dsl
+++ b/hw/i386/ssdt-misc.dsl
@@ -123,6 +123,7 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
Device(CPHD) {
Name(_HID, EISAID("PNP0C08"))
Name(CPPL, 32) // cpu-gpe length
+ ACPI_EXTRACT_NAME_WORD_CONST ssdt_cpugpe_port
Name(CPHP, 0xaf00)
OperationRegion(PRST, SystemIO, CPHP, CPPL)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH 11/11] ACPI: update ssdt-misc.hex.generated acpi-dsdt.hex.generated q35-acpi-dsdt.hex.generated
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
` (9 preceding siblings ...)
2013-12-13 16:22 ` [Qemu-devel] [PATCH 10/11] ACPI: set CPU hotplug io base dynamically Igor Mammedov
@ 2013-12-13 16:22 ` Igor Mammedov
10 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-13 16:22 UTC (permalink / raw)
To: qemu-devel
Cc: stefanha, mst, hutao, jjherne, brogers, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/acpi-dsdt.hex.generated | 152 ++-------------------
hw/i386/q35-acpi-dsdt.hex.generated | 168 +++--------------------
hw/i386/ssdt-misc.hex.generated | 262 +++++++++++++++++++++++++++++++++++-
3 files changed, 290 insertions(+), 292 deletions(-)
diff --git a/hw/i386/acpi-dsdt.hex.generated b/hw/i386/acpi-dsdt.hex.generated
index 2c01107..819d770 100644
--- a/hw/i386/acpi-dsdt.hex.generated
+++ b/hw/i386/acpi-dsdt.hex.generated
@@ -3,12 +3,12 @@ static unsigned char AcpiDsdtAmlCode[] = {
0x53,
0x44,
0x54,
-0x37,
-0x11,
+0xb7,
+0x10,
0x0,
0x0,
0x1,
-0xe0,
+0xfc,
0x42,
0x58,
0x50,
@@ -4016,8 +4016,8 @@ static unsigned char AcpiDsdtAmlCode[] = {
0x53,
0x1,
0x10,
-0x47,
-0xe,
+0x42,
+0x6,
0x5f,
0x53,
0x42,
@@ -4114,142 +4114,9 @@ static unsigned char AcpiDsdtAmlCode[] = {
0x22,
0xa,
0xc8,
-0x5b,
-0x80,
-0x50,
-0x52,
-0x53,
-0x54,
-0x1,
-0xb,
-0x0,
-0xaf,
-0xa,
-0x20,
-0x5b,
-0x81,
-0xc,
-0x50,
-0x52,
-0x53,
-0x54,
-0x1,
-0x50,
-0x52,
-0x53,
-0x5f,
-0x40,
0x10,
-0x14,
-0x4a,
-0x6,
-0x50,
-0x52,
-0x53,
-0x43,
-0x0,
-0x70,
-0x50,
-0x52,
-0x53,
-0x5f,
-0x65,
-0x70,
-0x0,
-0x62,
-0x70,
-0x0,
-0x60,
-0xa2,
-0x46,
-0x5,
-0x95,
-0x60,
-0x87,
-0x43,
-0x50,
-0x4f,
-0x4e,
-0x70,
-0x83,
-0x88,
-0x43,
-0x50,
-0x4f,
-0x4e,
-0x60,
-0x0,
-0x61,
-0xa0,
-0xa,
-0x7b,
-0x60,
-0xa,
-0x7,
-0x0,
-0x7a,
-0x62,
-0x1,
-0x62,
-0xa1,
-0xc,
-0x70,
-0x83,
-0x88,
-0x65,
-0x7a,
-0x60,
-0xa,
-0x3,
-0x0,
-0x0,
-0x62,
-0x70,
-0x7b,
-0x62,
-0x1,
-0x0,
-0x63,
-0xa0,
-0x22,
-0x92,
-0x93,
-0x61,
-0x63,
-0x70,
-0x63,
-0x88,
0x43,
-0x50,
-0x4f,
-0x4e,
-0x60,
-0x0,
-0xa0,
0xa,
-0x93,
-0x63,
-0x1,
-0x4e,
-0x54,
-0x46,
-0x59,
-0x60,
-0x1,
-0xa1,
-0x8,
-0x4e,
-0x54,
-0x46,
-0x59,
-0x60,
-0xa,
-0x3,
-0x75,
-0x60,
-0x10,
-0x4e,
-0x9,
0x5f,
0x47,
0x50,
@@ -4299,18 +4166,23 @@ static unsigned char AcpiDsdtAmlCode[] = {
0x4e,
0x46,
0x14,
-0x10,
+0x15,
0x5f,
0x45,
0x30,
0x32,
0x0,
0x5c,
-0x2e,
+0x2f,
+0x3,
0x5f,
0x53,
0x42,
0x5f,
+0x43,
+0x50,
+0x48,
+0x44,
0x50,
0x52,
0x53,
diff --git a/hw/i386/q35-acpi-dsdt.hex.generated b/hw/i386/q35-acpi-dsdt.hex.generated
index 32c16ff..ead88cd 100644
--- a/hw/i386/q35-acpi-dsdt.hex.generated
+++ b/hw/i386/q35-acpi-dsdt.hex.generated
@@ -3,12 +3,12 @@ static unsigned char Q35AcpiDsdtAmlCode[] = {
0x53,
0x44,
0x54,
-0xb0,
+0x30,
0x1c,
0x0,
0x0,
0x1,
-0x6,
+0x28,
0x42,
0x58,
0x50,
@@ -6968,8 +6968,8 @@ static unsigned char Q35AcpiDsdtAmlCode[] = {
0x53,
0x1,
0x10,
-0x47,
-0xe,
+0x42,
+0x6,
0x5f,
0x53,
0x42,
@@ -7066,142 +7066,9 @@ static unsigned char Q35AcpiDsdtAmlCode[] = {
0x22,
0xa,
0xc8,
-0x5b,
-0x80,
-0x50,
-0x52,
-0x53,
-0x54,
-0x1,
-0xb,
-0x0,
-0xaf,
-0xa,
-0x20,
-0x5b,
-0x81,
-0xc,
-0x50,
-0x52,
-0x53,
-0x54,
-0x1,
-0x50,
-0x52,
-0x53,
-0x5f,
-0x40,
0x10,
-0x14,
-0x4a,
-0x6,
-0x50,
-0x52,
-0x53,
-0x43,
-0x0,
-0x70,
-0x50,
-0x52,
-0x53,
-0x5f,
-0x65,
-0x70,
-0x0,
-0x62,
-0x70,
-0x0,
-0x60,
-0xa2,
-0x46,
-0x5,
-0x95,
-0x60,
-0x87,
-0x43,
-0x50,
-0x4f,
-0x4e,
-0x70,
-0x83,
-0x88,
-0x43,
-0x50,
-0x4f,
-0x4e,
-0x60,
-0x0,
-0x61,
-0xa0,
-0xa,
-0x7b,
-0x60,
-0xa,
-0x7,
-0x0,
-0x7a,
-0x62,
-0x1,
-0x62,
-0xa1,
-0xc,
-0x70,
-0x83,
-0x88,
-0x65,
-0x7a,
-0x60,
-0xa,
-0x3,
-0x0,
-0x0,
-0x62,
-0x70,
-0x7b,
-0x62,
-0x1,
-0x0,
-0x63,
-0xa0,
-0x22,
-0x92,
-0x93,
-0x61,
-0x63,
-0x70,
-0x63,
-0x88,
-0x43,
-0x50,
-0x4f,
-0x4e,
-0x60,
-0x0,
-0xa0,
-0xa,
-0x93,
-0x63,
-0x1,
-0x4e,
-0x54,
-0x46,
-0x59,
-0x60,
-0x1,
-0xa1,
-0x8,
-0x4e,
-0x54,
-0x46,
-0x59,
-0x60,
-0xa,
-0x3,
-0x75,
-0x60,
-0x10,
-0x4f,
-0x8,
+0x44,
+0x9,
0x5f,
0x47,
0x50,
@@ -7229,18 +7096,30 @@ static unsigned char Q35AcpiDsdtAmlCode[] = {
0x30,
0x0,
0x14,
-0x10,
+0x6,
0x5f,
0x4c,
0x30,
0x31,
0x0,
+0x14,
+0x15,
+0x5f,
+0x45,
+0x30,
+0x32,
+0x0,
0x5c,
-0x2e,
+0x2f,
+0x3,
0x5f,
0x53,
0x42,
0x5f,
+0x43,
+0x50,
+0x48,
+0x44,
0x50,
0x52,
0x53,
@@ -7250,13 +7129,6 @@ static unsigned char Q35AcpiDsdtAmlCode[] = {
0x5f,
0x4c,
0x30,
-0x32,
-0x0,
-0x14,
-0x6,
-0x5f,
-0x4c,
-0x30,
0x33,
0x0,
0x14,
diff --git a/hw/i386/ssdt-misc.hex.generated b/hw/i386/ssdt-misc.hex.generated
index 55e3bd2..c2c1431 100644
--- a/hw/i386/ssdt-misc.hex.generated
+++ b/hw/i386/ssdt-misc.hex.generated
@@ -4,6 +4,9 @@ static unsigned char acpi_pci64_length[] = {
static unsigned char acpi_s4_pkg[] = {
0x8f
};
+static unsigned short ssdt_cpugpe_port[] = {
+0x189
+};
static unsigned char acpi_s3_name[] = {
0x7c
};
@@ -18,12 +21,12 @@ static unsigned char ssdp_misc_aml[] = {
0x53,
0x44,
0x54,
-0x62,
-0x1,
+0x5d,
+0x2,
0x0,
0x0,
0x1,
-0x76,
+0xac,
0x42,
0x58,
0x50,
@@ -367,7 +370,258 @@ static unsigned char ssdp_misc_aml[] = {
0x49,
0x4f,
0x4d,
-0x58
+0x58,
+0x10,
+0x4a,
+0xf,
+0x5c,
+0x5f,
+0x53,
+0x42,
+0x5f,
+0x5b,
+0x82,
+0x41,
+0xf,
+0x43,
+0x50,
+0x48,
+0x44,
+0x8,
+0x5f,
+0x48,
+0x49,
+0x44,
+0xc,
+0x41,
+0xd0,
+0xc,
+0x8,
+0x8,
+0x43,
+0x50,
+0x50,
+0x4c,
+0xa,
+0x20,
+0x8,
+0x43,
+0x50,
+0x48,
+0x50,
+0xb,
+0x0,
+0xaf,
+0x5b,
+0x80,
+0x50,
+0x52,
+0x53,
+0x54,
+0x1,
+0x43,
+0x50,
+0x48,
+0x50,
+0x43,
+0x50,
+0x50,
+0x4c,
+0x5b,
+0x81,
+0xc,
+0x50,
+0x52,
+0x53,
+0x54,
+0x1,
+0x50,
+0x52,
+0x53,
+0x5f,
+0x40,
+0x10,
+0x14,
+0x4a,
+0x6,
+0x50,
+0x52,
+0x53,
+0x43,
+0x0,
+0x70,
+0x50,
+0x52,
+0x53,
+0x5f,
+0x65,
+0x70,
+0x0,
+0x62,
+0x70,
+0x0,
+0x60,
+0xa2,
+0x46,
+0x5,
+0x95,
+0x60,
+0x87,
+0x43,
+0x50,
+0x4f,
+0x4e,
+0x70,
+0x83,
+0x88,
+0x43,
+0x50,
+0x4f,
+0x4e,
+0x60,
+0x0,
+0x61,
+0xa0,
+0xa,
+0x7b,
+0x60,
+0xa,
+0x7,
+0x0,
+0x7a,
+0x62,
+0x1,
+0x62,
+0xa1,
+0xc,
+0x70,
+0x83,
+0x88,
+0x65,
+0x7a,
+0x60,
+0xa,
+0x3,
+0x0,
+0x0,
+0x62,
+0x70,
+0x7b,
+0x62,
+0x1,
+0x0,
+0x63,
+0xa0,
+0x22,
+0x92,
+0x93,
+0x61,
+0x63,
+0x70,
+0x63,
+0x88,
+0x43,
+0x50,
+0x4f,
+0x4e,
+0x60,
+0x0,
+0xa0,
+0xa,
+0x93,
+0x63,
+0x1,
+0x4e,
+0x54,
+0x46,
+0x59,
+0x60,
+0x1,
+0xa1,
+0x8,
+0x4e,
+0x54,
+0x46,
+0x59,
+0x60,
+0xa,
+0x3,
+0x75,
+0x60,
+0x8,
+0x5f,
+0x53,
+0x54,
+0x41,
+0xa,
+0xa,
+0x14,
+0x42,
+0x4,
+0x5f,
+0x43,
+0x52,
+0x53,
+0x0,
+0x70,
+0x11,
+0xd,
+0xa,
+0xa,
+0x47,
+0x1,
+0x0,
+0x0,
+0x0,
+0x0,
+0x1,
+0x15,
+0x79,
+0x0,
+0x60,
+0x8b,
+0x60,
+0xa,
+0x2,
+0x49,
+0x4f,
+0x4d,
+0x4e,
+0x8b,
+0x60,
+0xa,
+0x4,
+0x49,
+0x4f,
+0x4d,
+0x58,
+0x70,
+0x43,
+0x50,
+0x48,
+0x50,
+0x49,
+0x4f,
+0x4d,
+0x4e,
+0x74,
+0x72,
+0x43,
+0x50,
+0x48,
+0x50,
+0x43,
+0x50,
+0x50,
+0x4c,
+0x0,
+0x1,
+0x49,
+0x4f,
+0x4d,
+0x58,
+0xa4,
+0x60
};
static unsigned char ssdt_isa_pest[] = {
0xd0
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-13 16:22 ` [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT Igor Mammedov
@ 2013-12-16 19:30 ` Michael S. Tsirkin
2013-12-16 20:38 ` Igor Mammedov
2013-12-16 19:53 ` Michael S. Tsirkin
1 sibling, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-16 19:30 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> .. and report range used by it to OSPM via _CRS.
> PRST is needed in SSDT since its base will depend on
> chipset and will be dynamically set by QEMU.
> Also move PRSC() method along with PRST since cross
> table reference to PRST doesn't work.
Could you clarify this last sentence?
I don't mind where it is but I'd like to know
where does the limitation come from.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> hw/i386/acpi-dsdt.dsl | 2 +-
> hw/i386/q35-acpi-dsdt.dsl | 2 +-
> hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 68 insertions(+), 40 deletions(-)
>
> diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> index 995b415..f26f81b 100644
> --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> @@ -20,6 +20,7 @@
> Scope(\_SB) {
> /* Objects filled in by run-time generated SSDT */
> External(NTFY, MethodObj)
> + External(\_SB.CPHD.PRSC, MethodObj)
> External(CPON, PkgObj)
>
> /* Methods called by run-time generated SSDT Processor objects */
> @@ -51,42 +52,4 @@ Scope(\_SB) {
> // _EJ0 method - eject callback
> Sleep(200)
> }
> -
> - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> - Field(PRST, ByteAcc, NoLock, Preserve) {
> - PRS, 256
> - }
> - Method(PRSC, 0) {
> - // Local5 = active cpu bitmap
> - Store(PRS, Local5)
> - // Local2 = last read byte from bitmap
> - Store(Zero, Local2)
> - // Local0 = Processor ID / APIC ID iterator
> - Store(Zero, Local0)
> - While (LLess(Local0, SizeOf(CPON))) {
> - // Local1 = CPON flag for this cpu
> - Store(DerefOf(Index(CPON, Local0)), Local1)
> - If (And(Local0, 0x07)) {
> - // Shift down previously read bitmap byte
> - ShiftRight(Local2, 1, Local2)
> - } Else {
> - // Read next byte from cpu bitmap
> - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> - }
> - // Local3 = active state for this cpu
> - Store(And(Local2, 1), Local3)
> -
> - If (LNotEqual(Local1, Local3)) {
> - // State change - update CPON with new state
> - Store(Local3, Index(CPON, Local0))
> - // Do CPU notify
> - If (LEqual(Local3, 1)) {
> - NTFY(Local0, 1)
> - } Else {
> - NTFY(Local0, 3)
> - }
> - }
> - Increment(Local0)
> - }
> - }
> }
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index 90efce0..fa9f2d4 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -311,7 +311,7 @@ DefinitionBlock (
> }
> Method(_E02) {
> // CPU hotplug event
> - \_SB.PRSC()
> + \_SB.CPHD.PRSC()
> }
> Method(_L03) {
> }
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 22baa58..9ccc543 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -420,7 +420,7 @@ DefinitionBlock (
> }
> Method(_E02) {
> // CPU hotplug event
> - \_SB.PRSC()
> + \_SB.CPHD.PRSC()
> }
> Method(_L03) {
> }
> diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> index a4484b8..ec8893c 100644
> --- a/hw/i386/ssdt-misc.dsl
> +++ b/hw/i386/ssdt-misc.dsl
> @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> }
> }
> }
> + Scope(\_SB) {
> + External(NTFY, MethodObj)
> + External(CPON, PkgObj)
> +
> + Device(CPHD) {
> + Name(_HID, EISAID("PNP0C08"))
> + Name(CPPL, 32) // cpu-gpe length
> + Name(CPHP, 0xaf00)
> +
> + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> + Field(PRST, ByteAcc, NoLock, Preserve) {
> + PRS, 256
> + }
> +
> + Method(PRSC, 0) {
> + // Local5 = active cpu bitmap
> + Store(PRS, Local5)
> + // Local2 = last read byte from bitmap
> + Store(Zero, Local2)
> + // Local0 = Processor ID / APIC ID iterator
> + Store(Zero, Local0)
> + While (LLess(Local0, SizeOf(CPON))) {
> + // Local1 = CPON flag for this cpu
> + Store(DerefOf(Index(CPON, Local0)), Local1)
> + If (And(Local0, 0x07)) {
> + // Shift down previously read bitmap byte
> + ShiftRight(Local2, 1, Local2)
> + } Else {
> + // Read next byte from cpu bitmap
> + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> + }
> + // Local3 = active state for this cpu
> + Store(And(Local2, 1), Local3)
> +
> + If (LNotEqual(Local1, Local3)) {
> + // State change - update CPON with new state
> + Store(Local3, Index(CPON, Local0))
> + // Do CPU notify
> + If (LEqual(Local3, 1)) {
> + NTFY(Local0, 1)
> + } Else {
> + NTFY(Local0, 3)
> + }
> + }
> + Increment(Local0)
> + }
> + }
> +
> + /* Leave bit 0 cleared to avoid Windows BSOD */
> + Name(_STA, 0xA)
> +
> + Method(_CRS, 0) {
> + Store(ResourceTemplate() {
> + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> + }, Local0)
> +
> + CreateWordField(Local0, IO._MIN, IOMN)
> + CreateWordField(Local0, IO._MAX, IOMX)
> +
> + Store(CPHP, IOMN)
> + Subtract(Add(CPHP, CPPL), 1, IOMX)
> + Return(Local0)
> + }
> + } // Device(CPHD)
> + } // Scope(\_SB)
> }
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-13 16:22 ` [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT Igor Mammedov
2013-12-16 19:30 ` Michael S. Tsirkin
@ 2013-12-16 19:53 ` Michael S. Tsirkin
2013-12-16 22:15 ` Igor Mammedov
2013-12-22 14:51 ` Igor Mammedov
1 sibling, 2 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-16 19:53 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> .. and report range used by it to OSPM via _CRS.
> PRST is needed in SSDT since its base will depend on
> chipset and will be dynamically set by QEMU.
> Also move PRSC() method along with PRST since cross
> table reference to PRST doesn't work.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> hw/i386/acpi-dsdt.dsl | 2 +-
> hw/i386/q35-acpi-dsdt.dsl | 2 +-
> hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 68 insertions(+), 40 deletions(-)
>
> diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> index 995b415..f26f81b 100644
> --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> @@ -20,6 +20,7 @@
> Scope(\_SB) {
> /* Objects filled in by run-time generated SSDT */
> External(NTFY, MethodObj)
> + External(\_SB.CPHD.PRSC, MethodObj)
> External(CPON, PkgObj)
>
> /* Methods called by run-time generated SSDT Processor objects */
> @@ -51,42 +52,4 @@ Scope(\_SB) {
> // _EJ0 method - eject callback
> Sleep(200)
> }
> -
> - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> - Field(PRST, ByteAcc, NoLock, Preserve) {
> - PRS, 256
> - }
> - Method(PRSC, 0) {
> - // Local5 = active cpu bitmap
> - Store(PRS, Local5)
> - // Local2 = last read byte from bitmap
> - Store(Zero, Local2)
> - // Local0 = Processor ID / APIC ID iterator
> - Store(Zero, Local0)
> - While (LLess(Local0, SizeOf(CPON))) {
> - // Local1 = CPON flag for this cpu
> - Store(DerefOf(Index(CPON, Local0)), Local1)
> - If (And(Local0, 0x07)) {
> - // Shift down previously read bitmap byte
> - ShiftRight(Local2, 1, Local2)
> - } Else {
> - // Read next byte from cpu bitmap
> - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> - }
> - // Local3 = active state for this cpu
> - Store(And(Local2, 1), Local3)
> -
> - If (LNotEqual(Local1, Local3)) {
> - // State change - update CPON with new state
> - Store(Local3, Index(CPON, Local0))
> - // Do CPU notify
> - If (LEqual(Local3, 1)) {
> - NTFY(Local0, 1)
> - } Else {
> - NTFY(Local0, 3)
> - }
> - }
> - Increment(Local0)
> - }
> - }
> }
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index 90efce0..fa9f2d4 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -311,7 +311,7 @@ DefinitionBlock (
> }
> Method(_E02) {
> // CPU hotplug event
> - \_SB.PRSC()
> + \_SB.CPHD.PRSC()
> }
> Method(_L03) {
> }
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 22baa58..9ccc543 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -420,7 +420,7 @@ DefinitionBlock (
> }
> Method(_E02) {
> // CPU hotplug event
> - \_SB.PRSC()
> + \_SB.CPHD.PRSC()
> }
> Method(_L03) {
> }
> diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> index a4484b8..ec8893c 100644
> --- a/hw/i386/ssdt-misc.dsl
> +++ b/hw/i386/ssdt-misc.dsl
> @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> }
> }
> }
> + Scope(\_SB) {
> + External(NTFY, MethodObj)
> + External(CPON, PkgObj)
> +
> + Device(CPHD) {
> + Name(_HID, EISAID("PNP0C08"))
> + Name(CPPL, 32) // cpu-gpe length
> + Name(CPHP, 0xaf00)
> +
> + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> + Field(PRST, ByteAcc, NoLock, Preserve) {
> + PRS, 256
> + }
> +
> + Method(PRSC, 0) {
> + // Local5 = active cpu bitmap
> + Store(PRS, Local5)
> + // Local2 = last read byte from bitmap
> + Store(Zero, Local2)
> + // Local0 = Processor ID / APIC ID iterator
> + Store(Zero, Local0)
> + While (LLess(Local0, SizeOf(CPON))) {
> + // Local1 = CPON flag for this cpu
> + Store(DerefOf(Index(CPON, Local0)), Local1)
> + If (And(Local0, 0x07)) {
> + // Shift down previously read bitmap byte
> + ShiftRight(Local2, 1, Local2)
> + } Else {
> + // Read next byte from cpu bitmap
> + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> + }
> + // Local3 = active state for this cpu
> + Store(And(Local2, 1), Local3)
> +
> + If (LNotEqual(Local1, Local3)) {
> + // State change - update CPON with new state
> + Store(Local3, Index(CPON, Local0))
> + // Do CPU notify
> + If (LEqual(Local3, 1)) {
> + NTFY(Local0, 1)
> + } Else {
> + NTFY(Local0, 3)
> + }
> + }
> + Increment(Local0)
> + }
> + }
> +
> + /* Leave bit 0 cleared to avoid Windows BSOD */
> + Name(_STA, 0xA)
This shared the problem you yourself pointed out with
patch 'pc: ACPI BIOS: implement memory hotplug':
if we make device non present ospm can ignore our _CRS.
Can't we get a better handle on why windows crashes?
> +
> + Method(_CRS, 0) {
> + Store(ResourceTemplate() {
> + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> + }, Local0)
> +
> + CreateWordField(Local0, IO._MIN, IOMN)
> + CreateWordField(Local0, IO._MAX, IOMX)
> +
> + Store(CPHP, IOMN)
> + Subtract(Add(CPHP, CPPL), 1, IOMX)
> + Return(Local0)
> + }
> + } // Device(CPHD)
> + } // Scope(\_SB)
> }
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 19:30 ` Michael S. Tsirkin
@ 2013-12-16 20:38 ` Igor Mammedov
2013-12-16 21:13 ` Laszlo Ersek
2013-12-16 21:44 ` Laszlo Ersek
0 siblings, 2 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-16 20:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Mon, 16 Dec 2013 21:30:14 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > .. and report range used by it to OSPM via _CRS.
> > PRST is needed in SSDT since its base will depend on
> > chipset and will be dynamically set by QEMU.
> > Also move PRSC() method along with PRST since cross
> > table reference to PRST doesn't work.
>
> Could you clarify this last sentence?
> I don't mind where it is but I'd like to know
> where does the limitation come from.
It's empiric deduction so far I haven't found such limitation in spec yet.
iasl builds tables just fine but neither linux nor windows were able to find
Operation region from SSDT when loading DSDT, failing whole table loading
process. Decompiling DSDT/SSDT tables in guest shows that region is in
expected scope but OSPM refuses to see it when referenced outside SSDT.
Maybe there is some AML magic to make it work, I'm not aware of.
The same thing I had to do for memory hotplug as well. So I've tried to play
nicely 2 times and I have ended up with this solution both times.
>
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > hw/i386/acpi-dsdt.dsl | 2 +-
> > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 68 insertions(+), 40 deletions(-)
> >
> > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > index 995b415..f26f81b 100644
> > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > @@ -20,6 +20,7 @@
> > Scope(\_SB) {
> > /* Objects filled in by run-time generated SSDT */
> > External(NTFY, MethodObj)
> > + External(\_SB.CPHD.PRSC, MethodObj)
> > External(CPON, PkgObj)
> >
> > /* Methods called by run-time generated SSDT Processor objects */
> > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > // _EJ0 method - eject callback
> > Sleep(200)
> > }
> > -
> > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > - PRS, 256
> > - }
> > - Method(PRSC, 0) {
> > - // Local5 = active cpu bitmap
> > - Store(PRS, Local5)
> > - // Local2 = last read byte from bitmap
> > - Store(Zero, Local2)
> > - // Local0 = Processor ID / APIC ID iterator
> > - Store(Zero, Local0)
> > - While (LLess(Local0, SizeOf(CPON))) {
> > - // Local1 = CPON flag for this cpu
> > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > - If (And(Local0, 0x07)) {
> > - // Shift down previously read bitmap byte
> > - ShiftRight(Local2, 1, Local2)
> > - } Else {
> > - // Read next byte from cpu bitmap
> > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > - }
> > - // Local3 = active state for this cpu
> > - Store(And(Local2, 1), Local3)
> > -
> > - If (LNotEqual(Local1, Local3)) {
> > - // State change - update CPON with new state
> > - Store(Local3, Index(CPON, Local0))
> > - // Do CPU notify
> > - If (LEqual(Local3, 1)) {
> > - NTFY(Local0, 1)
> > - } Else {
> > - NTFY(Local0, 3)
> > - }
> > - }
> > - Increment(Local0)
> > - }
> > - }
> > }
> > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > index 90efce0..fa9f2d4 100644
> > --- a/hw/i386/acpi-dsdt.dsl
> > +++ b/hw/i386/acpi-dsdt.dsl
> > @@ -311,7 +311,7 @@ DefinitionBlock (
> > }
> > Method(_E02) {
> > // CPU hotplug event
> > - \_SB.PRSC()
> > + \_SB.CPHD.PRSC()
> > }
> > Method(_L03) {
> > }
> > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > index 22baa58..9ccc543 100644
> > --- a/hw/i386/q35-acpi-dsdt.dsl
> > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > @@ -420,7 +420,7 @@ DefinitionBlock (
> > }
> > Method(_E02) {
> > // CPU hotplug event
> > - \_SB.PRSC()
> > + \_SB.CPHD.PRSC()
> > }
> > Method(_L03) {
> > }
> > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > index a4484b8..ec8893c 100644
> > --- a/hw/i386/ssdt-misc.dsl
> > +++ b/hw/i386/ssdt-misc.dsl
> > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > }
> > }
> > }
> > + Scope(\_SB) {
> > + External(NTFY, MethodObj)
> > + External(CPON, PkgObj)
> > +
> > + Device(CPHD) {
> > + Name(_HID, EISAID("PNP0C08"))
> > + Name(CPPL, 32) // cpu-gpe length
> > + Name(CPHP, 0xaf00)
> > +
> > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > + PRS, 256
> > + }
> > +
> > + Method(PRSC, 0) {
> > + // Local5 = active cpu bitmap
> > + Store(PRS, Local5)
> > + // Local2 = last read byte from bitmap
> > + Store(Zero, Local2)
> > + // Local0 = Processor ID / APIC ID iterator
> > + Store(Zero, Local0)
> > + While (LLess(Local0, SizeOf(CPON))) {
> > + // Local1 = CPON flag for this cpu
> > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > + If (And(Local0, 0x07)) {
> > + // Shift down previously read bitmap byte
> > + ShiftRight(Local2, 1, Local2)
> > + } Else {
> > + // Read next byte from cpu bitmap
> > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > + }
> > + // Local3 = active state for this cpu
> > + Store(And(Local2, 1), Local3)
> > +
> > + If (LNotEqual(Local1, Local3)) {
> > + // State change - update CPON with new state
> > + Store(Local3, Index(CPON, Local0))
> > + // Do CPU notify
> > + If (LEqual(Local3, 1)) {
> > + NTFY(Local0, 1)
> > + } Else {
> > + NTFY(Local0, 3)
> > + }
> > + }
> > + Increment(Local0)
> > + }
> > + }
> > +
> > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > + Name(_STA, 0xA)
> > +
> > + Method(_CRS, 0) {
> > + Store(ResourceTemplate() {
> > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > + }, Local0)
> > +
> > + CreateWordField(Local0, IO._MIN, IOMN)
> > + CreateWordField(Local0, IO._MAX, IOMX)
> > +
> > + Store(CPHP, IOMN)
> > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > + Return(Local0)
> > + }
> > + } // Device(CPHD)
> > + } // Scope(\_SB)
> > }
> > --
> > 1.8.3.1
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 20:38 ` Igor Mammedov
@ 2013-12-16 21:13 ` Laszlo Ersek
2013-12-16 21:22 ` Laszlo Ersek
2013-12-16 21:53 ` Igor Mammedov
2013-12-16 21:44 ` Laszlo Ersek
1 sibling, 2 replies; 40+ messages in thread
From: Laszlo Ersek @ 2013-12-16 21:13 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, Michael S. Tsirkin, hutao, qemu-devel, brogers, kraxel,
aliguori, kaneshige.kenji, chen.fan.fnst, pbonzini, jjherne
On 12/16/13 21:38, Igor Mammedov wrote:
> On Mon, 16 Dec 2013 21:30:14 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
>>> .. and report range used by it to OSPM via _CRS.
>>> PRST is needed in SSDT since its base will depend on
>>> chipset and will be dynamically set by QEMU.
>>> Also move PRSC() method along with PRST since cross
>>> table reference to PRST doesn't work.
>>
>> Could you clarify this last sentence?
>> I don't mind where it is but I'd like to know
>> where does the limitation come from.
> It's empiric deduction so far I haven't found such limitation in spec yet.
> iasl builds tables just fine but neither linux nor windows were able to find
> Operation region from SSDT when loading DSDT, failing whole table loading
> process. Decompiling DSDT/SSDT tables in guest shows that region is in
> expected scope but OSPM refuses to see it when referenced outside SSDT.
There seem to be four things here:
- the OperationRegion definition,
- its external declaration,
- the Field() declaration,
- use of fields.
I think referencing an OperationRegion defined in another table should
work (by way of External). I suspect the tricky part is with Field():
The fields are parts of the object named by RegionName, but their
names appear in the same scope as the Field term.
So,
- maybe moving PRST only, and leaving the definition of PRS (as part of
Field()) together with PRSC would suffice,
- or, after moving the definition of PRS (as part of Field()) together
with PRST to another table, all references to PRS (in the PRSC method)
would have to be qualified. (But I guess this is what you tried.)
Laszlo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 21:13 ` Laszlo Ersek
@ 2013-12-16 21:22 ` Laszlo Ersek
2013-12-16 21:53 ` Igor Mammedov
1 sibling, 0 replies; 40+ messages in thread
From: Laszlo Ersek @ 2013-12-16 21:22 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, Michael S. Tsirkin, hutao, qemu-devel, brogers, kraxel,
aliguori, kaneshige.kenji, chen.fan.fnst, pbonzini, jjherne
On 12/16/13 22:13, Laszlo Ersek wrote:
> On 12/16/13 21:38, Igor Mammedov wrote:
>> On Mon, 16 Dec 2013 21:30:14 +0200
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
>>>> .. and report range used by it to OSPM via _CRS.
>>>> PRST is needed in SSDT since its base will depend on
>>>> chipset and will be dynamically set by QEMU.
>>>> Also move PRSC() method along with PRST since cross
>>>> table reference to PRST doesn't work.
>>>
>>> Could you clarify this last sentence?
>>> I don't mind where it is but I'd like to know
>>> where does the limitation come from.
>> It's empiric deduction so far I haven't found such limitation in spec yet.
>> iasl builds tables just fine but neither linux nor windows were able to find
>> Operation region from SSDT when loading DSDT, failing whole table loading
>> process. Decompiling DSDT/SSDT tables in guest shows that region is in
>> expected scope but OSPM refuses to see it when referenced outside SSDT.
>
> There seem to be four things here:
> - the OperationRegion definition,
> - its external declaration,
> - the Field() declaration,
> - use of fields.
>
> I think referencing an OperationRegion defined in another table should
> work (by way of External). I suspect the tricky part is with Field():
>
> The fields are parts of the object named by RegionName, but their
> names appear in the same scope as the Field term.
>
> So,
> - maybe moving PRST only, and leaving the definition of PRS (as part of
> Field()) together with PRSC would suffice,
I think this might be exactly what we don in OVMF's builtin tables. (I
could of course fully misinterpret the situation.)
In the DSDT we have a _CRS method that includes:
- External (FWDT, OpRegionObj)
- Field(FWDT, QWordAcc, NoLock, Preserve) { ... fields ... }
- accesses to these fields
In the SSDT we have:
- OperationRegion(FWDT, ...)
So I guess:
- you could put the PRST in the SSDT (this is the goal),
- you could keep the PRSC method in DSDT,
- but an External reference and the PRS field declaration would also
have to remain in DSDT.
Laszlo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 20:38 ` Igor Mammedov
2013-12-16 21:13 ` Laszlo Ersek
@ 2013-12-16 21:44 ` Laszlo Ersek
2013-12-16 21:59 ` Igor Mammedov
1 sibling, 1 reply; 40+ messages in thread
From: Laszlo Ersek @ 2013-12-16 21:44 UTC (permalink / raw)
To: Igor Mammedov
Cc: aliguori, Michael S. Tsirkin, qemu-devel, hutao, jjherne,
brogers, kraxel, stefanha, kaneshige.kenji, chen.fan.fnst,
pbonzini
On 12/16/13 21:38, Igor Mammedov wrote:
> On Mon, 16 Dec 2013 21:30:14 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
>>> .. and report range used by it to OSPM via _CRS.
>>> PRST is needed in SSDT since its base will depend on
>>> chipset and will be dynamically set by QEMU.
>>> Also move PRSC() method along with PRST since cross
>>> table reference to PRST doesn't work.
>>
>> Could you clarify this last sentence?
>> I don't mind where it is but I'd like to know
>> where does the limitation come from.
> It's empiric deduction so far I haven't found such limitation in spec yet.
> iasl builds tables just fine but neither linux nor windows were able to find
> Operation region from SSDT when loading DSDT, failing whole table loading
> process. Decompiling DSDT/SSDT tables in guest shows that region is in
> expected scope but OSPM refuses to see it when referenced outside SSDT.
> Maybe there is some AML magic to make it work, I'm not aware of.
> The same thing I had to do for memory hotplug as well. So I've tried to play
> nicely 2 times and I have ended up with this solution both times.
Would this work?
diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
index 995b415..34fad66 100644
--- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
@@ -52,8 +52,8 @@ Scope(\_SB) {
Sleep(200)
}
- OperationRegion(PRST, SystemIO, 0xaf00, 32)
- Field(PRST, ByteAcc, NoLock, Preserve) {
+ External(\_SB.CPHD.PRST, OpRegionObj)
+ Field(\_SB.CPHD.PRST, ByteAcc, NoLock, Preserve) {
PRS, 256
}
Method(PRSC, 0) {
diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
index a4484b8..b831fc3 100644
--- a/hw/i386/ssdt-misc.dsl
+++ b/hw/i386/ssdt-misc.dsl
@@ -116,4 +116,29 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
}
}
}
+ Scope(\_SB) {
+ Device(CPHD) {
+ Name(_HID, EISAID("PNP0C08"))
+ Name(CPPL, 32) // cpu-gpe length
+ Name(CPHP, 0xaf00)
+
+ OperationRegion(PRST, SystemIO, CPHP, CPPL)
+
+ /* Leave bit 0 cleared to avoid Windows BSOD */
+ Name(_STA, 0xA)
+
+ Method(_CRS, 0) {
+ Store(ResourceTemplate() {
+ IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
+ }, Local0)
+
+ CreateWordField(Local0, IO._MIN, IOMN)
+ CreateWordField(Local0, IO._MAX, IOMX)
+
+ Store(CPHP, IOMN)
+ Subtract(Add(CPHP, CPPL), 1, IOMX)
+ Return(Local0)
+ }
+ } // Device(CPHD)
+ } // Scope(\_SB)
}
Thanks
Laszlo
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 21:13 ` Laszlo Ersek
2013-12-16 21:22 ` Laszlo Ersek
@ 2013-12-16 21:53 ` Igor Mammedov
2013-12-17 10:39 ` Michael S. Tsirkin
1 sibling, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-16 21:53 UTC (permalink / raw)
To: Laszlo Ersek
Cc: stefanha, Michael S. Tsirkin, hutao, qemu-devel, brogers, kraxel,
aliguori, kaneshige.kenji, chen.fan.fnst, pbonzini, jjherne
On Mon, 16 Dec 2013 22:13:30 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/16/13 21:38, Igor Mammedov wrote:
> > On Mon, 16 Dec 2013 21:30:14 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> >>> .. and report range used by it to OSPM via _CRS.
> >>> PRST is needed in SSDT since its base will depend on
> >>> chipset and will be dynamically set by QEMU.
> >>> Also move PRSC() method along with PRST since cross
> >>> table reference to PRST doesn't work.
> >>
> >> Could you clarify this last sentence?
> >> I don't mind where it is but I'd like to know
> >> where does the limitation come from.
> > It's empiric deduction so far I haven't found such limitation in spec yet.
> > iasl builds tables just fine but neither linux nor windows were able to find
> > Operation region from SSDT when loading DSDT, failing whole table loading
> > process. Decompiling DSDT/SSDT tables in guest shows that region is in
> > expected scope but OSPM refuses to see it when referenced outside SSDT.
>
> There seem to be four things here:
> - the OperationRegion definition,
> - its external declaration,
> - the Field() declaration,
> - use of fields.
>
> I think referencing an OperationRegion defined in another table should
> work (by way of External). I suspect the tricky part is with Field():
^^^ it looks like it should work and decompiled tables
look fine as well but it unfortunately doesn't.
>
> The fields are parts of the object named by RegionName, but their
> names appear in the same scope as the Field term.
>
> So,
> - maybe moving PRST only, and leaving the definition of PRS (as part of
> Field()) together with PRSC would suffice,
That was the first thing I've tried.
> - or, after moving the definition of PRS (as part of Field()) together
> with PRST to another table, all references to PRS (in the PRSC method)
> would have to be qualified. (But I guess this is what you tried.)
yep, that didn't work too.
I'm not fun of moving a bunch of code around, but looks like there is
no other way. I'd be happy to try if there are any other suggestions.
>
> Laszlo
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 21:44 ` Laszlo Ersek
@ 2013-12-16 21:59 ` Igor Mammedov
2013-12-16 22:22 ` Laszlo Ersek
0 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-16 21:59 UTC (permalink / raw)
To: Laszlo Ersek
Cc: aliguori, Michael S. Tsirkin, qemu-devel, hutao, jjherne,
brogers, kraxel, stefanha, kaneshige.kenji, chen.fan.fnst,
pbonzini
On Mon, 16 Dec 2013 22:44:46 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/16/13 21:38, Igor Mammedov wrote:
> > On Mon, 16 Dec 2013 21:30:14 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> >>> .. and report range used by it to OSPM via _CRS.
> >>> PRST is needed in SSDT since its base will depend on
> >>> chipset and will be dynamically set by QEMU.
> >>> Also move PRSC() method along with PRST since cross
> >>> table reference to PRST doesn't work.
> >>
> >> Could you clarify this last sentence?
> >> I don't mind where it is but I'd like to know
> >> where does the limitation come from.
> > It's empiric deduction so far I haven't found such limitation in spec yet.
> > iasl builds tables just fine but neither linux nor windows were able to find
> > Operation region from SSDT when loading DSDT, failing whole table loading
> > process. Decompiling DSDT/SSDT tables in guest shows that region is in
> > expected scope but OSPM refuses to see it when referenced outside SSDT.
> > Maybe there is some AML magic to make it work, I'm not aware of.
> > The same thing I had to do for memory hotplug as well. So I've tried to play
> > nicely 2 times and I have ended up with this solution both times.
>
> Would this work?
>
> diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> index 995b415..34fad66 100644
> --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> @@ -52,8 +52,8 @@ Scope(\_SB) {
> Sleep(200)
> }
>
> - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> - Field(PRST, ByteAcc, NoLock, Preserve) {
> + External(\_SB.CPHD.PRST, OpRegionObj)
> + Field(\_SB.CPHD.PRST, ByteAcc, NoLock, Preserve) {
that was my first patch attempt :)
You have to be careful and touch [q35-]acpi-dsdt.dsl since make doesn't
handle deps to acpi-dsdt-cpu-hotplug.dsl correctly.
After that RHEL6 guest fails to load ACPI tables.
> PRS, 256
> }
> Method(PRSC, 0) {
> diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> index a4484b8..b831fc3 100644
> --- a/hw/i386/ssdt-misc.dsl
> +++ b/hw/i386/ssdt-misc.dsl
> @@ -116,4 +116,29 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> }
> }
> }
> + Scope(\_SB) {
> + Device(CPHD) {
> + Name(_HID, EISAID("PNP0C08"))
> + Name(CPPL, 32) // cpu-gpe length
> + Name(CPHP, 0xaf00)
> +
> + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> +
> + /* Leave bit 0 cleared to avoid Windows BSOD */
> + Name(_STA, 0xA)
> +
> + Method(_CRS, 0) {
> + Store(ResourceTemplate() {
> + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> + }, Local0)
> +
> + CreateWordField(Local0, IO._MIN, IOMN)
> + CreateWordField(Local0, IO._MAX, IOMX)
> +
> + Store(CPHP, IOMN)
> + Subtract(Add(CPHP, CPPL), 1, IOMX)
> + Return(Local0)
> + }
> + } // Device(CPHD)
> + } // Scope(\_SB)
> }
>
> Thanks
> Laszlo
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 19:53 ` Michael S. Tsirkin
@ 2013-12-16 22:15 ` Igor Mammedov
2013-12-22 14:51 ` Igor Mammedov
1 sibling, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-16 22:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aliguori, hutao, jjherne, qemu-devel, kraxel, stefanha,
kaneshige.kenji, chen.fan.fnst, pbonzini, brogers, lersek
On Mon, 16 Dec 2013 21:53:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > .. and report range used by it to OSPM via _CRS.
> > PRST is needed in SSDT since its base will depend on
> > chipset and will be dynamically set by QEMU.
> > Also move PRSC() method along with PRST since cross
> > table reference to PRST doesn't work.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > hw/i386/acpi-dsdt.dsl | 2 +-
> > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 68 insertions(+), 40 deletions(-)
> >
> > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > index 995b415..f26f81b 100644
> > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > @@ -20,6 +20,7 @@
> > Scope(\_SB) {
> > /* Objects filled in by run-time generated SSDT */
> > External(NTFY, MethodObj)
> > + External(\_SB.CPHD.PRSC, MethodObj)
> > External(CPON, PkgObj)
> >
> > /* Methods called by run-time generated SSDT Processor objects */
> > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > // _EJ0 method - eject callback
> > Sleep(200)
> > }
> > -
> > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > - PRS, 256
> > - }
> > - Method(PRSC, 0) {
> > - // Local5 = active cpu bitmap
> > - Store(PRS, Local5)
> > - // Local2 = last read byte from bitmap
> > - Store(Zero, Local2)
> > - // Local0 = Processor ID / APIC ID iterator
> > - Store(Zero, Local0)
> > - While (LLess(Local0, SizeOf(CPON))) {
> > - // Local1 = CPON flag for this cpu
> > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > - If (And(Local0, 0x07)) {
> > - // Shift down previously read bitmap byte
> > - ShiftRight(Local2, 1, Local2)
> > - } Else {
> > - // Read next byte from cpu bitmap
> > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > - }
> > - // Local3 = active state for this cpu
> > - Store(And(Local2, 1), Local3)
> > -
> > - If (LNotEqual(Local1, Local3)) {
> > - // State change - update CPON with new state
> > - Store(Local3, Index(CPON, Local0))
> > - // Do CPU notify
> > - If (LEqual(Local3, 1)) {
> > - NTFY(Local0, 1)
> > - } Else {
> > - NTFY(Local0, 3)
> > - }
> > - }
> > - Increment(Local0)
> > - }
> > - }
> > }
> > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > index 90efce0..fa9f2d4 100644
> > --- a/hw/i386/acpi-dsdt.dsl
> > +++ b/hw/i386/acpi-dsdt.dsl
> > @@ -311,7 +311,7 @@ DefinitionBlock (
> > }
> > Method(_E02) {
> > // CPU hotplug event
> > - \_SB.PRSC()
> > + \_SB.CPHD.PRSC()
> > }
> > Method(_L03) {
> > }
> > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > index 22baa58..9ccc543 100644
> > --- a/hw/i386/q35-acpi-dsdt.dsl
> > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > @@ -420,7 +420,7 @@ DefinitionBlock (
> > }
> > Method(_E02) {
> > // CPU hotplug event
> > - \_SB.PRSC()
> > + \_SB.CPHD.PRSC()
> > }
> > Method(_L03) {
> > }
> > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > index a4484b8..ec8893c 100644
> > --- a/hw/i386/ssdt-misc.dsl
> > +++ b/hw/i386/ssdt-misc.dsl
> > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > }
> > }
> > }
> > + Scope(\_SB) {
> > + External(NTFY, MethodObj)
> > + External(CPON, PkgObj)
> > +
> > + Device(CPHD) {
> > + Name(_HID, EISAID("PNP0C08"))
> > + Name(CPPL, 32) // cpu-gpe length
> > + Name(CPHP, 0xaf00)
> > +
> > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > + PRS, 256
> > + }
> > +
> > + Method(PRSC, 0) {
> > + // Local5 = active cpu bitmap
> > + Store(PRS, Local5)
> > + // Local2 = last read byte from bitmap
> > + Store(Zero, Local2)
> > + // Local0 = Processor ID / APIC ID iterator
> > + Store(Zero, Local0)
> > + While (LLess(Local0, SizeOf(CPON))) {
> > + // Local1 = CPON flag for this cpu
> > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > + If (And(Local0, 0x07)) {
> > + // Shift down previously read bitmap byte
> > + ShiftRight(Local2, 1, Local2)
> > + } Else {
> > + // Read next byte from cpu bitmap
> > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > + }
> > + // Local3 = active state for this cpu
> > + Store(And(Local2, 1), Local3)
> > +
> > + If (LNotEqual(Local1, Local3)) {
> > + // State change - update CPON with new state
> > + Store(Local3, Index(CPON, Local0))
> > + // Do CPU notify
> > + If (LEqual(Local3, 1)) {
> > + NTFY(Local0, 1)
> > + } Else {
> > + NTFY(Local0, 3)
> > + }
> > + }
> > + Increment(Local0)
> > + }
> > + }
> > +
> > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > + Name(_STA, 0xA)
>
> This shared the problem you yourself pointed out with
> patch 'pc: ACPI BIOS: implement memory hotplug':
> if we make device non present ospm can ignore our _CRS.
I repeat my question is there need to expose it OperationRegion as _CRS?
>
> Can't we get a better handle on why windows crashes?
Short of installing debug build and looking at crash dump and/or reverse
engineering acpi.sys, might give a clue, I don't have any other idea.
Are there any simpler suggestions?
>
>
> > +
> > + Method(_CRS, 0) {
> > + Store(ResourceTemplate() {
> > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > + }, Local0)
> > +
> > + CreateWordField(Local0, IO._MIN, IOMN)
> > + CreateWordField(Local0, IO._MAX, IOMX)
> > +
> > + Store(CPHP, IOMN)
> > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > + Return(Local0)
> > + }
> > + } // Device(CPHD)
> > + } // Scope(\_SB)
> > }
> > --
> > 1.8.3.1
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 21:59 ` Igor Mammedov
@ 2013-12-16 22:22 ` Laszlo Ersek
2013-12-16 23:13 ` Igor Mammedov
0 siblings, 1 reply; 40+ messages in thread
From: Laszlo Ersek @ 2013-12-16 22:22 UTC (permalink / raw)
To: Igor Mammedov
Cc: aliguori, Michael S. Tsirkin, qemu-devel, hutao, jjherne,
brogers, kraxel, stefanha, kaneshige.kenji, chen.fan.fnst,
pbonzini
On 12/16/13 22:59, Igor Mammedov wrote:
> On Mon, 16 Dec 2013 22:44:46 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 12/16/13 21:38, Igor Mammedov wrote:
>>> On Mon, 16 Dec 2013 21:30:14 +0200
>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
>>>>> .. and report range used by it to OSPM via _CRS.
>>>>> PRST is needed in SSDT since its base will depend on
>>>>> chipset and will be dynamically set by QEMU.
>>>>> Also move PRSC() method along with PRST since cross
>>>>> table reference to PRST doesn't work.
>>>>
>>>> Could you clarify this last sentence?
>>>> I don't mind where it is but I'd like to know
>>>> where does the limitation come from.
>>> It's empiric deduction so far I haven't found such limitation in spec yet.
>>> iasl builds tables just fine but neither linux nor windows were able to find
>>> Operation region from SSDT when loading DSDT, failing whole table loading
>>> process. Decompiling DSDT/SSDT tables in guest shows that region is in
>>> expected scope but OSPM refuses to see it when referenced outside SSDT.
>>> Maybe there is some AML magic to make it work, I'm not aware of.
>>> The same thing I had to do for memory hotplug as well. So I've tried to play
>>> nicely 2 times and I have ended up with this solution both times.
>>
>> Would this work?
>>
>> diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
>> index 995b415..34fad66 100644
>> --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
>> +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
>> @@ -52,8 +52,8 @@ Scope(\_SB) {
>> Sleep(200)
>> }
>>
>> - OperationRegion(PRST, SystemIO, 0xaf00, 32)
>> - Field(PRST, ByteAcc, NoLock, Preserve) {
>> + External(\_SB.CPHD.PRST, OpRegionObj)
>> + Field(\_SB.CPHD.PRST, ByteAcc, NoLock, Preserve) {
> that was my first patch attempt :)
> You have to be careful and touch [q35-]acpi-dsdt.dsl since make doesn't
> handle deps to acpi-dsdt-cpu-hotplug.dsl correctly.
Yeah I think I noticed from the "make" output :) I grepped it for this
file's name.
> After that RHEL6 guest fails to load ACPI tables.
Could even be an ACPICA problem...
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 22:22 ` Laszlo Ersek
@ 2013-12-16 23:13 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-16 23:13 UTC (permalink / raw)
To: Laszlo Ersek
Cc: stefanha, Michael S. Tsirkin, qemu-devel, hutao, jjherne,
brogers, kraxel, aliguori, kaneshige.kenji, chen.fan.fnst,
pbonzini
On Mon, 16 Dec 2013 23:22:56 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/16/13 22:59, Igor Mammedov wrote:
> > On Mon, 16 Dec 2013 22:44:46 +0100
> > Laszlo Ersek <lersek@redhat.com> wrote:
> >
> >> On 12/16/13 21:38, Igor Mammedov wrote:
> >>> On Mon, 16 Dec 2013 21:30:14 +0200
> >>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>
> >>>> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> >>>>> .. and report range used by it to OSPM via _CRS.
> >>>>> PRST is needed in SSDT since its base will depend on
> >>>>> chipset and will be dynamically set by QEMU.
> >>>>> Also move PRSC() method along with PRST since cross
> >>>>> table reference to PRST doesn't work.
> >>>>
> >>>> Could you clarify this last sentence?
> >>>> I don't mind where it is but I'd like to know
> >>>> where does the limitation come from.
> >>> It's empiric deduction so far I haven't found such limitation in spec yet.
> >>> iasl builds tables just fine but neither linux nor windows were able to find
> >>> Operation region from SSDT when loading DSDT, failing whole table loading
> >>> process. Decompiling DSDT/SSDT tables in guest shows that region is in
> >>> expected scope but OSPM refuses to see it when referenced outside SSDT.
> >>> Maybe there is some AML magic to make it work, I'm not aware of.
> >>> The same thing I had to do for memory hotplug as well. So I've tried to play
> >>> nicely 2 times and I have ended up with this solution both times.
> >>
> >> Would this work?
> >>
> >> diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> >> index 995b415..34fad66 100644
> >> --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> >> +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> >> @@ -52,8 +52,8 @@ Scope(\_SB) {
> >> Sleep(200)
> >> }
> >>
> >> - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> >> - Field(PRST, ByteAcc, NoLock, Preserve) {
> >> + External(\_SB.CPHD.PRST, OpRegionObj)
> >> + Field(\_SB.CPHD.PRST, ByteAcc, NoLock, Preserve) {
> > that was my first patch attempt :)
> > You have to be careful and touch [q35-]acpi-dsdt.dsl since make doesn't
> > handle deps to acpi-dsdt-cpu-hotplug.dsl correctly.
>
> Yeah I think I noticed from the "make" output :) I grepped it for this
> file's name.
>
> > After that RHEL6 guest fails to load ACPI tables.
>
> Could even be an ACPICA problem...
Maybe it's iasl issue or linux&windows implementation is wrong (unlikely).
>
> Thanks!
> Laszlo
>
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 21:53 ` Igor Mammedov
@ 2013-12-17 10:39 ` Michael S. Tsirkin
0 siblings, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-17 10:39 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, Laszlo Ersek
On Mon, Dec 16, 2013 at 10:53:24PM +0100, Igor Mammedov wrote:
> On Mon, 16 Dec 2013 22:13:30 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
> > On 12/16/13 21:38, Igor Mammedov wrote:
> > > On Mon, 16 Dec 2013 21:30:14 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > >> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > >>> .. and report range used by it to OSPM via _CRS.
> > >>> PRST is needed in SSDT since its base will depend on
> > >>> chipset and will be dynamically set by QEMU.
> > >>> Also move PRSC() method along with PRST since cross
> > >>> table reference to PRST doesn't work.
> > >>
> > >> Could you clarify this last sentence?
> > >> I don't mind where it is but I'd like to know
> > >> where does the limitation come from.
> > > It's empiric deduction so far I haven't found such limitation in spec yet.
> > > iasl builds tables just fine but neither linux nor windows were able to find
> > > Operation region from SSDT when loading DSDT, failing whole table loading
> > > process. Decompiling DSDT/SSDT tables in guest shows that region is in
> > > expected scope but OSPM refuses to see it when referenced outside SSDT.
> >
> > There seem to be four things here:
> > - the OperationRegion definition,
> > - its external declaration,
> > - the Field() declaration,
> > - use of fields.
> >
> > I think referencing an OperationRegion defined in another table should
> > work (by way of External). I suspect the tricky part is with Field():
> ^^^ it looks like it should work and decompiled tables
> look fine as well but it unfortunately doesn't.
>
> >
> > The fields are parts of the object named by RegionName, but their
> > names appear in the same scope as the Field term.
> >
> > So,
> > - maybe moving PRST only, and leaving the definition of PRS (as part of
> > Field()) together with PRSC would suffice,
> That was the first thing I've tried.
>
> > - or, after moving the definition of PRS (as part of Field()) together
> > with PRST to another table, all references to PRS (in the PRSC method)
> > would have to be qualified. (But I guess this is what you tried.)
> yep, that didn't work too.
>
> I'm not fun of moving a bunch of code around, but looks like there is
> no other way. I'd be happy to try if there are any other suggestions.
I'm not sure we need to spend too much time on it.
Just clarifying that 'doesn't work' means 'builds but
makes OSPM fail to resolve the OperationRegion,
even though the spec makes it look like this should work'
in the commit log should be enough.
> >
> > Laszlo
> >
>
>
> --
> Regards,
> Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35
2013-12-13 16:22 ` [Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35 Igor Mammedov
@ 2013-12-19 14:14 ` Michael S. Tsirkin
2013-12-19 15:13 ` Igor Mammedov
0 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-19 14:14 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Fri, Dec 13, 2013 at 05:22:08PM +0100, Igor Mammedov wrote:
> .. so it could be used for adding CPU hotplug to Q35 machine
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Sounds good but please call this file cpu_hotplug.c/.h or cpu-hotplug.c/h
and make all prefixes acpi_cpu_hotplug and AcpiCpuHotplug.
> ---
> hw/acpi/Makefile.objs | 2 +-
> hw/acpi/hotplug.c | 65 +++++++++++++++++++++++++++++++++++++++
> hw/acpi/piix4.c | 78 +++++------------------------------------------
> include/hw/acpi/hotplug.h | 31 +++++++++++++++++++
> 4 files changed, 104 insertions(+), 72 deletions(-)
> create mode 100644 hw/acpi/hotplug.c
> create mode 100644 include/hw/acpi/hotplug.h
>
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index a0b63b5..41dec06 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -1,2 +1,2 @@
> -common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o
> +common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o hotplug.o
>
> diff --git a/hw/acpi/hotplug.c b/hw/acpi/hotplug.c
> new file mode 100644
> index 0000000..fcc0988
> --- /dev/null
> +++ b/hw/acpi/hotplug.c
> @@ -0,0 +1,65 @@
> +/*
> + * QEMU ACPI hotplug utilities
> + *
> + * Copyright (C) 2013 Red Hat Inc
> + *
> + * Authors:
> + * Igor Mammedov <imammedo@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/hw.h"
> +#include "hw/acpi/hotplug.h"
> +
> +static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> + AcpiCPUHotplug *cpus = opaque;
> + uint64_t val = cpus->sts[addr];
> +
> + return val;
> +}
> +
> +static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned int size)
> +{
> + /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> +}
> +
> +static const MemoryRegionOps acpi_cpu_hotplug_ops = {
> + .read = cpu_status_read,
> + .write = cpu_status_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid = {
> + .min_access_size = 1,
> + .max_access_size = 1,
> + },
> +};
> +
> +void acpi_hotplug_cpu_add(ACPIGPE *gpe, AcpiCPUHotplug *g, CPUState *cpu)
> +{
> + CPUClass *k = CPU_GET_CLASS(cpu);
> + int64_t cpu_id;
> +
> + *gpe->sts = *gpe->sts | ACPI_CPU_HOTPLUG_STATUS;
> + cpu_id = k->get_arch_id(CPU(cpu));
> + g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> +}
> +
> +void acpi_hotplug_cpu_init(MemoryRegion *parent, Object *owner,
> + AcpiCPUHotplug *gpe_cpu, uint16_t base)
> +{
> + CPUState *cpu;
> +
> + CPU_FOREACH(cpu) {
> + CPUClass *cc = CPU_GET_CLASS(cpu);
> + int64_t id = cc->get_arch_id(cpu);
> +
> + g_assert((id / 8) < ACPI_GPE_PROC_LEN);
> + gpe_cpu->sts[id / 8] |= (1 << (id % 8));
> + }
> + gpe_cpu->io_base = base;
> + memory_region_init_io(&gpe_cpu->io, owner, &acpi_cpu_hotplug_ops,
> + gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> + memory_region_add_subregion(parent, gpe_cpu->io_base, &gpe_cpu->io);
> +}
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index b6b97ce..8ab85b2 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -30,6 +30,7 @@
> #include "hw/nvram/fw_cfg.h"
> #include "exec/address-spaces.h"
> #include "hw/acpi/piix4.h"
> +#include "hw/acpi/hotplug.h"
>
> //#define DEBUG
>
> @@ -50,20 +51,14 @@
> #define PCI_RMV_BASE 0xae0c
>
> #define PIIX4_PROC_BASE 0xaf00
> -#define PIIX4_PROC_LEN 32
>
> #define PIIX4_PCI_HOTPLUG_STATUS 2
> -#define PIIX4_CPU_HOTPLUG_STATUS 4
>
> struct pci_status {
> uint32_t up; /* deprecated, maintained for migration compatibility */
> uint32_t down;
> };
>
> -typedef struct CPUStatus {
> - uint8_t sts[PIIX4_PROC_LEN];
> -} CPUStatus;
> -
> typedef struct PIIX4PMState {
> /*< private >*/
> PCIDevice parent_obj;
> @@ -74,7 +69,6 @@ typedef struct PIIX4PMState {
>
> MemoryRegion io_gpe;
> MemoryRegion io_pci;
> - MemoryRegion io_cpu;
> ACPIREGS ar;
>
> APMState apm;
> @@ -97,7 +91,7 @@ typedef struct PIIX4PMState {
> uint8_t disable_s4;
> uint8_t s4_val;
>
> - CPUStatus gpe_cpu;
> + AcpiCPUHotplug gpe_cpu;
> Notifier cpu_added_notifier;
> } PIIX4PMState;
>
> @@ -628,61 +622,13 @@ static const MemoryRegionOps piix4_pci_ops = {
> },
> };
>
> -static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> -{
> - PIIX4PMState *s = opaque;
> - CPUStatus *cpus = &s->gpe_cpu;
> - uint64_t val = cpus->sts[addr];
> -
> - return val;
> -}
> -
> -static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> - unsigned int size)
> -{
> - /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> -}
> -
> -static const MemoryRegionOps cpu_hotplug_ops = {
> - .read = cpu_status_read,
> - .write = cpu_status_write,
> - .endianness = DEVICE_LITTLE_ENDIAN,
> - .valid = {
> - .min_access_size = 1,
> - .max_access_size = 1,
> - },
> -};
> -
> -typedef enum {
> - PLUG,
> - UNPLUG,
> -} HotplugEventType;
> -
> -static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
> - HotplugEventType action)
> -{
> - CPUStatus *g = &s->gpe_cpu;
> - ACPIGPE *gpe = &s->ar.gpe;
> - CPUClass *k = CPU_GET_CLASS(cpu);
> - int64_t cpu_id;
> -
> - assert(s != NULL);
> -
> - *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
> - cpu_id = k->get_arch_id(CPU(cpu));
> - if (action == PLUG) {
> - g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> - } else {
> - g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
> - }
> - acpi_update_sci(&s->ar, s->irq);
> -}
> -
> static void piix4_cpu_added_req(Notifier *n, void *opaque)
> {
> PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
>
> - piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
> + assert(s != NULL);
> + acpi_hotplug_cpu_add(&s->ar.gpe, &s->gpe_cpu, CPU(opaque));
> + acpi_update_sci(&s->ar, s->irq);
> }
>
> static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> @@ -691,8 +637,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> PCIBus *bus, PIIX4PMState *s)
> {
> - CPUState *cpu;
> -
> memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> "acpi-gpe0", GPE_LEN);
> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> @@ -703,16 +647,8 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> &s->io_pci);
> pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
>
> - CPU_FOREACH(cpu) {
> - CPUClass *cc = CPU_GET_CLASS(cpu);
> - int64_t id = cc->get_arch_id(cpu);
> -
> - g_assert((id / 8) < PIIX4_PROC_LEN);
> - s->gpe_cpu.sts[id / 8] |= (1 << (id % 8));
> - }
> - memory_region_init_io(&s->io_cpu, OBJECT(s), &cpu_hotplug_ops, s,
> - "acpi-cpu-hotplug", PIIX4_PROC_LEN);
> - memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
> + acpi_hotplug_cpu_init(parent, OBJECT(s), &s->gpe_cpu,
> + PIIX4_PROC_BASE);
> s->cpu_added_notifier.notify = piix4_cpu_added_req;
> qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> }
> diff --git a/include/hw/acpi/hotplug.h b/include/hw/acpi/hotplug.h
> new file mode 100644
> index 0000000..8bc176b
> --- /dev/null
> +++ b/include/hw/acpi/hotplug.h
> @@ -0,0 +1,31 @@
> +/*
> + * QEMU ACPI hotplug utilities
> + *
> + * Copyright (C) 2013 Red Hat Inc
> + *
> + * Authors:
> + * Igor Mammedov <imammedo@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef ACPI_HOTPLUG_H
> +#define ACPI_HOTPLUG_H
> +
> +#include "hw/acpi/acpi.h"
> +
> +#define ACPI_CPU_HOTPLUG_STATUS 4
> +
> +#define ACPI_GPE_PROC_LEN 32
> +
> +typedef struct AcpiCPUHotplug {
> + MemoryRegion io;
> + uint16_t io_base;
> + uint8_t sts[ACPI_GPE_PROC_LEN];
> +} AcpiCPUHotplug;
> +
> +void acpi_hotplug_cpu_add(ACPIGPE *gpe, AcpiCPUHotplug *g, CPUState *cpu);
> +
> +void acpi_hotplug_cpu_init(MemoryRegion *parent, Object *owner,
> + AcpiCPUHotplug *gpe_cpu, uint16_t base);
> +#endif
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 01/11] acpi: piix4: remove not needed GPE0 mask
2013-12-13 16:22 ` [Qemu-devel] [PATCH 01/11] acpi: piix4: remove not needed GPE0 mask Igor Mammedov
@ 2013-12-19 14:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-19 14:16 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Fri, Dec 13, 2013 at 05:22:06PM +0100, Igor Mammedov wrote:
> Hardcoded GPE0 mask isn't really needed. Since GPE0_STS initialized
> with all bits cleared and only QEMU itself can set bits there (i.e.
> guest can only clear bits in it). So guest can't triger SCI
> by setting _STS & _EN bits and there is not reason to mask out not
> supported _STS bits since they shouldn't be set by QEMU in the first
> place.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Applied, thanks.
> ---
> hw/acpi/piix4.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 93849c8..b4caeab 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -122,8 +122,7 @@ static void pm_update_sci(PIIX4PMState *s)
> ACPI_BITMASK_POWER_BUTTON_ENABLE |
> ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> - (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
> - (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
> + ((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) != 0);
>
> qemu_set_irq(s->irq, sci_level);
> /* schedule a timer interruption if needed */
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 02/11] acpi: factor out common pm_update_sci() into acpi core
2013-12-13 16:22 ` [Qemu-devel] [PATCH 02/11] acpi: factor out common pm_update_sci() into acpi core Igor Mammedov
@ 2013-12-19 14:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-19 14:16 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Fri, Dec 13, 2013 at 05:22:07PM +0100, Igor Mammedov wrote:
> ... and rename it into acpi_update_sci() since it changes
> SCI on only on PM registers status.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Applied, thanks.
> ---
> ---
> hw/acpi/core.c | 18 ++++++++++++++++++
> hw/acpi/ich9.c | 23 ++---------------------
> hw/acpi/piix4.c | 26 ++++----------------------
> include/hw/acpi/acpi.h | 8 ++++++++
> 4 files changed, 32 insertions(+), 43 deletions(-)
>
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 58308a3..79414b4 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -662,3 +662,21 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
>
> return val;
> }
> +
> +void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
> +{
> + int sci_level, pm1a_sts;
> +
> + pm1a_sts = acpi_pm1_evt_get_sts(regs);
> +
> + sci_level = ((pm1a_sts &
> + regs->pm1.evt.en & ACPI_BITMASK_PM1_COMMON_ENABLED) != 0) ||
> + ((regs->gpe.sts[0] & regs->gpe.en[0]) != 0);
> +
> + qemu_set_irq(irq, sci_level);
> +
> + /* schedule a timer interruption if needed */
> + acpi_pm_tmr_update(regs,
> + (regs->pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
> + !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
> +}
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 7e0429e..dcdef7c 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -44,29 +44,10 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
> #define ICH9_DEBUG(fmt, ...) do { } while (0)
> #endif
>
> -static void pm_update_sci(ICH9LPCPMRegs *pm)
> -{
> - int sci_level, pm1a_sts;
> -
> - pm1a_sts = acpi_pm1_evt_get_sts(&pm->acpi_regs);
> -
> - sci_level = (((pm1a_sts & pm->acpi_regs.pm1.evt.en) &
> - (ACPI_BITMASK_RT_CLOCK_ENABLE |
> - ACPI_BITMASK_POWER_BUTTON_ENABLE |
> - ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> - ACPI_BITMASK_TIMER_ENABLE)) != 0);
> - qemu_set_irq(pm->irq, sci_level);
> -
> - /* schedule a timer interruption if needed */
> - acpi_pm_tmr_update(&pm->acpi_regs,
> - (pm->acpi_regs.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
> - !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
> -}
> -
> static void ich9_pm_update_sci_fn(ACPIREGS *regs)
> {
> ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
> - pm_update_sci(pm);
> + acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
>
> static uint64_t ich9_gpe_readb(void *opaque, hwaddr addr, unsigned width)
> @@ -193,7 +174,7 @@ static void pm_reset(void *opaque)
> pm->smi_en |= ICH9_PMIO_SMI_EN_APMC_EN;
> }
>
> - pm_update_sci(pm);
> + acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
>
> static void pm_powerdown_req(Notifier *n, void *opaque)
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index b4caeab..b6b97ce 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -112,28 +112,10 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> #define ACPI_ENABLE 0xf1
> #define ACPI_DISABLE 0xf0
>
> -static void pm_update_sci(PIIX4PMState *s)
> -{
> - int sci_level, pmsts;
> -
> - pmsts = acpi_pm1_evt_get_sts(&s->ar);
> - sci_level = (((pmsts & s->ar.pm1.evt.en) &
> - (ACPI_BITMASK_RT_CLOCK_ENABLE |
> - ACPI_BITMASK_POWER_BUTTON_ENABLE |
> - ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> - ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> - ((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) != 0);
> -
> - qemu_set_irq(s->irq, sci_level);
> - /* schedule a timer interruption if needed */
> - acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
> - !(pmsts & ACPI_BITMASK_TIMER_STATUS));
> -}
> -
> static void pm_tmr_timer(ACPIREGS *ar)
> {
> PIIX4PMState *s = container_of(ar, PIIX4PMState, ar);
> - pm_update_sci(s);
> + acpi_update_sci(&s->ar, s->irq);
> }
>
> static void apm_ctrl_changed(uint32_t val, void *arg)
> @@ -577,7 +559,7 @@ static void gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
> PIIX4PMState *s = opaque;
>
> acpi_gpe_ioport_writeb(&s->ar, addr, val);
> - pm_update_sci(s);
> + acpi_update_sci(&s->ar, s->irq);
>
> PIIX4_DPRINTF("gpe write %" HWADDR_PRIx " <== %" PRIu64 "\n", addr, val);
> }
> @@ -693,7 +675,7 @@ static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
> } else {
> g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
> }
> - pm_update_sci(s);
> + acpi_update_sci(&s->ar, s->irq);
> }
>
> static void piix4_cpu_added_req(Notifier *n, void *opaque)
> @@ -767,7 +749,7 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> disable_device(s, slot);
> }
>
> - pm_update_sci(s);
> + acpi_update_sci(&s->ar, s->irq);
>
> return 0;
> }
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 6bbcb17..3e53297 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -69,6 +69,12 @@
> #define ACPI_BITMASK_RT_CLOCK_ENABLE 0x0400
> #define ACPI_BITMASK_PCIEXP_WAKE_DISABLE 0x4000 /* ACPI 3.0 */
>
> +#define ACPI_BITMASK_PM1_COMMON_ENABLED ( \
> + ACPI_BITMASK_RT_CLOCK_ENABLE | \
> + ACPI_BITMASK_POWER_BUTTON_ENABLE | \
> + ACPI_BITMASK_GLOBAL_LOCK_ENABLE | \
> + ACPI_BITMASK_TIMER_ENABLE)
> +
> /* PM1x_CNT */
> #define ACPI_BITMASK_SCI_ENABLE 0x0001
> #define ACPI_BITMASK_BUS_MASTER_RLD 0x0002
> @@ -160,6 +166,8 @@ void acpi_gpe_reset(ACPIREGS *ar);
> void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
> uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
>
> +void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
> +
> /* acpi.c */
> extern int acpi_enabled;
> extern char unsigned *acpi_tables;
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 05/11] acpi: ich9: allow guest to clear SCI rised by GPE
2013-12-13 16:22 ` [Qemu-devel] [PATCH 05/11] acpi: ich9: allow guest to clear SCI rised by GPE Igor Mammedov
@ 2013-12-19 14:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-19 14:16 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Fri, Dec 13, 2013 at 05:22:10PM +0100, Igor Mammedov wrote:
> it fixes IRQ storm since guest isn't able to lower SCI IRQ
> after it has been handled when it clears GPE event.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Applied, thanks.
> ---
> hw/acpi/ich9.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index dcdef7c..30f0df8 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -61,6 +61,7 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
> {
> ICH9LPCPMRegs *pm = opaque;
> acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> + acpi_update_sci(&pm->acpi_regs, pm->irq);
> }
>
> static const MemoryRegionOps ich9_gpe_ops = {
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine
2013-12-13 16:22 ` [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine Igor Mammedov
@ 2013-12-19 14:18 ` Michael S. Tsirkin
2013-12-19 15:17 ` Igor Mammedov
0 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-19 14:18 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Fri, Dec 13, 2013 at 05:22:11PM +0100, Igor Mammedov wrote:
> .. including readonly 'cpu-hotplug-io-base' property
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Is this documented in the Q35 spec? Could not find it there.
If this is our PV code like PIIX, then we need to document this
interface.
> ---
> hw/acpi/ich9.c | 27 +++++++++++++++++++++++++++
> include/hw/acpi/ich9.h | 4 ++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 30f0df8..9c88109 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -44,6 +44,8 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
> #define ICH9_DEBUG(fmt, ...) do { } while (0)
> #endif
>
> +#define ICH9_PROC_BASE 0xa18
> +
> static void ich9_pm_update_sci_fn(ACPIREGS *regs)
> {
> ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
> @@ -185,6 +187,15 @@ static void pm_powerdown_req(Notifier *n, void *opaque)
> acpi_pm1_evt_power_down(&pm->acpi_regs);
> }
>
> +static void ich9_cpu_added_req(Notifier *n, void *opaque)
> +{
> + ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
> +
> + assert(pm != NULL);
> + acpi_hotplug_cpu_add(&pm->acpi_regs.gpe, &pm->gpe_cpu, CPU(opaque));
> + acpi_update_sci(&pm->acpi_regs, pm->irq);
> +}
> +
> void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> qemu_irq sci_irq)
> {
> @@ -210,6 +221,11 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> qemu_register_reset(pm_reset, pm);
> pm->powerdown_notifier.notify = pm_powerdown_req;
> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> +
> + acpi_hotplug_cpu_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> + &pm->gpe_cpu, pm->gpe_cpu.io_base);
> + pm->cpu_added_notifier.notify = ich9_cpu_added_req;
> + qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
> }
>
> static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
> @@ -222,6 +238,14 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
> visit_type_uint32(v, &value, name, errp);
> }
>
> +static void ich9_pm_get_cpu_io_base(Object *obj, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> + visit_type_uint16(v, &s->pm.gpe_cpu.io_base, name, errp);
> +}
> +
> void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> {
> static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> @@ -233,4 +257,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> NULL, NULL, pm, NULL);
> object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> &gpe0_len, errp);
> + pm->gpe_cpu.io_base = ICH9_PROC_BASE;
> + object_property_add(obj, ACPI_CPU_HOTPLUG_IO_BASE_PROP, "int",
> + ich9_pm_get_cpu_io_base, NULL, NULL, NULL, NULL);
> }
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 82fcf9f..c71a9b4 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -22,6 +22,7 @@
> #define HW_ACPI_ICH9_H
>
> #include "hw/acpi/acpi.h"
> +#include "hw/acpi/hotplug.h"
>
> typedef struct ICH9LPCPMRegs {
> /*
> @@ -42,6 +43,9 @@ typedef struct ICH9LPCPMRegs {
>
> uint32_t pm_io_base;
> Notifier powerdown_notifier;
> +
> + AcpiCPUHotplug gpe_cpu;
> + Notifier cpu_added_notifier;
> } ICH9LPCPMRegs;
>
> void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35
2013-12-19 14:14 ` Michael S. Tsirkin
@ 2013-12-19 15:13 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-19 15:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Thu, 19 Dec 2013 16:14:03 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 13, 2013 at 05:22:08PM +0100, Igor Mammedov wrote:
> > .. so it could be used for adding CPU hotplug to Q35 machine
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Sounds good but please call this file cpu_hotplug.c/.h or cpu-hotplug.c/h
> and make all prefixes acpi_cpu_hotplug and AcpiCpuHotplug.
sure
>
>
> > ---
> > hw/acpi/Makefile.objs | 2 +-
> > hw/acpi/hotplug.c | 65 +++++++++++++++++++++++++++++++++++++++
> > hw/acpi/piix4.c | 78 +++++------------------------------------------
> > include/hw/acpi/hotplug.h | 31 +++++++++++++++++++
> > 4 files changed, 104 insertions(+), 72 deletions(-)
> > create mode 100644 hw/acpi/hotplug.c
> > create mode 100644 include/hw/acpi/hotplug.h
> >
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index a0b63b5..41dec06 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -1,2 +1,2 @@
> > -common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o
> > +common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o hotplug.o
> >
> > diff --git a/hw/acpi/hotplug.c b/hw/acpi/hotplug.c
> > new file mode 100644
> > index 0000000..fcc0988
> > --- /dev/null
> > +++ b/hw/acpi/hotplug.c
> > @@ -0,0 +1,65 @@
> > +/*
> > + * QEMU ACPI hotplug utilities
> > + *
> > + * Copyright (C) 2013 Red Hat Inc
> > + *
> > + * Authors:
> > + * Igor Mammedov <imammedo@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "hw/hw.h"
> > +#include "hw/acpi/hotplug.h"
> > +
> > +static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> > +{
> > + AcpiCPUHotplug *cpus = opaque;
> > + uint64_t val = cpus->sts[addr];
> > +
> > + return val;
> > +}
> > +
> > +static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> > + unsigned int size)
> > +{
> > + /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> > +}
> > +
> > +static const MemoryRegionOps acpi_cpu_hotplug_ops = {
> > + .read = cpu_status_read,
> > + .write = cpu_status_write,
> > + .endianness = DEVICE_LITTLE_ENDIAN,
> > + .valid = {
> > + .min_access_size = 1,
> > + .max_access_size = 1,
> > + },
> > +};
> > +
> > +void acpi_hotplug_cpu_add(ACPIGPE *gpe, AcpiCPUHotplug *g, CPUState *cpu)
> > +{
> > + CPUClass *k = CPU_GET_CLASS(cpu);
> > + int64_t cpu_id;
> > +
> > + *gpe->sts = *gpe->sts | ACPI_CPU_HOTPLUG_STATUS;
> > + cpu_id = k->get_arch_id(CPU(cpu));
> > + g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> > +}
> > +
> > +void acpi_hotplug_cpu_init(MemoryRegion *parent, Object *owner,
> > + AcpiCPUHotplug *gpe_cpu, uint16_t base)
> > +{
> > + CPUState *cpu;
> > +
> > + CPU_FOREACH(cpu) {
> > + CPUClass *cc = CPU_GET_CLASS(cpu);
> > + int64_t id = cc->get_arch_id(cpu);
> > +
> > + g_assert((id / 8) < ACPI_GPE_PROC_LEN);
> > + gpe_cpu->sts[id / 8] |= (1 << (id % 8));
> > + }
> > + gpe_cpu->io_base = base;
> > + memory_region_init_io(&gpe_cpu->io, owner, &acpi_cpu_hotplug_ops,
> > + gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> > + memory_region_add_subregion(parent, gpe_cpu->io_base, &gpe_cpu->io);
> > +}
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index b6b97ce..8ab85b2 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -30,6 +30,7 @@
> > #include "hw/nvram/fw_cfg.h"
> > #include "exec/address-spaces.h"
> > #include "hw/acpi/piix4.h"
> > +#include "hw/acpi/hotplug.h"
> >
> > //#define DEBUG
> >
> > @@ -50,20 +51,14 @@
> > #define PCI_RMV_BASE 0xae0c
> >
> > #define PIIX4_PROC_BASE 0xaf00
> > -#define PIIX4_PROC_LEN 32
> >
> > #define PIIX4_PCI_HOTPLUG_STATUS 2
> > -#define PIIX4_CPU_HOTPLUG_STATUS 4
> >
> > struct pci_status {
> > uint32_t up; /* deprecated, maintained for migration compatibility */
> > uint32_t down;
> > };
> >
> > -typedef struct CPUStatus {
> > - uint8_t sts[PIIX4_PROC_LEN];
> > -} CPUStatus;
> > -
> > typedef struct PIIX4PMState {
> > /*< private >*/
> > PCIDevice parent_obj;
> > @@ -74,7 +69,6 @@ typedef struct PIIX4PMState {
> >
> > MemoryRegion io_gpe;
> > MemoryRegion io_pci;
> > - MemoryRegion io_cpu;
> > ACPIREGS ar;
> >
> > APMState apm;
> > @@ -97,7 +91,7 @@ typedef struct PIIX4PMState {
> > uint8_t disable_s4;
> > uint8_t s4_val;
> >
> > - CPUStatus gpe_cpu;
> > + AcpiCPUHotplug gpe_cpu;
> > Notifier cpu_added_notifier;
> > } PIIX4PMState;
> >
> > @@ -628,61 +622,13 @@ static const MemoryRegionOps piix4_pci_ops = {
> > },
> > };
> >
> > -static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> > -{
> > - PIIX4PMState *s = opaque;
> > - CPUStatus *cpus = &s->gpe_cpu;
> > - uint64_t val = cpus->sts[addr];
> > -
> > - return val;
> > -}
> > -
> > -static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> > - unsigned int size)
> > -{
> > - /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> > -}
> > -
> > -static const MemoryRegionOps cpu_hotplug_ops = {
> > - .read = cpu_status_read,
> > - .write = cpu_status_write,
> > - .endianness = DEVICE_LITTLE_ENDIAN,
> > - .valid = {
> > - .min_access_size = 1,
> > - .max_access_size = 1,
> > - },
> > -};
> > -
> > -typedef enum {
> > - PLUG,
> > - UNPLUG,
> > -} HotplugEventType;
> > -
> > -static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
> > - HotplugEventType action)
> > -{
> > - CPUStatus *g = &s->gpe_cpu;
> > - ACPIGPE *gpe = &s->ar.gpe;
> > - CPUClass *k = CPU_GET_CLASS(cpu);
> > - int64_t cpu_id;
> > -
> > - assert(s != NULL);
> > -
> > - *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
> > - cpu_id = k->get_arch_id(CPU(cpu));
> > - if (action == PLUG) {
> > - g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> > - } else {
> > - g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
> > - }
> > - acpi_update_sci(&s->ar, s->irq);
> > -}
> > -
> > static void piix4_cpu_added_req(Notifier *n, void *opaque)
> > {
> > PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
> >
> > - piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
> > + assert(s != NULL);
> > + acpi_hotplug_cpu_add(&s->ar.gpe, &s->gpe_cpu, CPU(opaque));
> > + acpi_update_sci(&s->ar, s->irq);
> > }
> >
> > static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> > @@ -691,8 +637,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> > static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > PCIBus *bus, PIIX4PMState *s)
> > {
> > - CPUState *cpu;
> > -
> > memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> > "acpi-gpe0", GPE_LEN);
> > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> > @@ -703,16 +647,8 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > &s->io_pci);
> > pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
> >
> > - CPU_FOREACH(cpu) {
> > - CPUClass *cc = CPU_GET_CLASS(cpu);
> > - int64_t id = cc->get_arch_id(cpu);
> > -
> > - g_assert((id / 8) < PIIX4_PROC_LEN);
> > - s->gpe_cpu.sts[id / 8] |= (1 << (id % 8));
> > - }
> > - memory_region_init_io(&s->io_cpu, OBJECT(s), &cpu_hotplug_ops, s,
> > - "acpi-cpu-hotplug", PIIX4_PROC_LEN);
> > - memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
> > + acpi_hotplug_cpu_init(parent, OBJECT(s), &s->gpe_cpu,
> > + PIIX4_PROC_BASE);
> > s->cpu_added_notifier.notify = piix4_cpu_added_req;
> > qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> > }
> > diff --git a/include/hw/acpi/hotplug.h b/include/hw/acpi/hotplug.h
> > new file mode 100644
> > index 0000000..8bc176b
> > --- /dev/null
> > +++ b/include/hw/acpi/hotplug.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + * QEMU ACPI hotplug utilities
> > + *
> > + * Copyright (C) 2013 Red Hat Inc
> > + *
> > + * Authors:
> > + * Igor Mammedov <imammedo@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#ifndef ACPI_HOTPLUG_H
> > +#define ACPI_HOTPLUG_H
> > +
> > +#include "hw/acpi/acpi.h"
> > +
> > +#define ACPI_CPU_HOTPLUG_STATUS 4
> > +
> > +#define ACPI_GPE_PROC_LEN 32
> > +
> > +typedef struct AcpiCPUHotplug {
> > + MemoryRegion io;
> > + uint16_t io_base;
> > + uint8_t sts[ACPI_GPE_PROC_LEN];
> > +} AcpiCPUHotplug;
> > +
> > +void acpi_hotplug_cpu_add(ACPIGPE *gpe, AcpiCPUHotplug *g, CPUState *cpu);
> > +
> > +void acpi_hotplug_cpu_init(MemoryRegion *parent, Object *owner,
> > + AcpiCPUHotplug *gpe_cpu, uint16_t base);
> > +#endif
> > --
> > 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine
2013-12-19 14:18 ` Michael S. Tsirkin
@ 2013-12-19 15:17 ` Igor Mammedov
2013-12-19 15:33 ` Michael S. Tsirkin
0 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-19 15:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Thu, 19 Dec 2013 16:18:59 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 13, 2013 at 05:22:11PM +0100, Igor Mammedov wrote:
> > .. including readonly 'cpu-hotplug-io-base' property
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Is this documented in the Q35 spec? Could not find it there.
> If this is our PV code like PIIX, then we need to document this
> interface.
It's PV code and I need to update docs/specs/acpi_cpu_hotplug.txt
with Q35 io range.
I'll respin remained patches on top of PCI branch.
>
> > ---
> > hw/acpi/ich9.c | 27 +++++++++++++++++++++++++++
> > include/hw/acpi/ich9.h | 4 ++++
> > 2 files changed, 31 insertions(+)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 30f0df8..9c88109 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -44,6 +44,8 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
> > #define ICH9_DEBUG(fmt, ...) do { } while (0)
> > #endif
> >
> > +#define ICH9_PROC_BASE 0xa18
> > +
> > static void ich9_pm_update_sci_fn(ACPIREGS *regs)
> > {
> > ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
> > @@ -185,6 +187,15 @@ static void pm_powerdown_req(Notifier *n, void *opaque)
> > acpi_pm1_evt_power_down(&pm->acpi_regs);
> > }
> >
> > +static void ich9_cpu_added_req(Notifier *n, void *opaque)
> > +{
> > + ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
> > +
> > + assert(pm != NULL);
> > + acpi_hotplug_cpu_add(&pm->acpi_regs.gpe, &pm->gpe_cpu, CPU(opaque));
> > + acpi_update_sci(&pm->acpi_regs, pm->irq);
> > +}
> > +
> > void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > qemu_irq sci_irq)
> > {
> > @@ -210,6 +221,11 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > qemu_register_reset(pm_reset, pm);
> > pm->powerdown_notifier.notify = pm_powerdown_req;
> > qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> > +
> > + acpi_hotplug_cpu_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> > + &pm->gpe_cpu, pm->gpe_cpu.io_base);
> > + pm->cpu_added_notifier.notify = ich9_cpu_added_req;
> > + qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
> > }
> >
> > static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
> > @@ -222,6 +238,14 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
> > visit_type_uint32(v, &value, name, errp);
> > }
> >
> > +static void ich9_pm_get_cpu_io_base(Object *obj, Visitor *v, void *opaque,
> > + const char *name, Error **errp)
> > +{
> > + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > +
> > + visit_type_uint16(v, &s->pm.gpe_cpu.io_base, name, errp);
> > +}
> > +
> > void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> > {
> > static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> > @@ -233,4 +257,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> > NULL, NULL, pm, NULL);
> > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> > &gpe0_len, errp);
> > + pm->gpe_cpu.io_base = ICH9_PROC_BASE;
> > + object_property_add(obj, ACPI_CPU_HOTPLUG_IO_BASE_PROP, "int",
> > + ich9_pm_get_cpu_io_base, NULL, NULL, NULL, NULL);
> > }
> > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > index 82fcf9f..c71a9b4 100644
> > --- a/include/hw/acpi/ich9.h
> > +++ b/include/hw/acpi/ich9.h
> > @@ -22,6 +22,7 @@
> > #define HW_ACPI_ICH9_H
> >
> > #include "hw/acpi/acpi.h"
> > +#include "hw/acpi/hotplug.h"
> >
> > typedef struct ICH9LPCPMRegs {
> > /*
> > @@ -42,6 +43,9 @@ typedef struct ICH9LPCPMRegs {
> >
> > uint32_t pm_io_base;
> > Notifier powerdown_notifier;
> > +
> > + AcpiCPUHotplug gpe_cpu;
> > + Notifier cpu_added_notifier;
> > } ICH9LPCPMRegs;
> >
> > void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > --
> > 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine
2013-12-19 15:17 ` Igor Mammedov
@ 2013-12-19 15:33 ` Michael S. Tsirkin
0 siblings, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-19 15:33 UTC (permalink / raw)
To: Igor Mammedov
Cc: stefanha, hutao, qemu-devel, brogers, jjherne, kraxel, aliguori,
kaneshige.kenji, chen.fan.fnst, pbonzini, lersek
On Thu, Dec 19, 2013 at 04:17:08PM +0100, Igor Mammedov wrote:
> On Thu, 19 Dec 2013 16:18:59 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Fri, Dec 13, 2013 at 05:22:11PM +0100, Igor Mammedov wrote:
> > > .. including readonly 'cpu-hotplug-io-base' property
> > >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >
> > Is this documented in the Q35 spec? Could not find it there.
> > If this is our PV code like PIIX, then we need to document this
> > interface.
> It's PV code and I need to update docs/specs/acpi_cpu_hotplug.txt
> with Q35 io range.
BTW doesn't some system out there implement cpu hotplug?
If yes would have been nice to reuse that, though not a must.
> I'll respin remained patches on top of PCI branch.
>
> >
> > > ---
> > > hw/acpi/ich9.c | 27 +++++++++++++++++++++++++++
> > > include/hw/acpi/ich9.h | 4 ++++
> > > 2 files changed, 31 insertions(+)
> > >
> > > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > > index 30f0df8..9c88109 100644
> > > --- a/hw/acpi/ich9.c
> > > +++ b/hw/acpi/ich9.c
> > > @@ -44,6 +44,8 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
> > > #define ICH9_DEBUG(fmt, ...) do { } while (0)
> > > #endif
> > >
> > > +#define ICH9_PROC_BASE 0xa18
> > > +
> > > static void ich9_pm_update_sci_fn(ACPIREGS *regs)
> > > {
> > > ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
> > > @@ -185,6 +187,15 @@ static void pm_powerdown_req(Notifier *n, void *opaque)
> > > acpi_pm1_evt_power_down(&pm->acpi_regs);
> > > }
> > >
> > > +static void ich9_cpu_added_req(Notifier *n, void *opaque)
> > > +{
> > > + ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, cpu_added_notifier);
> > > +
> > > + assert(pm != NULL);
> > > + acpi_hotplug_cpu_add(&pm->acpi_regs.gpe, &pm->gpe_cpu, CPU(opaque));
> > > + acpi_update_sci(&pm->acpi_regs, pm->irq);
> > > +}
> > > +
> > > void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > > qemu_irq sci_irq)
> > > {
> > > @@ -210,6 +221,11 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > > qemu_register_reset(pm_reset, pm);
> > > pm->powerdown_notifier.notify = pm_powerdown_req;
> > > qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> > > +
> > > + acpi_hotplug_cpu_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> > > + &pm->gpe_cpu, pm->gpe_cpu.io_base);
> > > + pm->cpu_added_notifier.notify = ich9_cpu_added_req;
> > > + qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
> > > }
> > >
> > > static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
> > > @@ -222,6 +238,14 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
> > > visit_type_uint32(v, &value, name, errp);
> > > }
> > >
> > > +static void ich9_pm_get_cpu_io_base(Object *obj, Visitor *v, void *opaque,
> > > + const char *name, Error **errp)
> > > +{
> > > + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > > +
> > > + visit_type_uint16(v, &s->pm.gpe_cpu.io_base, name, errp);
> > > +}
> > > +
> > > void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> > > {
> > > static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> > > @@ -233,4 +257,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
> > > NULL, NULL, pm, NULL);
> > > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> > > &gpe0_len, errp);
> > > + pm->gpe_cpu.io_base = ICH9_PROC_BASE;
> > > + object_property_add(obj, ACPI_CPU_HOTPLUG_IO_BASE_PROP, "int",
> > > + ich9_pm_get_cpu_io_base, NULL, NULL, NULL, NULL);
> > > }
> > > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > > index 82fcf9f..c71a9b4 100644
> > > --- a/include/hw/acpi/ich9.h
> > > +++ b/include/hw/acpi/ich9.h
> > > @@ -22,6 +22,7 @@
> > > #define HW_ACPI_ICH9_H
> > >
> > > #include "hw/acpi/acpi.h"
> > > +#include "hw/acpi/hotplug.h"
> > >
> > > typedef struct ICH9LPCPMRegs {
> > > /*
> > > @@ -42,6 +43,9 @@ typedef struct ICH9LPCPMRegs {
> > >
> > > uint32_t pm_io_base;
> > > Notifier powerdown_notifier;
> > > +
> > > + AcpiCPUHotplug gpe_cpu;
> > > + Notifier cpu_added_notifier;
> > > } ICH9LPCPMRegs;
> > >
> > > void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > > --
> > > 1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-16 19:53 ` Michael S. Tsirkin
2013-12-16 22:15 ` Igor Mammedov
@ 2013-12-22 14:51 ` Igor Mammedov
2013-12-23 11:26 ` Michael S. Tsirkin
1 sibling, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-22 14:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aliguori, hutao, jjherne, qemu-devel, kraxel, stefanha,
kaneshige.kenji, chen.fan.fnst, pbonzini, brogers, lersek
On Mon, 16 Dec 2013 21:53:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > .. and report range used by it to OSPM via _CRS.
> > PRST is needed in SSDT since its base will depend on
> > chipset and will be dynamically set by QEMU.
> > Also move PRSC() method along with PRST since cross
> > table reference to PRST doesn't work.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > hw/i386/acpi-dsdt.dsl | 2 +-
> > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 68 insertions(+), 40 deletions(-)
> >
> > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > index 995b415..f26f81b 100644
> > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > @@ -20,6 +20,7 @@
> > Scope(\_SB) {
> > /* Objects filled in by run-time generated SSDT */
> > External(NTFY, MethodObj)
> > + External(\_SB.CPHD.PRSC, MethodObj)
> > External(CPON, PkgObj)
> >
> > /* Methods called by run-time generated SSDT Processor objects */
> > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > // _EJ0 method - eject callback
> > Sleep(200)
> > }
> > -
> > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > - PRS, 256
> > - }
> > - Method(PRSC, 0) {
> > - // Local5 = active cpu bitmap
> > - Store(PRS, Local5)
> > - // Local2 = last read byte from bitmap
> > - Store(Zero, Local2)
> > - // Local0 = Processor ID / APIC ID iterator
> > - Store(Zero, Local0)
> > - While (LLess(Local0, SizeOf(CPON))) {
> > - // Local1 = CPON flag for this cpu
> > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > - If (And(Local0, 0x07)) {
> > - // Shift down previously read bitmap byte
> > - ShiftRight(Local2, 1, Local2)
> > - } Else {
> > - // Read next byte from cpu bitmap
> > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > - }
> > - // Local3 = active state for this cpu
> > - Store(And(Local2, 1), Local3)
> > -
> > - If (LNotEqual(Local1, Local3)) {
> > - // State change - update CPON with new state
> > - Store(Local3, Index(CPON, Local0))
> > - // Do CPU notify
> > - If (LEqual(Local3, 1)) {
> > - NTFY(Local0, 1)
> > - } Else {
> > - NTFY(Local0, 3)
> > - }
> > - }
> > - Increment(Local0)
> > - }
> > - }
> > }
> > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > index 90efce0..fa9f2d4 100644
> > --- a/hw/i386/acpi-dsdt.dsl
> > +++ b/hw/i386/acpi-dsdt.dsl
> > @@ -311,7 +311,7 @@ DefinitionBlock (
> > }
> > Method(_E02) {
> > // CPU hotplug event
> > - \_SB.PRSC()
> > + \_SB.CPHD.PRSC()
> > }
> > Method(_L03) {
> > }
> > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > index 22baa58..9ccc543 100644
> > --- a/hw/i386/q35-acpi-dsdt.dsl
> > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > @@ -420,7 +420,7 @@ DefinitionBlock (
> > }
> > Method(_E02) {
> > // CPU hotplug event
> > - \_SB.PRSC()
> > + \_SB.CPHD.PRSC()
> > }
> > Method(_L03) {
> > }
> > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > index a4484b8..ec8893c 100644
> > --- a/hw/i386/ssdt-misc.dsl
> > +++ b/hw/i386/ssdt-misc.dsl
> > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > }
> > }
> > }
> > + Scope(\_SB) {
> > + External(NTFY, MethodObj)
> > + External(CPON, PkgObj)
> > +
> > + Device(CPHD) {
> > + Name(_HID, EISAID("PNP0C08"))
> > + Name(CPPL, 32) // cpu-gpe length
> > + Name(CPHP, 0xaf00)
> > +
> > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > + PRS, 256
> > + }
> > +
> > + Method(PRSC, 0) {
> > + // Local5 = active cpu bitmap
> > + Store(PRS, Local5)
> > + // Local2 = last read byte from bitmap
> > + Store(Zero, Local2)
> > + // Local0 = Processor ID / APIC ID iterator
> > + Store(Zero, Local0)
> > + While (LLess(Local0, SizeOf(CPON))) {
> > + // Local1 = CPON flag for this cpu
> > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > + If (And(Local0, 0x07)) {
> > + // Shift down previously read bitmap byte
> > + ShiftRight(Local2, 1, Local2)
> > + } Else {
> > + // Read next byte from cpu bitmap
> > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > + }
> > + // Local3 = active state for this cpu
> > + Store(And(Local2, 1), Local3)
> > +
> > + If (LNotEqual(Local1, Local3)) {
> > + // State change - update CPON with new state
> > + Store(Local3, Index(CPON, Local0))
> > + // Do CPU notify
> > + If (LEqual(Local3, 1)) {
> > + NTFY(Local0, 1)
> > + } Else {
> > + NTFY(Local0, 3)
> > + }
> > + }
> > + Increment(Local0)
> > + }
> > + }
> > +
> > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > + Name(_STA, 0xA)
>
> This shared the problem you yourself pointed out with
> patch 'pc: ACPI BIOS: implement memory hotplug':
> if we make device non present ospm can ignore our _CRS.
>
> Can't we get a better handle on why windows crashes?
Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or
ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack:
: nt!PnpBootDeviceWait+0x181
: nt!IopInitializeBootDrivers+0xbd3
: nt!IoInitSystem+0xbb6
: nt!Phase1InitializationDiscard+0x16f6
: nt!Phase1Initialization+0x13
: nt!PspSystemThreadStartup+0x23d
: nt!KiStartSystemThread+0x16
However it works fine if containing device is PNP0C02 "Motherboard registers",
so we probably should use it instead.
>
>
> > +
> > + Method(_CRS, 0) {
> > + Store(ResourceTemplate() {
> > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > + }, Local0)
> > +
> > + CreateWordField(Local0, IO._MIN, IOMN)
> > + CreateWordField(Local0, IO._MAX, IOMX)
> > +
> > + Store(CPHP, IOMN)
> > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > + Return(Local0)
> > + }
> > + } // Device(CPHD)
> > + } // Scope(\_SB)
> > }
> > --
> > 1.8.3.1
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-22 14:51 ` Igor Mammedov
@ 2013-12-23 11:26 ` Michael S. Tsirkin
2013-12-23 13:06 ` Igor Mammedov
0 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-23 11:26 UTC (permalink / raw)
To: Igor Mammedov
Cc: aliguori, hutao, jjherne, qemu-devel, kraxel, stefanha,
kaneshige.kenji, chen.fan.fnst, pbonzini, brogers, lersek
On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote:
> On Mon, 16 Dec 2013 21:53:07 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > > .. and report range used by it to OSPM via _CRS.
> > > PRST is needed in SSDT since its base will depend on
> > > chipset and will be dynamically set by QEMU.
> > > Also move PRSC() method along with PRST since cross
> > > table reference to PRST doesn't work.
> > >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > > hw/i386/acpi-dsdt.dsl | 2 +-
> > > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 68 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > index 995b415..f26f81b 100644
> > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > @@ -20,6 +20,7 @@
> > > Scope(\_SB) {
> > > /* Objects filled in by run-time generated SSDT */
> > > External(NTFY, MethodObj)
> > > + External(\_SB.CPHD.PRSC, MethodObj)
> > > External(CPON, PkgObj)
> > >
> > > /* Methods called by run-time generated SSDT Processor objects */
> > > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > > // _EJ0 method - eject callback
> > > Sleep(200)
> > > }
> > > -
> > > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > > - PRS, 256
> > > - }
> > > - Method(PRSC, 0) {
> > > - // Local5 = active cpu bitmap
> > > - Store(PRS, Local5)
> > > - // Local2 = last read byte from bitmap
> > > - Store(Zero, Local2)
> > > - // Local0 = Processor ID / APIC ID iterator
> > > - Store(Zero, Local0)
> > > - While (LLess(Local0, SizeOf(CPON))) {
> > > - // Local1 = CPON flag for this cpu
> > > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > > - If (And(Local0, 0x07)) {
> > > - // Shift down previously read bitmap byte
> > > - ShiftRight(Local2, 1, Local2)
> > > - } Else {
> > > - // Read next byte from cpu bitmap
> > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > - }
> > > - // Local3 = active state for this cpu
> > > - Store(And(Local2, 1), Local3)
> > > -
> > > - If (LNotEqual(Local1, Local3)) {
> > > - // State change - update CPON with new state
> > > - Store(Local3, Index(CPON, Local0))
> > > - // Do CPU notify
> > > - If (LEqual(Local3, 1)) {
> > > - NTFY(Local0, 1)
> > > - } Else {
> > > - NTFY(Local0, 3)
> > > - }
> > > - }
> > > - Increment(Local0)
> > > - }
> > > - }
> > > }
> > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > > index 90efce0..fa9f2d4 100644
> > > --- a/hw/i386/acpi-dsdt.dsl
> > > +++ b/hw/i386/acpi-dsdt.dsl
> > > @@ -311,7 +311,7 @@ DefinitionBlock (
> > > }
> > > Method(_E02) {
> > > // CPU hotplug event
> > > - \_SB.PRSC()
> > > + \_SB.CPHD.PRSC()
> > > }
> > > Method(_L03) {
> > > }
> > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > > index 22baa58..9ccc543 100644
> > > --- a/hw/i386/q35-acpi-dsdt.dsl
> > > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > > @@ -420,7 +420,7 @@ DefinitionBlock (
> > > }
> > > Method(_E02) {
> > > // CPU hotplug event
> > > - \_SB.PRSC()
> > > + \_SB.CPHD.PRSC()
> > > }
> > > Method(_L03) {
> > > }
> > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > > index a4484b8..ec8893c 100644
> > > --- a/hw/i386/ssdt-misc.dsl
> > > +++ b/hw/i386/ssdt-misc.dsl
> > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > > }
> > > }
> > > }
> > > + Scope(\_SB) {
> > > + External(NTFY, MethodObj)
> > > + External(CPON, PkgObj)
> > > +
> > > + Device(CPHD) {
> > > + Name(_HID, EISAID("PNP0C08"))
> > > + Name(CPPL, 32) // cpu-gpe length
> > > + Name(CPHP, 0xaf00)
> > > +
> > > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > > + PRS, 256
> > > + }
> > > +
> > > + Method(PRSC, 0) {
> > > + // Local5 = active cpu bitmap
> > > + Store(PRS, Local5)
> > > + // Local2 = last read byte from bitmap
> > > + Store(Zero, Local2)
> > > + // Local0 = Processor ID / APIC ID iterator
> > > + Store(Zero, Local0)
> > > + While (LLess(Local0, SizeOf(CPON))) {
> > > + // Local1 = CPON flag for this cpu
> > > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > > + If (And(Local0, 0x07)) {
> > > + // Shift down previously read bitmap byte
> > > + ShiftRight(Local2, 1, Local2)
> > > + } Else {
> > > + // Read next byte from cpu bitmap
> > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > + }
> > > + // Local3 = active state for this cpu
> > > + Store(And(Local2, 1), Local3)
> > > +
> > > + If (LNotEqual(Local1, Local3)) {
> > > + // State change - update CPON with new state
> > > + Store(Local3, Index(CPON, Local0))
> > > + // Do CPU notify
> > > + If (LEqual(Local3, 1)) {
> > > + NTFY(Local0, 1)
> > > + } Else {
> > > + NTFY(Local0, 3)
> > > + }
> > > + }
> > > + Increment(Local0)
> > > + }
> > > + }
> > > +
> > > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > > + Name(_STA, 0xA)
> >
> > This shared the problem you yourself pointed out with
> > patch 'pc: ACPI BIOS: implement memory hotplug':
> > if we make device non present ospm can ignore our _CRS.
> >
> > Can't we get a better handle on why windows crashes?
> Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or
> ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack:
> : nt!PnpBootDeviceWait+0x181
> : nt!IopInitializeBootDrivers+0xbd3
> : nt!IoInitSystem+0xbb6
> : nt!Phase1InitializationDiscard+0x16f6
> : nt!Phase1Initialization+0x13
> : nt!PspSystemThreadStartup+0x23d
> : nt!KiStartSystemThread+0x16
Interesting. This seems to imply that it can't access
IO port for the disk. Which boot disk type are you using?
Is the _CRS resource overlapping any other resource
by any chance?
>
> However it works fine if containing device is PNP0C02 "Motherboard registers",
> so we probably should use it instead.
>
>
> >
> >
> > > +
> > > + Method(_CRS, 0) {
> > > + Store(ResourceTemplate() {
> > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > > + }, Local0)
> > > +
> > > + CreateWordField(Local0, IO._MIN, IOMN)
> > > + CreateWordField(Local0, IO._MAX, IOMX)
> > > +
> > > + Store(CPHP, IOMN)
> > > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > > + Return(Local0)
> > > + }
> > > + } // Device(CPHD)
> > > + } // Scope(\_SB)
> > > }
> > > --
> > > 1.8.3.1
> >
>
>
> --
> Regards,
> Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-23 11:26 ` Michael S. Tsirkin
@ 2013-12-23 13:06 ` Igor Mammedov
2013-12-23 14:48 ` Michael S. Tsirkin
0 siblings, 1 reply; 40+ messages in thread
From: Igor Mammedov @ 2013-12-23 13:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aliguori, hutao, jjherne, qemu-devel, kraxel, stefanha,
kaneshige.kenji, chen.fan.fnst, pbonzini, brogers, lersek
On Mon, 23 Dec 2013 13:26:37 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote:
> > On Mon, 16 Dec 2013 21:53:07 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > > > .. and report range used by it to OSPM via _CRS.
> > > > PRST is needed in SSDT since its base will depend on
> > > > chipset and will be dynamically set by QEMU.
> > > > Also move PRSC() method along with PRST since cross
> > > > table reference to PRST doesn't work.
> > > >
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > > > hw/i386/acpi-dsdt.dsl | 2 +-
> > > > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 68 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > index 995b415..f26f81b 100644
> > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > @@ -20,6 +20,7 @@
> > > > Scope(\_SB) {
> > > > /* Objects filled in by run-time generated SSDT */
> > > > External(NTFY, MethodObj)
> > > > + External(\_SB.CPHD.PRSC, MethodObj)
> > > > External(CPON, PkgObj)
> > > >
> > > > /* Methods called by run-time generated SSDT Processor objects */
> > > > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > > > // _EJ0 method - eject callback
> > > > Sleep(200)
> > > > }
> > > > -
> > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > > > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > - PRS, 256
> > > > - }
> > > > - Method(PRSC, 0) {
> > > > - // Local5 = active cpu bitmap
> > > > - Store(PRS, Local5)
> > > > - // Local2 = last read byte from bitmap
> > > > - Store(Zero, Local2)
> > > > - // Local0 = Processor ID / APIC ID iterator
> > > > - Store(Zero, Local0)
> > > > - While (LLess(Local0, SizeOf(CPON))) {
> > > > - // Local1 = CPON flag for this cpu
> > > > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > - If (And(Local0, 0x07)) {
> > > > - // Shift down previously read bitmap byte
> > > > - ShiftRight(Local2, 1, Local2)
> > > > - } Else {
> > > > - // Read next byte from cpu bitmap
> > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > - }
> > > > - // Local3 = active state for this cpu
> > > > - Store(And(Local2, 1), Local3)
> > > > -
> > > > - If (LNotEqual(Local1, Local3)) {
> > > > - // State change - update CPON with new state
> > > > - Store(Local3, Index(CPON, Local0))
> > > > - // Do CPU notify
> > > > - If (LEqual(Local3, 1)) {
> > > > - NTFY(Local0, 1)
> > > > - } Else {
> > > > - NTFY(Local0, 3)
> > > > - }
> > > > - }
> > > > - Increment(Local0)
> > > > - }
> > > > - }
> > > > }
> > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > > > index 90efce0..fa9f2d4 100644
> > > > --- a/hw/i386/acpi-dsdt.dsl
> > > > +++ b/hw/i386/acpi-dsdt.dsl
> > > > @@ -311,7 +311,7 @@ DefinitionBlock (
> > > > }
> > > > Method(_E02) {
> > > > // CPU hotplug event
> > > > - \_SB.PRSC()
> > > > + \_SB.CPHD.PRSC()
> > > > }
> > > > Method(_L03) {
> > > > }
> > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > > > index 22baa58..9ccc543 100644
> > > > --- a/hw/i386/q35-acpi-dsdt.dsl
> > > > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > > > @@ -420,7 +420,7 @@ DefinitionBlock (
> > > > }
> > > > Method(_E02) {
> > > > // CPU hotplug event
> > > > - \_SB.PRSC()
> > > > + \_SB.CPHD.PRSC()
> > > > }
> > > > Method(_L03) {
> > > > }
> > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > > > index a4484b8..ec8893c 100644
> > > > --- a/hw/i386/ssdt-misc.dsl
> > > > +++ b/hw/i386/ssdt-misc.dsl
> > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > > > }
> > > > }
> > > > }
> > > > + Scope(\_SB) {
> > > > + External(NTFY, MethodObj)
> > > > + External(CPON, PkgObj)
> > > > +
> > > > + Device(CPHD) {
> > > > + Name(_HID, EISAID("PNP0C08"))
> > > > + Name(CPPL, 32) // cpu-gpe length
> > > > + Name(CPHP, 0xaf00)
> > > > +
> > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > > > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > + PRS, 256
> > > > + }
> > > > +
> > > > + Method(PRSC, 0) {
> > > > + // Local5 = active cpu bitmap
> > > > + Store(PRS, Local5)
> > > > + // Local2 = last read byte from bitmap
> > > > + Store(Zero, Local2)
> > > > + // Local0 = Processor ID / APIC ID iterator
> > > > + Store(Zero, Local0)
> > > > + While (LLess(Local0, SizeOf(CPON))) {
> > > > + // Local1 = CPON flag for this cpu
> > > > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > + If (And(Local0, 0x07)) {
> > > > + // Shift down previously read bitmap byte
> > > > + ShiftRight(Local2, 1, Local2)
> > > > + } Else {
> > > > + // Read next byte from cpu bitmap
> > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > + }
> > > > + // Local3 = active state for this cpu
> > > > + Store(And(Local2, 1), Local3)
> > > > +
> > > > + If (LNotEqual(Local1, Local3)) {
> > > > + // State change - update CPON with new state
> > > > + Store(Local3, Index(CPON, Local0))
> > > > + // Do CPU notify
> > > > + If (LEqual(Local3, 1)) {
> > > > + NTFY(Local0, 1)
> > > > + } Else {
> > > > + NTFY(Local0, 3)
> > > > + }
> > > > + }
> > > > + Increment(Local0)
> > > > + }
> > > > + }
> > > > +
> > > > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > > > + Name(_STA, 0xA)
> > >
> > > This shared the problem you yourself pointed out with
> > > patch 'pc: ACPI BIOS: implement memory hotplug':
> > > if we make device non present ospm can ignore our _CRS.
> > >
> > > Can't we get a better handle on why windows crashes?
> > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or
> > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack:
> > : nt!PnpBootDeviceWait+0x181
> > : nt!IopInitializeBootDrivers+0xbd3
> > : nt!IoInitSystem+0xbb6
> > : nt!Phase1InitializationDiscard+0x16f6
> > : nt!Phase1Initialization+0x13
> > : nt!PspSystemThreadStartup+0x23d
> > : nt!KiStartSystemThread+0x16
>
>
> Interesting. This seems to imply that it can't access
> IO port for the disk. Which boot disk type are you using?
> Is the _CRS resource overlapping any other resource
> by any chance?
Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus.
PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and
basically take all io ports except of 0xcf8-0xcff hole.
Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already
covered by bus CRS, the same applies to PCI hotplug as well.
So I was doing it wrong trying advertise bus resources out of bus scope.
What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO
ports CRS to PCI bus. Question is what PNP IDs they should use?
It looks pretty much out of scope of cpu_hotplug and should be done for
pci hotplug as well. So I'm ditching ACPI device and CRS parts from this
series as not directly related.
Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably
for all resources consumed by it to make it right.
>
> >
> > However it works fine if containing device is PNP0C02 "Motherboard registers",
> > so we probably should use it instead.
> >
> >
> > >
> > >
> > > > +
> > > > + Method(_CRS, 0) {
> > > > + Store(ResourceTemplate() {
> > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > > > + }, Local0)
> > > > +
> > > > + CreateWordField(Local0, IO._MIN, IOMN)
> > > > + CreateWordField(Local0, IO._MAX, IOMX)
> > > > +
> > > > + Store(CPHP, IOMN)
> > > > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > > > + Return(Local0)
> > > > + }
> > > > + } // Device(CPHD)
> > > > + } // Scope(\_SB)
> > > > }
> > > > --
> > > > 1.8.3.1
> > >
> >
> >
> > --
> > Regards,
> > Igor
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-23 13:06 ` Igor Mammedov
@ 2013-12-23 14:48 ` Michael S. Tsirkin
2013-12-23 16:24 ` Igor Mammedov
0 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-23 14:48 UTC (permalink / raw)
To: Igor Mammedov
Cc: aliguori, hutao, jjherne, qemu-devel, kraxel, stefanha,
kaneshige.kenji, chen.fan.fnst, pbonzini, brogers, lersek
On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote:
> On Mon, 23 Dec 2013 13:26:37 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote:
> > > On Mon, 16 Dec 2013 21:53:07 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > > > > .. and report range used by it to OSPM via _CRS.
> > > > > PRST is needed in SSDT since its base will depend on
> > > > > chipset and will be dynamically set by QEMU.
> > > > > Also move PRSC() method along with PRST since cross
> > > > > table reference to PRST doesn't work.
> > > > >
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > > > > hw/i386/acpi-dsdt.dsl | 2 +-
> > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > > > > 4 files changed, 68 insertions(+), 40 deletions(-)
> > > > >
> > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > index 995b415..f26f81b 100644
> > > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > @@ -20,6 +20,7 @@
> > > > > Scope(\_SB) {
> > > > > /* Objects filled in by run-time generated SSDT */
> > > > > External(NTFY, MethodObj)
> > > > > + External(\_SB.CPHD.PRSC, MethodObj)
> > > > > External(CPON, PkgObj)
> > > > >
> > > > > /* Methods called by run-time generated SSDT Processor objects */
> > > > > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > > > > // _EJ0 method - eject callback
> > > > > Sleep(200)
> > > > > }
> > > > > -
> > > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > > > > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > > - PRS, 256
> > > > > - }
> > > > > - Method(PRSC, 0) {
> > > > > - // Local5 = active cpu bitmap
> > > > > - Store(PRS, Local5)
> > > > > - // Local2 = last read byte from bitmap
> > > > > - Store(Zero, Local2)
> > > > > - // Local0 = Processor ID / APIC ID iterator
> > > > > - Store(Zero, Local0)
> > > > > - While (LLess(Local0, SizeOf(CPON))) {
> > > > > - // Local1 = CPON flag for this cpu
> > > > > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > > - If (And(Local0, 0x07)) {
> > > > > - // Shift down previously read bitmap byte
> > > > > - ShiftRight(Local2, 1, Local2)
> > > > > - } Else {
> > > > > - // Read next byte from cpu bitmap
> > > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > > - }
> > > > > - // Local3 = active state for this cpu
> > > > > - Store(And(Local2, 1), Local3)
> > > > > -
> > > > > - If (LNotEqual(Local1, Local3)) {
> > > > > - // State change - update CPON with new state
> > > > > - Store(Local3, Index(CPON, Local0))
> > > > > - // Do CPU notify
> > > > > - If (LEqual(Local3, 1)) {
> > > > > - NTFY(Local0, 1)
> > > > > - } Else {
> > > > > - NTFY(Local0, 3)
> > > > > - }
> > > > > - }
> > > > > - Increment(Local0)
> > > > > - }
> > > > > - }
> > > > > }
> > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > > > > index 90efce0..fa9f2d4 100644
> > > > > --- a/hw/i386/acpi-dsdt.dsl
> > > > > +++ b/hw/i386/acpi-dsdt.dsl
> > > > > @@ -311,7 +311,7 @@ DefinitionBlock (
> > > > > }
> > > > > Method(_E02) {
> > > > > // CPU hotplug event
> > > > > - \_SB.PRSC()
> > > > > + \_SB.CPHD.PRSC()
> > > > > }
> > > > > Method(_L03) {
> > > > > }
> > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > > > > index 22baa58..9ccc543 100644
> > > > > --- a/hw/i386/q35-acpi-dsdt.dsl
> > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > > > > @@ -420,7 +420,7 @@ DefinitionBlock (
> > > > > }
> > > > > Method(_E02) {
> > > > > // CPU hotplug event
> > > > > - \_SB.PRSC()
> > > > > + \_SB.CPHD.PRSC()
> > > > > }
> > > > > Method(_L03) {
> > > > > }
> > > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > > > > index a4484b8..ec8893c 100644
> > > > > --- a/hw/i386/ssdt-misc.dsl
> > > > > +++ b/hw/i386/ssdt-misc.dsl
> > > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > > > > }
> > > > > }
> > > > > }
> > > > > + Scope(\_SB) {
> > > > > + External(NTFY, MethodObj)
> > > > > + External(CPON, PkgObj)
> > > > > +
> > > > > + Device(CPHD) {
> > > > > + Name(_HID, EISAID("PNP0C08"))
> > > > > + Name(CPPL, 32) // cpu-gpe length
> > > > > + Name(CPHP, 0xaf00)
> > > > > +
> > > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > > > > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > > + PRS, 256
> > > > > + }
> > > > > +
> > > > > + Method(PRSC, 0) {
> > > > > + // Local5 = active cpu bitmap
> > > > > + Store(PRS, Local5)
> > > > > + // Local2 = last read byte from bitmap
> > > > > + Store(Zero, Local2)
> > > > > + // Local0 = Processor ID / APIC ID iterator
> > > > > + Store(Zero, Local0)
> > > > > + While (LLess(Local0, SizeOf(CPON))) {
> > > > > + // Local1 = CPON flag for this cpu
> > > > > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > > + If (And(Local0, 0x07)) {
> > > > > + // Shift down previously read bitmap byte
> > > > > + ShiftRight(Local2, 1, Local2)
> > > > > + } Else {
> > > > > + // Read next byte from cpu bitmap
> > > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > > + }
> > > > > + // Local3 = active state for this cpu
> > > > > + Store(And(Local2, 1), Local3)
> > > > > +
> > > > > + If (LNotEqual(Local1, Local3)) {
> > > > > + // State change - update CPON with new state
> > > > > + Store(Local3, Index(CPON, Local0))
> > > > > + // Do CPU notify
> > > > > + If (LEqual(Local3, 1)) {
> > > > > + NTFY(Local0, 1)
> > > > > + } Else {
> > > > > + NTFY(Local0, 3)
> > > > > + }
> > > > > + }
> > > > > + Increment(Local0)
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > > > > + Name(_STA, 0xA)
> > > >
> > > > This shared the problem you yourself pointed out with
> > > > patch 'pc: ACPI BIOS: implement memory hotplug':
> > > > if we make device non present ospm can ignore our _CRS.
> > > >
> > > > Can't we get a better handle on why windows crashes?
> > > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or
> > > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack:
> > > : nt!PnpBootDeviceWait+0x181
> > > : nt!IopInitializeBootDrivers+0xbd3
> > > : nt!IoInitSystem+0xbb6
> > > : nt!Phase1InitializationDiscard+0x16f6
> > > : nt!Phase1Initialization+0x13
> > > : nt!PspSystemThreadStartup+0x23d
> > > : nt!KiStartSystemThread+0x16
> >
> >
> > Interesting. This seems to imply that it can't access
> > IO port for the disk. Which boot disk type are you using?
> > Is the _CRS resource overlapping any other resource
> > by any chance?
> Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus.
> PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and
> basically take all io ports except of 0xcf8-0xcff hole.
> Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already
> covered by bus CRS, the same applies to PCI hotplug as well.
> So I was doing it wrong trying advertise bus resources out of bus scope.
Yes, that's explicitly prohibited by the firmware specification.
> What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO
> ports CRS to PCI bus. Question is what PNP IDs they should use?
>
> It looks pretty much out of scope of cpu_hotplug and should be done for
> pci hotplug as well. So I'm ditching ACPI device and CRS parts from this
> series as not directly related.
> Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably
> for all resources consumed by it to make it right.
Could be ok but are we using any new ports?
If yes then I think before doing that we should make sure _CRS for
the host bridge does not include them, or consume them
in some child device.
Otherwise we increase the chance of a conflict in case some
guest uses that port.
>
> >
> > >
> > > However it works fine if containing device is PNP0C02 "Motherboard registers",
> > > so we probably should use it instead.
> > >
> > >
> > > >
> > > >
> > > > > +
> > > > > + Method(_CRS, 0) {
> > > > > + Store(ResourceTemplate() {
> > > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > > > > + }, Local0)
> > > > > +
> > > > > + CreateWordField(Local0, IO._MIN, IOMN)
> > > > > + CreateWordField(Local0, IO._MAX, IOMX)
> > > > > +
> > > > > + Store(CPHP, IOMN)
> > > > > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > > > > + Return(Local0)
> > > > > + }
> > > > > + } // Device(CPHD)
> > > > > + } // Scope(\_SB)
> > > > > }
> > > > > --
> > > > > 1.8.3.1
> > > >
> > >
> > >
> > > --
> > > Regards,
> > > Igor
>
>
> --
> Regards,
> Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-23 14:48 ` Michael S. Tsirkin
@ 2013-12-23 16:24 ` Igor Mammedov
2013-12-23 16:52 ` Laszlo Ersek
2013-12-23 16:55 ` Michael S. Tsirkin
0 siblings, 2 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-23 16:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aliguori, hutao, jjherne, qemu-devel, kraxel, stefanha,
kaneshige.kenji, chen.fan.fnst, pbonzini, brogers, lersek
On Mon, 23 Dec 2013 16:48:49 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote:
> > On Mon, 23 Dec 2013 13:26:37 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote:
> > > > On Mon, 16 Dec 2013 21:53:07 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > > > > > .. and report range used by it to OSPM via _CRS.
> > > > > > PRST is needed in SSDT since its base will depend on
> > > > > > chipset and will be dynamically set by QEMU.
> > > > > > Also move PRSC() method along with PRST since cross
> > > > > > table reference to PRST doesn't work.
> > > > > >
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > ---
> > > > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > > > > > hw/i386/acpi-dsdt.dsl | 2 +-
> > > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > > > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > > > > > 4 files changed, 68 insertions(+), 40 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > > index 995b415..f26f81b 100644
> > > > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > > @@ -20,6 +20,7 @@
> > > > > > Scope(\_SB) {
> > > > > > /* Objects filled in by run-time generated SSDT */
> > > > > > External(NTFY, MethodObj)
> > > > > > + External(\_SB.CPHD.PRSC, MethodObj)
> > > > > > External(CPON, PkgObj)
> > > > > >
> > > > > > /* Methods called by run-time generated SSDT Processor objects */
> > > > > > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > > > > > // _EJ0 method - eject callback
> > > > > > Sleep(200)
> > > > > > }
> > > > > > -
> > > > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > > > > > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > > > - PRS, 256
> > > > > > - }
> > > > > > - Method(PRSC, 0) {
> > > > > > - // Local5 = active cpu bitmap
> > > > > > - Store(PRS, Local5)
> > > > > > - // Local2 = last read byte from bitmap
> > > > > > - Store(Zero, Local2)
> > > > > > - // Local0 = Processor ID / APIC ID iterator
> > > > > > - Store(Zero, Local0)
> > > > > > - While (LLess(Local0, SizeOf(CPON))) {
> > > > > > - // Local1 = CPON flag for this cpu
> > > > > > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > > > - If (And(Local0, 0x07)) {
> > > > > > - // Shift down previously read bitmap byte
> > > > > > - ShiftRight(Local2, 1, Local2)
> > > > > > - } Else {
> > > > > > - // Read next byte from cpu bitmap
> > > > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > > > - }
> > > > > > - // Local3 = active state for this cpu
> > > > > > - Store(And(Local2, 1), Local3)
> > > > > > -
> > > > > > - If (LNotEqual(Local1, Local3)) {
> > > > > > - // State change - update CPON with new state
> > > > > > - Store(Local3, Index(CPON, Local0))
> > > > > > - // Do CPU notify
> > > > > > - If (LEqual(Local3, 1)) {
> > > > > > - NTFY(Local0, 1)
> > > > > > - } Else {
> > > > > > - NTFY(Local0, 3)
> > > > > > - }
> > > > > > - }
> > > > > > - Increment(Local0)
> > > > > > - }
> > > > > > - }
> > > > > > }
> > > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > > > > > index 90efce0..fa9f2d4 100644
> > > > > > --- a/hw/i386/acpi-dsdt.dsl
> > > > > > +++ b/hw/i386/acpi-dsdt.dsl
> > > > > > @@ -311,7 +311,7 @@ DefinitionBlock (
> > > > > > }
> > > > > > Method(_E02) {
> > > > > > // CPU hotplug event
> > > > > > - \_SB.PRSC()
> > > > > > + \_SB.CPHD.PRSC()
> > > > > > }
> > > > > > Method(_L03) {
> > > > > > }
> > > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > > > > > index 22baa58..9ccc543 100644
> > > > > > --- a/hw/i386/q35-acpi-dsdt.dsl
> > > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > > > > > @@ -420,7 +420,7 @@ DefinitionBlock (
> > > > > > }
> > > > > > Method(_E02) {
> > > > > > // CPU hotplug event
> > > > > > - \_SB.PRSC()
> > > > > > + \_SB.CPHD.PRSC()
> > > > > > }
> > > > > > Method(_L03) {
> > > > > > }
> > > > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > > > > > index a4484b8..ec8893c 100644
> > > > > > --- a/hw/i386/ssdt-misc.dsl
> > > > > > +++ b/hw/i386/ssdt-misc.dsl
> > > > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > > > > > }
> > > > > > }
> > > > > > }
> > > > > > + Scope(\_SB) {
> > > > > > + External(NTFY, MethodObj)
> > > > > > + External(CPON, PkgObj)
> > > > > > +
> > > > > > + Device(CPHD) {
> > > > > > + Name(_HID, EISAID("PNP0C08"))
> > > > > > + Name(CPPL, 32) // cpu-gpe length
> > > > > > + Name(CPHP, 0xaf00)
> > > > > > +
> > > > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > > > > > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > > > + PRS, 256
> > > > > > + }
> > > > > > +
> > > > > > + Method(PRSC, 0) {
> > > > > > + // Local5 = active cpu bitmap
> > > > > > + Store(PRS, Local5)
> > > > > > + // Local2 = last read byte from bitmap
> > > > > > + Store(Zero, Local2)
> > > > > > + // Local0 = Processor ID / APIC ID iterator
> > > > > > + Store(Zero, Local0)
> > > > > > + While (LLess(Local0, SizeOf(CPON))) {
> > > > > > + // Local1 = CPON flag for this cpu
> > > > > > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > > > + If (And(Local0, 0x07)) {
> > > > > > + // Shift down previously read bitmap byte
> > > > > > + ShiftRight(Local2, 1, Local2)
> > > > > > + } Else {
> > > > > > + // Read next byte from cpu bitmap
> > > > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > > > + }
> > > > > > + // Local3 = active state for this cpu
> > > > > > + Store(And(Local2, 1), Local3)
> > > > > > +
> > > > > > + If (LNotEqual(Local1, Local3)) {
> > > > > > + // State change - update CPON with new state
> > > > > > + Store(Local3, Index(CPON, Local0))
> > > > > > + // Do CPU notify
> > > > > > + If (LEqual(Local3, 1)) {
> > > > > > + NTFY(Local0, 1)
> > > > > > + } Else {
> > > > > > + NTFY(Local0, 3)
> > > > > > + }
> > > > > > + }
> > > > > > + Increment(Local0)
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > > > > > + Name(_STA, 0xA)
> > > > >
> > > > > This shared the problem you yourself pointed out with
> > > > > patch 'pc: ACPI BIOS: implement memory hotplug':
> > > > > if we make device non present ospm can ignore our _CRS.
> > > > >
> > > > > Can't we get a better handle on why windows crashes?
> > > > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or
> > > > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack:
> > > > : nt!PnpBootDeviceWait+0x181
> > > > : nt!IopInitializeBootDrivers+0xbd3
> > > > : nt!IoInitSystem+0xbb6
> > > > : nt!Phase1InitializationDiscard+0x16f6
> > > > : nt!Phase1Initialization+0x13
> > > > : nt!PspSystemThreadStartup+0x23d
> > > > : nt!KiStartSystemThread+0x16
> > >
> > >
> > > Interesting. This seems to imply that it can't access
> > > IO port for the disk. Which boot disk type are you using?
> > > Is the _CRS resource overlapping any other resource
> > > by any chance?
> > Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus.
> > PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and
> > basically take all io ports except of 0xcf8-0xcff hole.
> > Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already
> > covered by bus CRS, the same applies to PCI hotplug as well.
> > So I was doing it wrong trying advertise bus resources out of bus scope.
>
> Yes, that's explicitly prohibited by the firmware specification.
>
> > What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO
> > ports CRS to PCI bus. Question is what PNP IDs they should use?
> >
> > It looks pretty much out of scope of cpu_hotplug and should be done for
> > pci hotplug as well. So I'm ditching ACPI device and CRS parts from this
> > series as not directly related.
> > Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably
> > for all resources consumed by it to make it right.
>
> Could be ok but are we using any new ports?
yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not
to use 0xaf00-0xaf1f.
> If yes then I think before doing that we should make sure _CRS for
> the host bridge does not include them, or consume them
I'm fine with making holes in PCI bus CRES template, I can do it for
pci-hotplug as well while at it.
> in some child device.
that would be cleanest, but is there any suggestion what device(s) it would be
for piix and ich9-lpc, i.e. which PNP IDs should we use?
> Otherwise we increase the chance of a conflict in case some
> guest uses that port.
>
> >
> > >
> > > >
> > > > However it works fine if containing device is PNP0C02 "Motherboard registers",
> > > > so we probably should use it instead.
> > > >
> > > >
> > > > >
> > > > >
> > > > > > +
> > > > > > + Method(_CRS, 0) {
> > > > > > + Store(ResourceTemplate() {
> > > > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > > > > > + }, Local0)
> > > > > > +
> > > > > > + CreateWordField(Local0, IO._MIN, IOMN)
> > > > > > + CreateWordField(Local0, IO._MAX, IOMX)
> > > > > > +
> > > > > > + Store(CPHP, IOMN)
> > > > > > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > > > > > + Return(Local0)
> > > > > > + }
> > > > > > + } // Device(CPHD)
> > > > > > + } // Scope(\_SB)
> > > > > > }
> > > > > > --
> > > > > > 1.8.3.1
> > > > >
> > > >
> > > >
> > > > --
> > > > Regards,
> > > > Igor
> >
> >
> > --
> > Regards,
> > Igor
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-23 16:24 ` Igor Mammedov
@ 2013-12-23 16:52 ` Laszlo Ersek
2013-12-28 0:39 ` Igor Mammedov
2013-12-23 16:55 ` Michael S. Tsirkin
1 sibling, 1 reply; 40+ messages in thread
From: Laszlo Ersek @ 2013-12-23 16:52 UTC (permalink / raw)
To: Igor Mammedov
Cc: aliguori, Michael S. Tsirkin, qemu-devel, hutao, jjherne,
brogers, kraxel, stefanha, kaneshige.kenji, chen.fan.fnst,
pbonzini
On 12/23/13 17:24, Igor Mammedov wrote:
> On Mon, 23 Dec 2013 16:48:49 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote:
>>> On Mon, 23 Dec 2013 13:26:37 +0200
>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>> Interesting. This seems to imply that it can't access
>>>> IO port for the disk. Which boot disk type are you using?
>>>> Is the _CRS resource overlapping any other resource
>>>> by any chance?
>>> Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus.
>>> PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and
>>> basically take all io ports except of 0xcf8-0xcff hole.
>>> Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already
>>> covered by bus CRS, the same applies to PCI hotplug as well.
>>> So I was doing it wrong trying advertise bus resources out of bus scope.
>>
>> Yes, that's explicitly prohibited by the firmware specification.
>>
>>> What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO
>>> ports CRS to PCI bus. Question is what PNP IDs they should use?
>>>
>>> It looks pretty much out of scope of cpu_hotplug and should be done for
>>> pci hotplug as well. So I'm ditching ACPI device and CRS parts from this
>>> series as not directly related.
>>> Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably
>>> for all resources consumed by it to make it right.
>>
>> Could be ok but are we using any new ports?
> yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not
> to use 0xaf00-0xaf1f.
>
>> If yes then I think before doing that we should make sure _CRS for
>> the host bridge does not include them, or consume them
> I'm fine with making holes in PCI bus CRES template, I can do it for
> pci-hotplug as well while at it.
Can you guys please summarize the problem? (Just so I can understand.)
\_SB.PCI0 consumes 0x0CF8..0x0CFF, and is a resource producer for all
other IO ports (ie. it passes them on to child devices). Did we try to
consume such a passed-on port in a device that is *not* a child of
\_SB.PCI0?
And if so, what's the suggested solution? Make the new consumer a child
of \_SB.PCI0, or punch out the new (non-child) consumer's port range
from \_SB.PCI0's forwarding?
>> in some child device.
> that would be cleanest, but is there any suggestion what device(s) it would be
> for piix and ich9-lpc, i.e. which PNP IDs should we use?
You could browse
<http://www.plasma-online.de/english/identify/serial/pnp_id_pnp.html>.
One suggestion could be:
PNP0C02 -- General ID for reserving resources required by PnP
motherboard registers. (Not device specific.)
(AFAICS this PNP ID has been mentioned earlier in the thread.)
PNP0C08 -- ACPI system board hardware
(Also used already, apparently.)
Laszlo
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-23 16:24 ` Igor Mammedov
2013-12-23 16:52 ` Laszlo Ersek
@ 2013-12-23 16:55 ` Michael S. Tsirkin
1 sibling, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2013-12-23 16:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: aliguori, hutao, jjherne, qemu-devel, kraxel, stefanha,
kaneshige.kenji, chen.fan.fnst, pbonzini, brogers, lersek
On Mon, Dec 23, 2013 at 05:24:30PM +0100, Igor Mammedov wrote:
> On Mon, 23 Dec 2013 16:48:49 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote:
> > > On Mon, 23 Dec 2013 13:26:37 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote:
> > > > > On Mon, 16 Dec 2013 21:53:07 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >
> > > > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote:
> > > > > > > .. and report range used by it to OSPM via _CRS.
> > > > > > > PRST is needed in SSDT since its base will depend on
> > > > > > > chipset and will be dynamically set by QEMU.
> > > > > > > Also move PRSC() method along with PRST since cross
> > > > > > > table reference to PRST doesn't work.
> > > > > > >
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > ---
> > > > > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +----------------------
> > > > > > > hw/i386/acpi-dsdt.dsl | 2 +-
> > > > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +-
> > > > > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++
> > > > > > > 4 files changed, 68 insertions(+), 40 deletions(-)
> > > > > > >
> > > > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > > > index 995b415..f26f81b 100644
> > > > > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > > > > > @@ -20,6 +20,7 @@
> > > > > > > Scope(\_SB) {
> > > > > > > /* Objects filled in by run-time generated SSDT */
> > > > > > > External(NTFY, MethodObj)
> > > > > > > + External(\_SB.CPHD.PRSC, MethodObj)
> > > > > > > External(CPON, PkgObj)
> > > > > > >
> > > > > > > /* Methods called by run-time generated SSDT Processor objects */
> > > > > > > @@ -51,42 +52,4 @@ Scope(\_SB) {
> > > > > > > // _EJ0 method - eject callback
> > > > > > > Sleep(200)
> > > > > > > }
> > > > > > > -
> > > > > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32)
> > > > > > > - Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > > > > - PRS, 256
> > > > > > > - }
> > > > > > > - Method(PRSC, 0) {
> > > > > > > - // Local5 = active cpu bitmap
> > > > > > > - Store(PRS, Local5)
> > > > > > > - // Local2 = last read byte from bitmap
> > > > > > > - Store(Zero, Local2)
> > > > > > > - // Local0 = Processor ID / APIC ID iterator
> > > > > > > - Store(Zero, Local0)
> > > > > > > - While (LLess(Local0, SizeOf(CPON))) {
> > > > > > > - // Local1 = CPON flag for this cpu
> > > > > > > - Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > > > > - If (And(Local0, 0x07)) {
> > > > > > > - // Shift down previously read bitmap byte
> > > > > > > - ShiftRight(Local2, 1, Local2)
> > > > > > > - } Else {
> > > > > > > - // Read next byte from cpu bitmap
> > > > > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > > > > - }
> > > > > > > - // Local3 = active state for this cpu
> > > > > > > - Store(And(Local2, 1), Local3)
> > > > > > > -
> > > > > > > - If (LNotEqual(Local1, Local3)) {
> > > > > > > - // State change - update CPON with new state
> > > > > > > - Store(Local3, Index(CPON, Local0))
> > > > > > > - // Do CPU notify
> > > > > > > - If (LEqual(Local3, 1)) {
> > > > > > > - NTFY(Local0, 1)
> > > > > > > - } Else {
> > > > > > > - NTFY(Local0, 3)
> > > > > > > - }
> > > > > > > - }
> > > > > > > - Increment(Local0)
> > > > > > > - }
> > > > > > > - }
> > > > > > > }
> > > > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > > > > > > index 90efce0..fa9f2d4 100644
> > > > > > > --- a/hw/i386/acpi-dsdt.dsl
> > > > > > > +++ b/hw/i386/acpi-dsdt.dsl
> > > > > > > @@ -311,7 +311,7 @@ DefinitionBlock (
> > > > > > > }
> > > > > > > Method(_E02) {
> > > > > > > // CPU hotplug event
> > > > > > > - \_SB.PRSC()
> > > > > > > + \_SB.CPHD.PRSC()
> > > > > > > }
> > > > > > > Method(_L03) {
> > > > > > > }
> > > > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > > > > > > index 22baa58..9ccc543 100644
> > > > > > > --- a/hw/i386/q35-acpi-dsdt.dsl
> > > > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > > > > > > @@ -420,7 +420,7 @@ DefinitionBlock (
> > > > > > > }
> > > > > > > Method(_E02) {
> > > > > > > // CPU hotplug event
> > > > > > > - \_SB.PRSC()
> > > > > > > + \_SB.CPHD.PRSC()
> > > > > > > }
> > > > > > > Method(_L03) {
> > > > > > > }
> > > > > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > > > > > > index a4484b8..ec8893c 100644
> > > > > > > --- a/hw/i386/ssdt-misc.dsl
> > > > > > > +++ b/hw/i386/ssdt-misc.dsl
> > > > > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > > > > > > }
> > > > > > > }
> > > > > > > }
> > > > > > > + Scope(\_SB) {
> > > > > > > + External(NTFY, MethodObj)
> > > > > > > + External(CPON, PkgObj)
> > > > > > > +
> > > > > > > + Device(CPHD) {
> > > > > > > + Name(_HID, EISAID("PNP0C08"))
> > > > > > > + Name(CPPL, 32) // cpu-gpe length
> > > > > > > + Name(CPHP, 0xaf00)
> > > > > > > +
> > > > > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL)
> > > > > > > + Field(PRST, ByteAcc, NoLock, Preserve) {
> > > > > > > + PRS, 256
> > > > > > > + }
> > > > > > > +
> > > > > > > + Method(PRSC, 0) {
> > > > > > > + // Local5 = active cpu bitmap
> > > > > > > + Store(PRS, Local5)
> > > > > > > + // Local2 = last read byte from bitmap
> > > > > > > + Store(Zero, Local2)
> > > > > > > + // Local0 = Processor ID / APIC ID iterator
> > > > > > > + Store(Zero, Local0)
> > > > > > > + While (LLess(Local0, SizeOf(CPON))) {
> > > > > > > + // Local1 = CPON flag for this cpu
> > > > > > > + Store(DerefOf(Index(CPON, Local0)), Local1)
> > > > > > > + If (And(Local0, 0x07)) {
> > > > > > > + // Shift down previously read bitmap byte
> > > > > > > + ShiftRight(Local2, 1, Local2)
> > > > > > > + } Else {
> > > > > > > + // Read next byte from cpu bitmap
> > > > > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
> > > > > > > + }
> > > > > > > + // Local3 = active state for this cpu
> > > > > > > + Store(And(Local2, 1), Local3)
> > > > > > > +
> > > > > > > + If (LNotEqual(Local1, Local3)) {
> > > > > > > + // State change - update CPON with new state
> > > > > > > + Store(Local3, Index(CPON, Local0))
> > > > > > > + // Do CPU notify
> > > > > > > + If (LEqual(Local3, 1)) {
> > > > > > > + NTFY(Local0, 1)
> > > > > > > + } Else {
> > > > > > > + NTFY(Local0, 3)
> > > > > > > + }
> > > > > > > + }
> > > > > > > + Increment(Local0)
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > + /* Leave bit 0 cleared to avoid Windows BSOD */
> > > > > > > + Name(_STA, 0xA)
> > > > > >
> > > > > > This shared the problem you yourself pointed out with
> > > > > > patch 'pc: ACPI BIOS: implement memory hotplug':
> > > > > > if we make device non present ospm can ignore our _CRS.
> > > > > >
> > > > > > Can't we get a better handle on why windows crashes?
> > > > > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or
> > > > > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack:
> > > > > : nt!PnpBootDeviceWait+0x181
> > > > > : nt!IopInitializeBootDrivers+0xbd3
> > > > > : nt!IoInitSystem+0xbb6
> > > > > : nt!Phase1InitializationDiscard+0x16f6
> > > > > : nt!Phase1Initialization+0x13
> > > > > : nt!PspSystemThreadStartup+0x23d
> > > > > : nt!KiStartSystemThread+0x16
> > > >
> > > >
> > > > Interesting. This seems to imply that it can't access
> > > > IO port for the disk. Which boot disk type are you using?
> > > > Is the _CRS resource overlapping any other resource
> > > > by any chance?
> > > Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus.
> > > PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and
> > > basically take all io ports except of 0xcf8-0xcff hole.
> > > Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already
> > > covered by bus CRS, the same applies to PCI hotplug as well.
> > > So I was doing it wrong trying advertise bus resources out of bus scope.
> >
> > Yes, that's explicitly prohibited by the firmware specification.
> >
> > > What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO
> > > ports CRS to PCI bus. Question is what PNP IDs they should use?
> > >
> > > It looks pretty much out of scope of cpu_hotplug and should be done for
> > > pci hotplug as well. So I'm ditching ACPI device and CRS parts from this
> > > series as not directly related.
> > > Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably
> > > for all resources consumed by it to make it right.
> >
> > Could be ok but are we using any new ports?
> yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not
> to use 0xaf00-0xaf1f.
>
> > If yes then I think before doing that we should make sure _CRS for
> > the host bridge does not include them, or consume them
> I'm fine with making holes in PCI bus CRES template, I can do it for
> pci-hotplug as well while at it.
Cool. We might need to make this conditional on using
a new machine type.
> > in some child device.
> that would be cleanest, but is there any suggestion what device(s) it would be
> for piix and ich9-lpc, i.e. which PNP IDs should we use?
AFAIK if they are pci devices they don't need PNP IDs, they
are matched by PCI bus address.
>
> > Otherwise we increase the chance of a conflict in case some
> > guest uses that port.
> >
> > >
> > > >
> > > > >
> > > > > However it works fine if containing device is PNP0C02 "Motherboard registers",
> > > > > so we probably should use it instead.
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > +
> > > > > > > + Method(_CRS, 0) {
> > > > > > > + Store(ResourceTemplate() {
> > > > > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO)
> > > > > > > + }, Local0)
> > > > > > > +
> > > > > > > + CreateWordField(Local0, IO._MIN, IOMN)
> > > > > > > + CreateWordField(Local0, IO._MAX, IOMX)
> > > > > > > +
> > > > > > > + Store(CPHP, IOMN)
> > > > > > > + Subtract(Add(CPHP, CPPL), 1, IOMX)
> > > > > > > + Return(Local0)
> > > > > > > + }
> > > > > > > + } // Device(CPHD)
> > > > > > > + } // Scope(\_SB)
> > > > > > > }
> > > > > > > --
> > > > > > > 1.8.3.1
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Igor
> > >
> > >
> > > --
> > > Regards,
> > > Igor
>
>
> --
> Regards,
> Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
2013-12-23 16:52 ` Laszlo Ersek
@ 2013-12-28 0:39 ` Igor Mammedov
0 siblings, 0 replies; 40+ messages in thread
From: Igor Mammedov @ 2013-12-28 0:39 UTC (permalink / raw)
To: Laszlo Ersek
Cc: aliguori, Michael S. Tsirkin, qemu-devel, hutao, jjherne,
brogers, kraxel, stefanha, kaneshige.kenji, chen.fan.fnst,
pbonzini
On Mon, 23 Dec 2013 17:52:03 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/23/13 17:24, Igor Mammedov wrote:
> > On Mon, 23 Dec 2013 16:48:49 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote:
> >>> On Mon, 23 Dec 2013 13:26:37 +0200
> >>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> >>>> Interesting. This seems to imply that it can't access
> >>>> IO port for the disk. Which boot disk type are you using?
> >>>> Is the _CRS resource overlapping any other resource
> >>>> by any chance?
> >>> Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus.
> >>> PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and
> >>> basically take all io ports except of 0xcf8-0xcff hole.
> >>> Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already
> >>> covered by bus CRS, the same applies to PCI hotplug as well.
> >>> So I was doing it wrong trying advertise bus resources out of bus scope.
> >>
> >> Yes, that's explicitly prohibited by the firmware specification.
> >>
> >>> What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO
> >>> ports CRS to PCI bus. Question is what PNP IDs they should use?
> >>>
> >>> It looks pretty much out of scope of cpu_hotplug and should be done for
> >>> pci hotplug as well. So I'm ditching ACPI device and CRS parts from this
> >>> series as not directly related.
> >>> Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably
> >>> for all resources consumed by it to make it right.
> >>
> >> Could be ok but are we using any new ports?
> > yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not
> > to use 0xaf00-0xaf1f.
> >
> >> If yes then I think before doing that we should make sure _CRS for
> >> the host bridge does not include them, or consume them
> > I'm fine with making holes in PCI bus CRES template, I can do it for
> > pci-hotplug as well while at it.
>
> Can you guys please summarize the problem? (Just so I can understand.)
>
> \_SB.PCI0 consumes 0x0CF8..0x0CFF, and is a resource producer for all
> other IO ports (ie. it passes them on to child devices). Did we try to
> consume such a passed-on port in a device that is *not* a child of
> \_SB.PCI0?
yep, and that was an error.
>
> And if so, what's the suggested solution? Make the new consumer a child
> of \_SB.PCI0, or punch out the new (non-child) consumer's port range
> from \_SB.PCI0's forwarding?
I've tried to add bridge that uses hotplug ports as Device under PCI bus with
respective _CRS so it would consume ports, unfortunately for PIIX bridge
isn't visible under Windows and any other ways to confirm that _CRS was
consumed failed. The same for linux. For Q35, Windows' device manager shows
LPC bridge however there is no "Resources" tab with consumed ports.
Due to lack of ways to confirm that _CRS was really consumed,
I've switched to punching holes, and exposing hotplug IO region as
\_SB.Device(ACPI0004)._CRS. It works rather stable with new and old
versions Windows.
>
> >> in some child device.
> > that would be cleanest, but is there any suggestion what device(s) it would be
> > for piix and ich9-lpc, i.e. which PNP IDs should we use?
>
> You could browse
> <http://www.plasma-online.de/english/identify/serial/pnp_id_pnp.html>.
>
> One suggestion could be:
>
> PNP0C02 -- General ID for reserving resources required by PnP
> motherboard registers. (Not device specific.)
I've chosen ACPI0004 instead, as the later we might wish to group
CPU devices under it (when extending support for 1024 and more CPUs)
>
> (AFAICS this PNP ID has been mentioned earlier in the thread.)
>
> PNP0C08 -- ACPI system board hardware
it doesn't work.
>
> (Also used already, apparently.)
>
> Laszlo
--
Regards,
Igor
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2013-12-28 0:40 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-13 16:22 [Qemu-devel] [PATCH 00/11] pc: CPU hotplug support for Q35 Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 01/11] acpi: piix4: remove not needed GPE0 mask Igor Mammedov
2013-12-19 14:16 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 02/11] acpi: factor out common pm_update_sci() into acpi core Igor Mammedov
2013-12-19 14:16 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35 Igor Mammedov
2013-12-19 14:14 ` Michael S. Tsirkin
2013-12-19 15:13 ` Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 04/11] acpi/piix4: add readonly "cpu-hotplug-io-base" property Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 05/11] acpi: ich9: allow guest to clear SCI rised by GPE Igor Mammedov
2013-12-19 14:16 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 06/11] acpi/ich9: add CPU hotplug handling to Q35 machine Igor Mammedov
2013-12-19 14:18 ` Michael S. Tsirkin
2013-12-19 15:17 ` Igor Mammedov
2013-12-19 15:33 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 07/11] ACPI: Q35 DSDT: fix CPU hotplug GPE0.2 handler Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 08/11] ACPI/DSDT-CPU: cleanup bogus comment Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT Igor Mammedov
2013-12-16 19:30 ` Michael S. Tsirkin
2013-12-16 20:38 ` Igor Mammedov
2013-12-16 21:13 ` Laszlo Ersek
2013-12-16 21:22 ` Laszlo Ersek
2013-12-16 21:53 ` Igor Mammedov
2013-12-17 10:39 ` Michael S. Tsirkin
2013-12-16 21:44 ` Laszlo Ersek
2013-12-16 21:59 ` Igor Mammedov
2013-12-16 22:22 ` Laszlo Ersek
2013-12-16 23:13 ` Igor Mammedov
2013-12-16 19:53 ` Michael S. Tsirkin
2013-12-16 22:15 ` Igor Mammedov
2013-12-22 14:51 ` Igor Mammedov
2013-12-23 11:26 ` Michael S. Tsirkin
2013-12-23 13:06 ` Igor Mammedov
2013-12-23 14:48 ` Michael S. Tsirkin
2013-12-23 16:24 ` Igor Mammedov
2013-12-23 16:52 ` Laszlo Ersek
2013-12-28 0:39 ` Igor Mammedov
2013-12-23 16:55 ` Michael S. Tsirkin
2013-12-13 16:22 ` [Qemu-devel] [PATCH 10/11] ACPI: set CPU hotplug io base dynamically Igor Mammedov
2013-12-13 16:22 ` [Qemu-devel] [PATCH 11/11] ACPI: update ssdt-misc.hex.generated acpi-dsdt.hex.generated q35-acpi-dsdt.hex.generated Igor Mammedov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.