| Message ID | 20090203140129.GC30234@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 -- 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
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. -- 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
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 -- 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
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. -- 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
> > 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.
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. -- 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
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. -- 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
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); + } }
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> -- 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