* [PATCH] various fixes for CPU/PCI hotplug
@ 2009-02-03 14:01 Gleb Natapov
2009-02-03 14:46 ` dima
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Gleb Natapov @ 2009-02-03 14:01 UTC (permalink / raw)
To: avi; +Cc: glommer, kvm
Hi,
The fixed problems are:
1) Disabled processor _STA method should return 0 (this fixes Vista's
resume after hibernate problem)
2) Disabled processor _MAT method should return disabled MADT entry
instead of 0
3) Fix number of hot pluggable CPUs to be 16
4) Properly handle write to GPE STS register (write 1 should clear bit)
5) Generate interrupt only if corespondent EN bit is set
5) Use reserved STS bits from PIIX4 chipset to avoid clash in the
future.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
index d67616d..c400382 100755
--- a/bios/acpi-dsdt.dsl
+++ b/bios/acpi-dsdt.dsl
@@ -27,28 +27,29 @@ DefinitionBlock (
{
Scope (\_PR)
{
- OperationRegion( PRST, SystemIO, 0xaf00, 0x02)
- Field (PRST, ByteAcc, NoLock, WriteAsZeros)
+ OperationRegion( PRST, SystemIO, 0xaf00, 0x04)
+ Field (PRST, WordAcc, NoLock, Preserve)
{
- PRU, 8,
- PRD, 8,
+ PRU, 16,
+ PRD, 16,
}
#define gen_processor(nr, name) \
Processor (CPU##name, nr, 0x0000b010, 0x06) { \
- Name (TMP, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0, 0x0}) \
+ Name (PREN, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0, 0x0}) \
+ Name (PRDS, Buffer(0x8) {0x0, 0x8, nr, nr, 0x0, 0x0, 0x0, 0x0}) \
Method(_MAT, 0) { \
- If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(TMP) } \
- Else { Return(0x0) } \
+ If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(PREN) } \
+ Else { Return(PRDS) } \
} \
Method (_STA) { \
- Return(0xF) \
+ If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(0xF) } \
+ Else { Return(0x0) } \
} \
} \
-
- Processor (CPU0, 0x00, 0x0000b010, 0x06) {Method (_STA) { Return(0xF)}}
+ gen_processor(0, 0)
gen_processor(1, 1)
gen_processor(2, 2)
gen_processor(3, 3)
@@ -63,6 +64,28 @@ DefinitionBlock (
gen_processor(12, C)
gen_processor(13, D)
gen_processor(14, E)
+
+ Method (NTFY, 2, NotSerialized) {
+#define gen_ntfy(nr) \
+ If (LEqual(Arg0, 0x##nr)) { \
+ Notify(CPU##nr, Arg1) \
+ }
+ gen_ntfy(0)
+ gen_ntfy(1)
+ gen_ntfy(2)
+ gen_ntfy(3)
+ gen_ntfy(4)
+ gen_ntfy(5)
+ gen_ntfy(6)
+ gen_ntfy(7)
+ gen_ntfy(8)
+ gen_ntfy(9)
+ gen_ntfy(A)
+ gen_ntfy(B)
+ gen_ntfy(C)
+ gen_ntfy(D)
+ gen_ntfy(E)
+ }
}
Scope (\)
@@ -666,33 +689,12 @@ DefinitionBlock (
Zero, /* reserved */
Zero /* reserved */
})
+
Scope (\_GPE)
{
-
-#define gen_cpu_hotplug(name, nr) \
- If (And(\_PR.PRU, ShiftLeft(1, nr))) { \
- Notify(\_PR.CPU##name, 1) \
- } \
- If (And(\_PR.PRD, ShiftLeft(1, nr))) { \
- Notify(\_PR.CPU##name, 3) \
- }
+ Name(_HID, "ACPI0006")
Method(_L00) {
- gen_cpu_hotplug(1, 1)
- gen_cpu_hotplug(2, 2)
- gen_cpu_hotplug(3, 3)
- gen_cpu_hotplug(4, 4)
- gen_cpu_hotplug(5, 5)
- gen_cpu_hotplug(6, 6)
- gen_cpu_hotplug(7, 7)
- gen_cpu_hotplug(8, 8)
- gen_cpu_hotplug(9, 9)
- gen_cpu_hotplug(A, 10)
- gen_cpu_hotplug(B, 11)
- gen_cpu_hotplug(C, 12)
- gen_cpu_hotplug(D, 13)
- gen_cpu_hotplug(E, 14)
-
Return(0x01)
}
@@ -739,9 +741,29 @@ DefinitionBlock (
Return(0x01)
}
+
Method(_L02) {
+ Store(Zero, Local3)
+ Store(\_PR.PRU, Local2)
+ Xor(Local2, \_PR.PRD, Local0)
+ Store(Local2, \_PR.PRD)
+ Store(\_PR.PRD, Local1)
+ While (LNotEqual (Local0, Zero)) {
+ Store(ShiftLeft(1, Local3), Local1)
+ If (And(Local0, Local1)) {
+ Store(And(Local0, Not(Local1)), Local0)
+ If (And(Local2, Local1)) {
+ Store(1, Local4)
+ } Else {
+ Store(3, Local4)
+ }
+ \_PR.NTFY(Local3, Local4)
+ }
+ Increment(Local3)
+ }
Return(0x01)
}
+
Method(_L03) {
Return(0x01)
}
diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
index b998225..337432a 100644
--- a/qemu/hw/acpi.c
+++ b/qemu/hw/acpi.c
@@ -578,8 +578,8 @@ void qemu_system_powerdown(void)
struct gpe_regs {
uint16_t sts; /* status */
uint16_t en; /* enabled */
- uint8_t up;
- uint8_t down;
+ uint16_t cpus_sts;
+ uint16_t bios_cpus_sts;
};
struct pci_status {
@@ -590,29 +590,34 @@ struct pci_status {
static struct gpe_regs gpe;
static struct pci_status pci0_status;
+static uint32_t gpe_read_val(uint16_t val, uint32_t addr)
+{
+ if (addr & 1)
+ return (val >> 8) & 0xff;
+ return val & 0xff;
+}
+
static uint32_t gpe_readb(void *opaque, uint32_t addr)
{
uint32_t val = 0;
struct gpe_regs *g = opaque;
switch (addr) {
case PROC_BASE:
- val = g->up;
- break;
case PROC_BASE + 1:
- val = g->down;
+ val = gpe_read_val(g->cpus_sts, addr);
+ break;
+ case PROC_BASE + 2:
+ case PROC_BASE + 3:
+ val = gpe_read_val(g->bios_cpus_sts, addr);
break;
case GPE_BASE:
- val = g->sts & 0xFF;
- break;
case GPE_BASE + 1:
- val = (g->sts >> 8) & 0xFF;
+ val = gpe_read_val(g->sts, addr);
break;
case GPE_BASE + 2:
- val = g->en & 0xFF;
- break;
case GPE_BASE + 3:
- val = (g->en >> 8) & 0xFF;
+ val = gpe_read_val(g->en, addr);
break;
default:
break;
@@ -624,28 +629,45 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
return val;
}
+static uint16_t gpe_write_val(uint16_t cur, int addr, uint32_t val)
+{
+ if (addr & 1)
+ return (cur & 0xff) | (val << 8);
+ return (cur & 0xff00) | (val & 0xff);
+}
+
+static uint16_t gpe_reset_val(uint16_t cur, int addr, uint32_t val)
+{
+ uint16_t x1, x0 = val & 0xff;
+ int shift = (addr & 1) ? 8 : 0;
+
+ x1 = (cur >> shift) & 0xff;
+
+ x1 = x1 & ~x0;
+
+ return (cur & (0xff << (8 - shift))) | (x1 << shift);
+}
+
static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
{
struct gpe_regs *g = opaque;
switch (addr) {
case PROC_BASE:
- g->up = val;
- break;
case PROC_BASE + 1:
- g->down = val;
+ /* don't allow to change cpu_sts from inside a guest */
+ break;
+ case PROC_BASE + 2:
+ case PROC_BASE + 3:
+ g->bios_cpus_sts = gpe_write_val(g->bios_cpus_sts, addr, val);
break;
case GPE_BASE:
- g->sts = (g->sts & ~0xFFFF) | (val & 0xFFFF);
- break;
case GPE_BASE + 1:
- g->sts = (g->sts & 0xFFFF) | (val << 8);
+ g->sts = gpe_reset_val(g->sts, addr, val);
break;
case GPE_BASE + 2:
- g->en = (g->en & ~0xFFFF) | (val & 0xFFFF);
- break;
case GPE_BASE + 3:
- g->en = (g->en & 0xFFFF) | (val << 8);
+ g->en = gpe_write_val(g->en, addr, val);
break;
default:
break;
@@ -717,6 +739,7 @@ static const char *model;
void qemu_system_hot_add_init(const char *cpu_model)
{
+ gpe.bios_cpus_sts = gpe.cpus_sts = (1 << smp_cpus) - 1;
register_ioport_write(GPE_BASE, 4, 1, gpe_writeb, &gpe);
register_ioport_read(GPE_BASE, 4, 1, gpe_readb, &gpe);
@@ -734,16 +757,14 @@ void qemu_system_hot_add_init(const char *cpu_model)
static void enable_processor(struct gpe_regs *g, int cpu)
{
- g->sts |= 1;
- g->en |= 1;
- g->up |= (1 << cpu);
+ g->sts |= 4;
+ g->cpus_sts |= (1 << cpu);
}
static void disable_processor(struct gpe_regs *g, int cpu)
{
- g->sts |= 1;
- g->en |= 1;
- g->down |= (1 << cpu);
+ g->sts |= 4;
+ g->cpus_sts &= ~(1 << cpu);
}
#if defined(TARGET_I386) || defined(TARGET_X86_64)
@@ -784,39 +805,40 @@ void qemu_system_cpu_hot_add(int cpu, int state)
#endif
}
- qemu_set_irq(pm_state->irq, 1);
- gpe.up = 0;
- gpe.down = 0;
if (state)
enable_processor(&gpe, cpu);
else
disable_processor(&gpe, cpu);
- qemu_set_irq(pm_state->irq, 0);
+ if (gpe.en & 4) {
+ qemu_set_irq(pm_state->irq, 1);
+ qemu_set_irq(pm_state->irq, 0);
+ }
}
#endif
static void enable_device(struct pci_status *p, struct gpe_regs *g, int slot)
{
g->sts |= 2;
- g->en |= 2;
p->up |= (1 << slot);
}
static void disable_device(struct pci_status *p, struct gpe_regs *g, int slot)
{
g->sts |= 2;
- g->en |= 2;
p->down |= (1 << slot);
}
void qemu_system_device_hot_add(int pcibus, int slot, int state)
{
- qemu_set_irq(pm_state->irq, 1);
pci0_status.up = 0;
pci0_status.down = 0;
if (state)
enable_device(&pci0_status, &gpe, slot);
else
disable_device(&pci0_status, &gpe, slot);
- qemu_set_irq(pm_state->irq, 0);
+
+ if (gpe.en & 2) {
+ qemu_set_irq(pm_state->irq, 1);
+ qemu_set_irq(pm_state->irq, 0);
+ }
}
--
Gleb.
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] various fixes for CPU/PCI hotplug
2009-02-03 14:01 [PATCH] various fixes for CPU/PCI hotplug Gleb Natapov
@ 2009-02-03 14:46 ` dima
2009-02-03 15:13 ` Gleb Natapov
2009-02-03 16:43 ` Marcelo Tosatti
2009-02-03 18:58 ` Glauber Costa
2 siblings, 1 reply; 8+ messages in thread
From: dima @ 2009-02-03 14:46 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm
how to add new cpu by hotplug to running system ?
On Tuesday 03 February 2009 17:01:29 Gleb Natapov wrote:
> Hi,
>
> The fixed problems are:
> 1) Disabled processor _STA method should return 0 (this fixes Vista's
> resume after hibernate problem)
> 2) Disabled processor _MAT method should return disabled MADT entry
> instead of 0
> 3) Fix number of hot pluggable CPUs to be 16
> 4) Properly handle write to GPE STS register (write 1 should clear bit)
> 5) Generate interrupt only if corespondent EN bit is set
> 5) Use reserved STS bits from PIIX4 chipset to avoid clash in the
> future.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
> index d67616d..c400382 100755
> --- a/bios/acpi-dsdt.dsl
> +++ b/bios/acpi-dsdt.dsl
> @@ -27,28 +27,29 @@ DefinitionBlock (
> {
> Scope (\_PR)
> {
> - OperationRegion( PRST, SystemIO, 0xaf00, 0x02)
> - Field (PRST, ByteAcc, NoLock, WriteAsZeros)
> + OperationRegion( PRST, SystemIO, 0xaf00, 0x04)
> + Field (PRST, WordAcc, NoLock, Preserve)
> {
> - PRU, 8,
> - PRD, 8,
> + PRU, 16,
> + PRD, 16,
> }
>
> #define gen_processor(nr, name) \
> Processor (CPU##name, nr, 0x0000b010, 0x06) { \
> - Name (TMP, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0, 0x0})
> \ + Name (PREN, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0,
> 0x0}) \ + Name (PRDS, Buffer(0x8) {0x0, 0x8, nr, nr, 0x0, 0x0,
> 0x0, 0x0}) \ Method(_MAT, 0) {
> \ - If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(TMP) }
> \ - Else { Return(0x0) }
> \ + If (And(\_PR.PRU, ShiftLeft(1, nr))) {
> Return(PREN) } \ + Else { Return(PRDS) }
> \ }
> \ Method (_STA) {
> \ - Return(0xF)
> \ + If (And(\_PR.PRU, ShiftLeft(1, nr))) {
> Return(0xF) } \ + Else { Return(0x0) }
> \ }
> \ }
> \
>
>
> -
> - Processor (CPU0, 0x00, 0x0000b010, 0x06) {Method (_STA) {
> Return(0xF)}} + gen_processor(0, 0)
> gen_processor(1, 1)
> gen_processor(2, 2)
> gen_processor(3, 3)
> @@ -63,6 +64,28 @@ DefinitionBlock (
> gen_processor(12, C)
> gen_processor(13, D)
> gen_processor(14, E)
> +
> + Method (NTFY, 2, NotSerialized) {
> +#define gen_ntfy(nr) \
> + If (LEqual(Arg0, 0x##nr)) { \
> + Notify(CPU##nr, Arg1) \
> + }
> + gen_ntfy(0)
> + gen_ntfy(1)
> + gen_ntfy(2)
> + gen_ntfy(3)
> + gen_ntfy(4)
> + gen_ntfy(5)
> + gen_ntfy(6)
> + gen_ntfy(7)
> + gen_ntfy(8)
> + gen_ntfy(9)
> + gen_ntfy(A)
> + gen_ntfy(B)
> + gen_ntfy(C)
> + gen_ntfy(D)
> + gen_ntfy(E)
> + }
> }
>
> Scope (\)
> @@ -666,33 +689,12 @@ DefinitionBlock (
> Zero, /* reserved */
> Zero /* reserved */
> })
> +
> Scope (\_GPE)
> {
> -
> -#define gen_cpu_hotplug(name, nr) \
> - If (And(\_PR.PRU, ShiftLeft(1, nr))) { \
> - Notify(\_PR.CPU##name, 1) \
> - } \
> - If (And(\_PR.PRD, ShiftLeft(1, nr))) { \
> - Notify(\_PR.CPU##name, 3) \
> - }
> + Name(_HID, "ACPI0006")
>
> Method(_L00) {
> - gen_cpu_hotplug(1, 1)
> - gen_cpu_hotplug(2, 2)
> - gen_cpu_hotplug(3, 3)
> - gen_cpu_hotplug(4, 4)
> - gen_cpu_hotplug(5, 5)
> - gen_cpu_hotplug(6, 6)
> - gen_cpu_hotplug(7, 7)
> - gen_cpu_hotplug(8, 8)
> - gen_cpu_hotplug(9, 9)
> - gen_cpu_hotplug(A, 10)
> - gen_cpu_hotplug(B, 11)
> - gen_cpu_hotplug(C, 12)
> - gen_cpu_hotplug(D, 13)
> - gen_cpu_hotplug(E, 14)
> -
> Return(0x01)
> }
>
> @@ -739,9 +741,29 @@ DefinitionBlock (
>
> Return(0x01)
> }
> +
> Method(_L02) {
> + Store(Zero, Local3)
> + Store(\_PR.PRU, Local2)
> + Xor(Local2, \_PR.PRD, Local0)
> + Store(Local2, \_PR.PRD)
> + Store(\_PR.PRD, Local1)
> + While (LNotEqual (Local0, Zero)) {
> + Store(ShiftLeft(1, Local3), Local1)
> + If (And(Local0, Local1)) {
> + Store(And(Local0, Not(Local1)), Local0)
> + If (And(Local2, Local1)) {
> + Store(1, Local4)
> + } Else {
> + Store(3, Local4)
> + }
> + \_PR.NTFY(Local3, Local4)
> + }
> + Increment(Local3)
> + }
> Return(0x01)
> }
> +
> Method(_L03) {
> Return(0x01)
> }
> diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
> index b998225..337432a 100644
> --- a/qemu/hw/acpi.c
> +++ b/qemu/hw/acpi.c
> @@ -578,8 +578,8 @@ void qemu_system_powerdown(void)
> struct gpe_regs {
> uint16_t sts; /* status */
> uint16_t en; /* enabled */
> - uint8_t up;
> - uint8_t down;
> + uint16_t cpus_sts;
> + uint16_t bios_cpus_sts;
> };
>
> struct pci_status {
> @@ -590,29 +590,34 @@ struct pci_status {
> static struct gpe_regs gpe;
> static struct pci_status pci0_status;
>
> +static uint32_t gpe_read_val(uint16_t val, uint32_t addr)
> +{
> + if (addr & 1)
> + return (val >> 8) & 0xff;
> + return val & 0xff;
> +}
> +
> static uint32_t gpe_readb(void *opaque, uint32_t addr)
> {
> uint32_t val = 0;
> struct gpe_regs *g = opaque;
> switch (addr) {
> case PROC_BASE:
> - val = g->up;
> - break;
> case PROC_BASE + 1:
> - val = g->down;
> + val = gpe_read_val(g->cpus_sts, addr);
> + break;
> + case PROC_BASE + 2:
> + case PROC_BASE + 3:
> + val = gpe_read_val(g->bios_cpus_sts, addr);
> break;
>
> case GPE_BASE:
> - val = g->sts & 0xFF;
> - break;
> case GPE_BASE + 1:
> - val = (g->sts >> 8) & 0xFF;
> + val = gpe_read_val(g->sts, addr);
> break;
> case GPE_BASE + 2:
> - val = g->en & 0xFF;
> - break;
> case GPE_BASE + 3:
> - val = (g->en >> 8) & 0xFF;
> + val = gpe_read_val(g->en, addr);
> break;
> default:
> break;
> @@ -624,28 +629,45 @@ static uint32_t gpe_readb(void *opaque, uint32_t
> addr) return val;
> }
>
> +static uint16_t gpe_write_val(uint16_t cur, int addr, uint32_t val)
> +{
> + if (addr & 1)
> + return (cur & 0xff) | (val << 8);
> + return (cur & 0xff00) | (val & 0xff);
> +}
> +
> +static uint16_t gpe_reset_val(uint16_t cur, int addr, uint32_t val)
> +{
> + uint16_t x1, x0 = val & 0xff;
> + int shift = (addr & 1) ? 8 : 0;
> +
> + x1 = (cur >> shift) & 0xff;
> +
> + x1 = x1 & ~x0;
> +
> + return (cur & (0xff << (8 - shift))) | (x1 << shift);
> +}
> +
> static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
> {
> struct gpe_regs *g = opaque;
> switch (addr) {
> case PROC_BASE:
> - g->up = val;
> - break;
> case PROC_BASE + 1:
> - g->down = val;
> + /* don't allow to change cpu_sts from inside a guest */
> + break;
> + case PROC_BASE + 2:
> + case PROC_BASE + 3:
> + g->bios_cpus_sts = gpe_write_val(g->bios_cpus_sts, addr, val);
> break;
>
> case GPE_BASE:
> - g->sts = (g->sts & ~0xFFFF) | (val & 0xFFFF);
> - break;
> case GPE_BASE + 1:
> - g->sts = (g->sts & 0xFFFF) | (val << 8);
> + g->sts = gpe_reset_val(g->sts, addr, val);
> break;
> case GPE_BASE + 2:
> - g->en = (g->en & ~0xFFFF) | (val & 0xFFFF);
> - break;
> case GPE_BASE + 3:
> - g->en = (g->en & 0xFFFF) | (val << 8);
> + g->en = gpe_write_val(g->en, addr, val);
> break;
> default:
> break;
> @@ -717,6 +739,7 @@ static const char *model;
>
> void qemu_system_hot_add_init(const char *cpu_model)
> {
> + gpe.bios_cpus_sts = gpe.cpus_sts = (1 << smp_cpus) - 1;
> register_ioport_write(GPE_BASE, 4, 1, gpe_writeb, &gpe);
> register_ioport_read(GPE_BASE, 4, 1, gpe_readb, &gpe);
>
> @@ -734,16 +757,14 @@ void qemu_system_hot_add_init(const char *cpu_model)
>
> static void enable_processor(struct gpe_regs *g, int cpu)
> {
> - g->sts |= 1;
> - g->en |= 1;
> - g->up |= (1 << cpu);
> + g->sts |= 4;
> + g->cpus_sts |= (1 << cpu);
> }
>
> static void disable_processor(struct gpe_regs *g, int cpu)
> {
> - g->sts |= 1;
> - g->en |= 1;
> - g->down |= (1 << cpu);
> + g->sts |= 4;
> + g->cpus_sts &= ~(1 << cpu);
> }
>
> #if defined(TARGET_I386) || defined(TARGET_X86_64)
> @@ -784,39 +805,40 @@ void qemu_system_cpu_hot_add(int cpu, int state)
> #endif
> }
>
> - qemu_set_irq(pm_state->irq, 1);
> - gpe.up = 0;
> - gpe.down = 0;
> if (state)
> enable_processor(&gpe, cpu);
> else
> disable_processor(&gpe, cpu);
> - qemu_set_irq(pm_state->irq, 0);
> + if (gpe.en & 4) {
> + qemu_set_irq(pm_state->irq, 1);
> + qemu_set_irq(pm_state->irq, 0);
> + }
> }
> #endif
>
> static void enable_device(struct pci_status *p, struct gpe_regs *g, int
> slot) {
> g->sts |= 2;
> - g->en |= 2;
> p->up |= (1 << slot);
> }
>
> static void disable_device(struct pci_status *p, struct gpe_regs *g, int
> slot) {
> g->sts |= 2;
> - g->en |= 2;
> p->down |= (1 << slot);
> }
>
> void qemu_system_device_hot_add(int pcibus, int slot, int state)
> {
> - qemu_set_irq(pm_state->irq, 1);
> pci0_status.up = 0;
> pci0_status.down = 0;
> if (state)
> enable_device(&pci0_status, &gpe, slot);
> else
> disable_device(&pci0_status, &gpe, slot);
> - qemu_set_irq(pm_state->irq, 0);
> +
> + if (gpe.en & 2) {
> + qemu_set_irq(pm_state->irq, 1);
> + qemu_set_irq(pm_state->irq, 0);
> + }
> }
> --
> Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] various fixes for CPU/PCI hotplug
2009-02-03 14:46 ` dima
@ 2009-02-03 15:13 ` Gleb Natapov
0 siblings, 0 replies; 8+ messages in thread
From: Gleb Natapov @ 2009-02-03 15:13 UTC (permalink / raw)
To: dima; +Cc: kvm
On Tue, Feb 03, 2009 at 05:46:28PM +0300, dima wrote:
> how to add new cpu by hotplug to running system ?
>
See cpu_set command in the monitor.
>
> On Tuesday 03 February 2009 17:01:29 Gleb Natapov wrote:
> > Hi,
> >
> > The fixed problems are:
> > 1) Disabled processor _STA method should return 0 (this fixes Vista's
> > resume after hibernate problem)
> > 2) Disabled processor _MAT method should return disabled MADT entry
> > instead of 0
> > 3) Fix number of hot pluggable CPUs to be 16
> > 4) Properly handle write to GPE STS register (write 1 should clear bit)
> > 5) Generate interrupt only if corespondent EN bit is set
> > 5) Use reserved STS bits from PIIX4 chipset to avoid clash in the
> > future.
> >
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
> > index d67616d..c400382 100755
> > --- a/bios/acpi-dsdt.dsl
> > +++ b/bios/acpi-dsdt.dsl
> > @@ -27,28 +27,29 @@ DefinitionBlock (
> > {
> > Scope (\_PR)
> > {
> > - OperationRegion( PRST, SystemIO, 0xaf00, 0x02)
> > - Field (PRST, ByteAcc, NoLock, WriteAsZeros)
> > + OperationRegion( PRST, SystemIO, 0xaf00, 0x04)
> > + Field (PRST, WordAcc, NoLock, Preserve)
> > {
> > - PRU, 8,
> > - PRD, 8,
> > + PRU, 16,
> > + PRD, 16,
> > }
> >
> > #define gen_processor(nr, name) \
> > Processor (CPU##name, nr, 0x0000b010, 0x06) { \
> > - Name (TMP, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0, 0x0})
> > \ + Name (PREN, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0,
> > 0x0}) \ + Name (PRDS, Buffer(0x8) {0x0, 0x8, nr, nr, 0x0, 0x0,
> > 0x0, 0x0}) \ Method(_MAT, 0) {
> > \ - If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(TMP) }
> > \ - Else { Return(0x0) }
> > \ + If (And(\_PR.PRU, ShiftLeft(1, nr))) {
> > Return(PREN) } \ + Else { Return(PRDS) }
> > \ }
> > \ Method (_STA) {
> > \ - Return(0xF)
> > \ + If (And(\_PR.PRU, ShiftLeft(1, nr))) {
> > Return(0xF) } \ + Else { Return(0x0) }
> > \ }
> > \ }
> > \
> >
> >
> > -
> > - Processor (CPU0, 0x00, 0x0000b010, 0x06) {Method (_STA) {
> > Return(0xF)}} + gen_processor(0, 0)
> > gen_processor(1, 1)
> > gen_processor(2, 2)
> > gen_processor(3, 3)
> > @@ -63,6 +64,28 @@ DefinitionBlock (
> > gen_processor(12, C)
> > gen_processor(13, D)
> > gen_processor(14, E)
> > +
> > + Method (NTFY, 2, NotSerialized) {
> > +#define gen_ntfy(nr) \
> > + If (LEqual(Arg0, 0x##nr)) { \
> > + Notify(CPU##nr, Arg1) \
> > + }
> > + gen_ntfy(0)
> > + gen_ntfy(1)
> > + gen_ntfy(2)
> > + gen_ntfy(3)
> > + gen_ntfy(4)
> > + gen_ntfy(5)
> > + gen_ntfy(6)
> > + gen_ntfy(7)
> > + gen_ntfy(8)
> > + gen_ntfy(9)
> > + gen_ntfy(A)
> > + gen_ntfy(B)
> > + gen_ntfy(C)
> > + gen_ntfy(D)
> > + gen_ntfy(E)
> > + }
> > }
> >
> > Scope (\)
> > @@ -666,33 +689,12 @@ DefinitionBlock (
> > Zero, /* reserved */
> > Zero /* reserved */
> > })
> > +
> > Scope (\_GPE)
> > {
> > -
> > -#define gen_cpu_hotplug(name, nr) \
> > - If (And(\_PR.PRU, ShiftLeft(1, nr))) { \
> > - Notify(\_PR.CPU##name, 1) \
> > - } \
> > - If (And(\_PR.PRD, ShiftLeft(1, nr))) { \
> > - Notify(\_PR.CPU##name, 3) \
> > - }
> > + Name(_HID, "ACPI0006")
> >
> > Method(_L00) {
> > - gen_cpu_hotplug(1, 1)
> > - gen_cpu_hotplug(2, 2)
> > - gen_cpu_hotplug(3, 3)
> > - gen_cpu_hotplug(4, 4)
> > - gen_cpu_hotplug(5, 5)
> > - gen_cpu_hotplug(6, 6)
> > - gen_cpu_hotplug(7, 7)
> > - gen_cpu_hotplug(8, 8)
> > - gen_cpu_hotplug(9, 9)
> > - gen_cpu_hotplug(A, 10)
> > - gen_cpu_hotplug(B, 11)
> > - gen_cpu_hotplug(C, 12)
> > - gen_cpu_hotplug(D, 13)
> > - gen_cpu_hotplug(E, 14)
> > -
> > Return(0x01)
> > }
> >
> > @@ -739,9 +741,29 @@ DefinitionBlock (
> >
> > Return(0x01)
> > }
> > +
> > Method(_L02) {
> > + Store(Zero, Local3)
> > + Store(\_PR.PRU, Local2)
> > + Xor(Local2, \_PR.PRD, Local0)
> > + Store(Local2, \_PR.PRD)
> > + Store(\_PR.PRD, Local1)
> > + While (LNotEqual (Local0, Zero)) {
> > + Store(ShiftLeft(1, Local3), Local1)
> > + If (And(Local0, Local1)) {
> > + Store(And(Local0, Not(Local1)), Local0)
> > + If (And(Local2, Local1)) {
> > + Store(1, Local4)
> > + } Else {
> > + Store(3, Local4)
> > + }
> > + \_PR.NTFY(Local3, Local4)
> > + }
> > + Increment(Local3)
> > + }
> > Return(0x01)
> > }
> > +
> > Method(_L03) {
> > Return(0x01)
> > }
> > diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
> > index b998225..337432a 100644
> > --- a/qemu/hw/acpi.c
> > +++ b/qemu/hw/acpi.c
> > @@ -578,8 +578,8 @@ void qemu_system_powerdown(void)
> > struct gpe_regs {
> > uint16_t sts; /* status */
> > uint16_t en; /* enabled */
> > - uint8_t up;
> > - uint8_t down;
> > + uint16_t cpus_sts;
> > + uint16_t bios_cpus_sts;
> > };
> >
> > struct pci_status {
> > @@ -590,29 +590,34 @@ struct pci_status {
> > static struct gpe_regs gpe;
> > static struct pci_status pci0_status;
> >
> > +static uint32_t gpe_read_val(uint16_t val, uint32_t addr)
> > +{
> > + if (addr & 1)
> > + return (val >> 8) & 0xff;
> > + return val & 0xff;
> > +}
> > +
> > static uint32_t gpe_readb(void *opaque, uint32_t addr)
> > {
> > uint32_t val = 0;
> > struct gpe_regs *g = opaque;
> > switch (addr) {
> > case PROC_BASE:
> > - val = g->up;
> > - break;
> > case PROC_BASE + 1:
> > - val = g->down;
> > + val = gpe_read_val(g->cpus_sts, addr);
> > + break;
> > + case PROC_BASE + 2:
> > + case PROC_BASE + 3:
> > + val = gpe_read_val(g->bios_cpus_sts, addr);
> > break;
> >
> > case GPE_BASE:
> > - val = g->sts & 0xFF;
> > - break;
> > case GPE_BASE + 1:
> > - val = (g->sts >> 8) & 0xFF;
> > + val = gpe_read_val(g->sts, addr);
> > break;
> > case GPE_BASE + 2:
> > - val = g->en & 0xFF;
> > - break;
> > case GPE_BASE + 3:
> > - val = (g->en >> 8) & 0xFF;
> > + val = gpe_read_val(g->en, addr);
> > break;
> > default:
> > break;
> > @@ -624,28 +629,45 @@ static uint32_t gpe_readb(void *opaque, uint32_t
> > addr) return val;
> > }
> >
> > +static uint16_t gpe_write_val(uint16_t cur, int addr, uint32_t val)
> > +{
> > + if (addr & 1)
> > + return (cur & 0xff) | (val << 8);
> > + return (cur & 0xff00) | (val & 0xff);
> > +}
> > +
> > +static uint16_t gpe_reset_val(uint16_t cur, int addr, uint32_t val)
> > +{
> > + uint16_t x1, x0 = val & 0xff;
> > + int shift = (addr & 1) ? 8 : 0;
> > +
> > + x1 = (cur >> shift) & 0xff;
> > +
> > + x1 = x1 & ~x0;
> > +
> > + return (cur & (0xff << (8 - shift))) | (x1 << shift);
> > +}
> > +
> > static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
> > {
> > struct gpe_regs *g = opaque;
> > switch (addr) {
> > case PROC_BASE:
> > - g->up = val;
> > - break;
> > case PROC_BASE + 1:
> > - g->down = val;
> > + /* don't allow to change cpu_sts from inside a guest */
> > + break;
> > + case PROC_BASE + 2:
> > + case PROC_BASE + 3:
> > + g->bios_cpus_sts = gpe_write_val(g->bios_cpus_sts, addr, val);
> > break;
> >
> > case GPE_BASE:
> > - g->sts = (g->sts & ~0xFFFF) | (val & 0xFFFF);
> > - break;
> > case GPE_BASE + 1:
> > - g->sts = (g->sts & 0xFFFF) | (val << 8);
> > + g->sts = gpe_reset_val(g->sts, addr, val);
> > break;
> > case GPE_BASE + 2:
> > - g->en = (g->en & ~0xFFFF) | (val & 0xFFFF);
> > - break;
> > case GPE_BASE + 3:
> > - g->en = (g->en & 0xFFFF) | (val << 8);
> > + g->en = gpe_write_val(g->en, addr, val);
> > break;
> > default:
> > break;
> > @@ -717,6 +739,7 @@ static const char *model;
> >
> > void qemu_system_hot_add_init(const char *cpu_model)
> > {
> > + gpe.bios_cpus_sts = gpe.cpus_sts = (1 << smp_cpus) - 1;
> > register_ioport_write(GPE_BASE, 4, 1, gpe_writeb, &gpe);
> > register_ioport_read(GPE_BASE, 4, 1, gpe_readb, &gpe);
> >
> > @@ -734,16 +757,14 @@ void qemu_system_hot_add_init(const char *cpu_model)
> >
> > static void enable_processor(struct gpe_regs *g, int cpu)
> > {
> > - g->sts |= 1;
> > - g->en |= 1;
> > - g->up |= (1 << cpu);
> > + g->sts |= 4;
> > + g->cpus_sts |= (1 << cpu);
> > }
> >
> > static void disable_processor(struct gpe_regs *g, int cpu)
> > {
> > - g->sts |= 1;
> > - g->en |= 1;
> > - g->down |= (1 << cpu);
> > + g->sts |= 4;
> > + g->cpus_sts &= ~(1 << cpu);
> > }
> >
> > #if defined(TARGET_I386) || defined(TARGET_X86_64)
> > @@ -784,39 +805,40 @@ void qemu_system_cpu_hot_add(int cpu, int state)
> > #endif
> > }
> >
> > - qemu_set_irq(pm_state->irq, 1);
> > - gpe.up = 0;
> > - gpe.down = 0;
> > if (state)
> > enable_processor(&gpe, cpu);
> > else
> > disable_processor(&gpe, cpu);
> > - qemu_set_irq(pm_state->irq, 0);
> > + if (gpe.en & 4) {
> > + qemu_set_irq(pm_state->irq, 1);
> > + qemu_set_irq(pm_state->irq, 0);
> > + }
> > }
> > #endif
> >
> > static void enable_device(struct pci_status *p, struct gpe_regs *g, int
> > slot) {
> > g->sts |= 2;
> > - g->en |= 2;
> > p->up |= (1 << slot);
> > }
> >
> > static void disable_device(struct pci_status *p, struct gpe_regs *g, int
> > slot) {
> > g->sts |= 2;
> > - g->en |= 2;
> > p->down |= (1 << slot);
> > }
> >
> > void qemu_system_device_hot_add(int pcibus, int slot, int state)
> > {
> > - qemu_set_irq(pm_state->irq, 1);
> > pci0_status.up = 0;
> > pci0_status.down = 0;
> > if (state)
> > enable_device(&pci0_status, &gpe, slot);
> > else
> > disable_device(&pci0_status, &gpe, slot);
> > - qemu_set_irq(pm_state->irq, 0);
> > +
> > + if (gpe.en & 2) {
> > + qemu_set_irq(pm_state->irq, 1);
> > + qemu_set_irq(pm_state->irq, 0);
> > + }
> > }
> > --
> > Gleb.
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] various fixes for CPU/PCI hotplug
2009-02-03 14:01 [PATCH] various fixes for CPU/PCI hotplug Gleb Natapov
2009-02-03 14:46 ` dima
@ 2009-02-03 16:43 ` Marcelo Tosatti
2009-02-03 16:47 ` Gleb Natapov
2009-02-03 18:58 ` Glauber Costa
2 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2009-02-03 16:43 UTC (permalink / raw)
To: Gleb Natapov; +Cc: avi, glommer, kvm
On Tue, Feb 03, 2009 at 04:01:29PM +0200, Gleb Natapov wrote:
> Hi,
>
> The fixed problems are:
> 1) Disabled processor _STA method should return 0 (this fixes Vista's
> resume after hibernate problem)
> 2) Disabled processor _MAT method should return disabled MADT entry
> instead of 0
> 3) Fix number of hot pluggable CPUs to be 16
Breaks Win2k. See commit 18dcd6be77864842b42e3d4f88215a6832d1f01c.
> 4) Properly handle write to GPE STS register (write 1 should clear bit)
> 5) Generate interrupt only if corespondent EN bit is set
> 5) Use reserved STS bits from PIIX4 chipset to avoid clash in the
> future.
Can you please split the patch in cpu / non cpu parts?
Looks good to me (the non cpu stuff). Glauber, can you ack the cpu hotplug ?
Thanks
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] various fixes for CPU/PCI hotplug
2009-02-03 16:43 ` Marcelo Tosatti
@ 2009-02-03 16:47 ` Gleb Natapov
0 siblings, 0 replies; 8+ messages in thread
From: Gleb Natapov @ 2009-02-03 16:47 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: avi, glommer, kvm
On Tue, Feb 03, 2009 at 02:43:46PM -0200, Marcelo Tosatti wrote:
> On Tue, Feb 03, 2009 at 04:01:29PM +0200, Gleb Natapov wrote:
> > Hi,
> >
> > The fixed problems are:
> > 1) Disabled processor _STA method should return 0 (this fixes Vista's
> > resume after hibernate problem)
> > 2) Disabled processor _MAT method should return disabled MADT entry
> > instead of 0
> > 3) Fix number of hot pluggable CPUs to be 16
>
> Breaks Win2k. See commit 18dcd6be77864842b42e3d4f88215a6832d1f01c.
>
Actually 3 should be "Fix number of hot pluggable CPUs to be 15" :)
What I mean is that I extended bitmask that we use to pass CPU hotplug
event from 8 bits to 16 bits. I didn't add one more processor object to
ACPI.
> > 4) Properly handle write to GPE STS register (write 1 should clear bit)
> > 5) Generate interrupt only if corespondent EN bit is set
> > 5) Use reserved STS bits from PIIX4 chipset to avoid clash in the
> > future.
>
> Can you please split the patch in cpu / non cpu parts?
>
OK.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] various fixes for CPU/PCI hotplug
2009-02-03 14:01 [PATCH] various fixes for CPU/PCI hotplug Gleb Natapov
2009-02-03 14:46 ` dima
2009-02-03 16:43 ` Marcelo Tosatti
@ 2009-02-03 18:58 ` Glauber Costa
2009-02-03 19:11 ` Marcelo Tosatti
2009-02-03 20:27 ` Gleb Natapov
2 siblings, 2 replies; 8+ messages in thread
From: Glauber Costa @ 2009-02-03 18:58 UTC (permalink / raw)
To: Gleb Natapov; +Cc: avi, glommer, kvm
>
> void qemu_system_device_hot_add(int pcibus, int slot, int state)
> {
> - qemu_set_irq(pm_state->irq, 1);
> pci0_status.up = 0;
> pci0_status.down = 0;
> if (state)
> enable_device(&pci0_status, &gpe, slot);
> else
> disable_device(&pci0_status, &gpe, slot);
> - qemu_set_irq(pm_state->irq, 0);
> +
> + if (gpe.en & 2) {
> + qemu_set_irq(pm_state->irq, 1);
> + qemu_set_irq(pm_state->irq, 0);
> + }
> }
Are you sure this works? The trigger is now happening after all the
events take place. It might well work depending on how qemu exposes
this, but I have some spare memories of having problems with this in
the past.
--
Glauber Costa.
"Free as in Freedom"
http://glommer.net
"The less confident you are, the more serious you have to act."
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] various fixes for CPU/PCI hotplug
2009-02-03 18:58 ` Glauber Costa
@ 2009-02-03 19:11 ` Marcelo Tosatti
2009-02-03 20:27 ` Gleb Natapov
1 sibling, 0 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2009-02-03 19:11 UTC (permalink / raw)
To: Glauber Costa; +Cc: Gleb Natapov, avi, glommer, kvm
On Tue, Feb 03, 2009 at 04:58:31PM -0200, Glauber Costa wrote:
> >
> > void qemu_system_device_hot_add(int pcibus, int slot, int state)
> > {
> > - qemu_set_irq(pm_state->irq, 1);
> > pci0_status.up = 0;
> > pci0_status.down = 0;
> > if (state)
> > enable_device(&pci0_status, &gpe, slot);
> > else
> > disable_device(&pci0_status, &gpe, slot);
> > - qemu_set_irq(pm_state->irq, 0);
> > +
> > + if (gpe.en & 2) {
> > + qemu_set_irq(pm_state->irq, 1);
> > + qemu_set_irq(pm_state->irq, 0);
> > + }
> > }
>
> Are you sure this works? The trigger is now happening after all the
> events take place. It might well work depending on how qemu exposes
> this, but I have some spare memories of having problems with this in
> the past.
The ACPI code will read the up/down status after the IRQ is triggered
(and nothing will change it, in the meantime). What is your concern?
The OS is responsible for programming the GPE enable bit.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] various fixes for CPU/PCI hotplug
2009-02-03 18:58 ` Glauber Costa
2009-02-03 19:11 ` Marcelo Tosatti
@ 2009-02-03 20:27 ` Gleb Natapov
1 sibling, 0 replies; 8+ messages in thread
From: Gleb Natapov @ 2009-02-03 20:27 UTC (permalink / raw)
To: Glauber Costa; +Cc: avi, glommer, kvm
On Tue, Feb 03, 2009 at 04:58:31PM -0200, Glauber Costa wrote:
> >
> > void qemu_system_device_hot_add(int pcibus, int slot, int state)
> > {
> > - qemu_set_irq(pm_state->irq, 1);
> > pci0_status.up = 0;
> > pci0_status.down = 0;
> > if (state)
> > enable_device(&pci0_status, &gpe, slot);
> > else
> > disable_device(&pci0_status, &gpe, slot);
> > - qemu_set_irq(pm_state->irq, 0);
> > +
> > + if (gpe.en & 2) {
> > + qemu_set_irq(pm_state->irq, 1);
> > + qemu_set_irq(pm_state->irq, 0);
> > + }
> > }
>
> Are you sure this works? The trigger is now happening after all the
> events take place. It might well work depending on how qemu exposes
> this, but I have some spare memories of having problems with this in
> the past.
>
>From my limited testing it works and I don't see why it shoudn't. What
do you think can go wrong? Actually triggering an interrupt before
programming status looks suspicious to me.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-02-03 20:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-02-03 14:01 [PATCH] various fixes for CPU/PCI hotplug Gleb Natapov
2009-02-03 14:46 ` dima
2009-02-03 15:13 ` Gleb Natapov
2009-02-03 16:43 ` Marcelo Tosatti
2009-02-03 16:47 ` Gleb Natapov
2009-02-03 18:58 ` Glauber Costa
2009-02-03 19:11 ` Marcelo Tosatti
2009-02-03 20:27 ` Gleb Natapov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox