[Qemu-devel] [PATCH 0/2] CPU hotplug support

[Qemu-devel] [PATCH 0/2] CPU hotplug support

[Glauber Costa]

Hi,

Hotplug is not very popular these days, due to global warming concerns.
However, it is still important, and we have support from it in kvm-userspace,
and getting slowly being merged into qemu.

Part of it is done in the bios, so.. here we go! I'm sending it where it belongs

Enjoy

[Qemu-devel] [PATCH 1/2] create acpi cpu definitions

[Glauber Costa]

This comes directly from kvm-userspace. It creates
the necessary infrastructure for cpu hotplug, by
creating _MAT and _STA entries in cpu devices,
and by allowing notifications to the guest to happen

note that now, we have to fill acpi tables with MAX_CPUS,
instead of smp_cpus, otherwise we'll never be able to grow
the cpu number.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 bios/acpi-dsdt.dsl |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 bios/rombios.h     |    2 +
 bios/rombios32.c   |   72 +++++-------------------------
 3 files changed, 141 insertions(+), 61 deletions(-)

diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
index d35886d..9376179 100644
--- a/bios/acpi-dsdt.dsl
+++ b/bios/acpi-dsdt.dsl
@@ -25,6 +25,120 @@ DefinitionBlock (
     0x1                 // OEM Revision
     )
 {
+   Scope (\_PR)
+   {
+	OperationRegion(PRST, SystemIO, 0xaf00, 32)
+	Field (PRST, ByteAcc, NoLock, Preserve)
+	{
+		PRS, 256
+	}
+
+	Name(PRSS, Buffer(32){}) /* shadow CPU status bitmask */
+	Name(SSVL, 0)
+
+	Method(CRST, 1) {
+		If (LEqual(SSVL, 0)) {
+			Store(PRS, PRSS) /* read CPUs status bitmaks from HW */
+			Store(1, SSVL)
+                }
+		ShiftRight(Arg0, 3, Local1)
+		Store(DerefOf(Index(PRSS, Local1)), Local2)
+	        Return(And(Local2, ShiftLeft(1, And(Arg0, 0x7))))
+	}
+
+#define gen_processor(nr, name) 				            \
+	Processor (CPU##name, nr, 0x0000b010, 0x06) {                       \
+            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 (CRST(nr)) { Return(PREN) }                              \
+                Else { Return(PRDS) }                                       \
+            }                                                               \
+            Method (_STA) {                                                 \
+                If (CRST(nr)) { Return(0xF) }                               \
+                Else { Return(0x9) }                                        \
+            }                                                               \
+        }                                                                   \
+
+
+	gen_processor(0, 0)
+	gen_processor(1, 1)
+	gen_processor(2, 2)
+	gen_processor(3, 3)
+	gen_processor(4, 4)
+	gen_processor(5, 5)
+	gen_processor(6, 6)
+	gen_processor(7, 7)
+	gen_processor(8, 8)
+	gen_processor(9, 9)
+	gen_processor(10, A)
+	gen_processor(11, B)
+	gen_processor(12, C)
+	gen_processor(13, D)
+	gen_processor(14, E)
+
+	Method (NTFY, 2) {
+#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)
+		Return(One)
+	}
+
+	/* Works on 8 bit quentity.
+         * Arg1 - Shadow status bits
+         * Arg2 - Current status bits
+	 */
+        Method(PR1, 3) {
+	    Xor(Arg1, Arg2, Local0) /* figure out what chaged */
+	    ShiftLeft(Arg0, 3, Local1)
+            While (LNotEqual(Local0, Zero)) {
+		If (And(Local0, 1)) {      /* if staus have changed */
+                    if(And(Arg2, 1)) {     /* check previous status */
+	                Store(3, Local3)
+		    } Else {
+	                Store(1, Local3)
+	            }
+		    NTFY(Local1, Local3)
+                }
+		ShiftRight(Local0, 1, Local0)
+		ShiftRight(Arg2, 1, Arg2)
+		Increment(Local1)
+	    }
+	    Return(One)
+	}
+
+	Method(PRSC, 0) {
+		Store(Buffer(32){}, Local0)
+		Store(PRS, Local0) /* read CPUs status bitmask into Local0 */
+		Store(Zero, Local1)
+		/* loop over bitmask byte by byte to see what have chaged */
+		While(LLess(Local1, 32)) {
+			Store(DerefOf(Index(Local0, Local1)), Local2)
+			Store(DerefOf(Index(PRSS, Local1)), Local3)
+			PR1(Local1, Local2, Local3)
+			Increment(Local1)
+                }
+		Store(Local0, PRSS) /* store curr satust bitmask into shadow */
+		Return(One)
+	}
+    }
+
     Scope (\)
     {
         /* Debug Output */
@@ -591,4 +705,18 @@ DefinitionBlock (
         Zero,  /* reserved */
         Zero   /* reserved */
     })
+    Scope (\_GPE)
+    {
+        Name(_HID, "ACPI0006")
+
+        Method(_L00) {
+            Return(0x01)
+        }
+        Method(_L01) {
+            Return(0x01)
+        }
+        Method(_L02) {
+            Return(\_PR.PRSC())
+        }
+    }
 }
diff --git a/bios/rombios.h b/bios/rombios.h
index 361b958..07ace35 100644
--- a/bios/rombios.h
+++ b/bios/rombios.h
@@ -58,6 +58,8 @@
 #define SMB_IO_BASE       0xb100
 #define SMP_MSR_ADDR      0x0510
 
+#define MAX_CPUS	256
+
   // Define the application NAME
 #if defined(BX_QEMU)
 #  define BX_APPNAME "QEMU"
diff --git a/bios/rombios32.c b/bios/rombios32.c
index 2b084b2..3c7757b 100644
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -1098,20 +1098,22 @@ static void mptable_init(void)
     putstr(&q, "0.1         "); /* vendor id */
     putle32(&q, 0); /* OEM table ptr */
     putle16(&q, 0); /* OEM table size */
-    putle16(&q, smp_cpus + 18); /* entry count */
+    putle16(&q, MAX_CPUS + 18); /* entry count */
     putle32(&q, 0xfee00000); /* local APIC addr */
     putle16(&q, 0); /* ext table length */
     putb(&q, 0); /* ext table checksum */
     putb(&q, 0); /* reserved */
 
-    for(i = 0; i < smp_cpus; i++) {
+    for(i = 0; i < MAX_CPUS ; i++) {
         putb(&q, 0); /* entry type = processor */
         putb(&q, i); /* APIC id */
         putb(&q, 0x11); /* local APIC version number */
         if (i == 0)
             putb(&q, 3); /* cpu flags: enabled, bootstrap cpu */
-        else
+        else if (i < smp_cpus)
             putb(&q, 1); /* cpu flags: enabled */
+	else
+            putb(&q, 1); /* cpu flags: disabled */
         if (cpuid_signature) {
             putle32(&q, cpuid_signature);
             putle32(&q, cpuid_features);
@@ -1484,57 +1486,6 @@ static void acpi_build_table_header(struct acpi_table_header *h,
     h->checksum = acpi_checksum((void *)h, len);
 }
 
-int acpi_build_processor_ssdt(uint8_t *ssdt)
-{
-    uint8_t *ssdt_ptr = ssdt;
-    int i, length;
-    int acpi_cpus = smp_cpus > 0xff ? 0xff : smp_cpus;
-
-    ssdt_ptr[9] = 0; // checksum;
-    ssdt_ptr += sizeof(struct acpi_table_header);
-
-    // caluculate the length of processor block and scope block excluding PkgLength
-    length = 0x0d * acpi_cpus + 4;
-
-    // build processor scope header
-    *(ssdt_ptr++) = 0x10; // ScopeOp
-    if (length <= 0x3e) {
-        *(ssdt_ptr++) = length + 1;
-    } else {
-        *(ssdt_ptr++) = 0x7F;
-        *(ssdt_ptr++) = (length + 2) >> 6;
-    }
-    *(ssdt_ptr++) = '_'; // Name
-    *(ssdt_ptr++) = 'P';
-    *(ssdt_ptr++) = 'R';
-    *(ssdt_ptr++) = '_';
-
-    // build object for each processor
-    for(i=0;i<acpi_cpus;i++) {
-        *(ssdt_ptr++) = 0x5B; // ProcessorOp
-        *(ssdt_ptr++) = 0x83;
-        *(ssdt_ptr++) = 0x0B; // Length
-        *(ssdt_ptr++) = 'C';  // Name (CPUxx)
-        *(ssdt_ptr++) = 'P';
-        if ((i & 0xf0) != 0)
-            *(ssdt_ptr++) = (i >> 4) < 0xa ? (i >> 4) + '0' : (i >> 4) + 'A' - 0xa;
-        else
-            *(ssdt_ptr++) = 'U';
-        *(ssdt_ptr++) = (i & 0xf) < 0xa ? (i & 0xf) + '0' : (i & 0xf) + 'A' - 0xa;
-        *(ssdt_ptr++) = i;
-        *(ssdt_ptr++) = 0x10; // Processor block address
-        *(ssdt_ptr++) = 0xb0;
-        *(ssdt_ptr++) = 0;
-        *(ssdt_ptr++) = 0;
-        *(ssdt_ptr++) = 6;    // Processor block length
-    }
-
-    acpi_build_table_header((struct acpi_table_header *)ssdt,
-                            "SSDT", ssdt_ptr - ssdt, 1);
-
-    return ssdt_ptr - ssdt;
-}
-
 /* base_addr must be a multiple of 4KB */
 void acpi_bios_init(void)
 {
@@ -1582,14 +1533,10 @@ void acpi_bios_init(void)
     dsdt = (void *)(addr);
     addr += sizeof(AmlCode);
 
-    ssdt_addr = addr;
-    ssdt = (void *)(addr);
-    addr += acpi_build_processor_ssdt(ssdt);
-
     addr = (addr + 7) & ~7;
     madt_addr = addr;
     madt_size = sizeof(*madt) +
-        sizeof(struct madt_processor_apic) * smp_cpus +
+        sizeof(struct madt_processor_apic) * MAX_CPUS +
 #ifdef BX_QEMU
         sizeof(struct madt_io_apic) + sizeof(struct madt_int_override);
 #else
@@ -1677,12 +1624,15 @@ void acpi_bios_init(void)
         madt->local_apic_address = cpu_to_le32(0xfee00000);
         madt->flags = cpu_to_le32(1);
         apic = (void *)(madt + 1);
-        for(i=0;i<smp_cpus;i++) {
+        for(i=0;i< MAX_CPUS;i++) {
             apic->type = APIC_PROCESSOR;
             apic->length = sizeof(*apic);
             apic->processor_id = i;
             apic->local_apic_id = i;
-            apic->flags = cpu_to_le32(1);
+            if (i < smp_cpus)
+                apic->flags = cpu_to_le32(1);
+            else
+                apic->flags = 0;
             apic++;
         }
         io_apic = (void *)apic;
-- 
1.5.6.6

[Qemu-devel] [PATCH 2/2] kvm: bios: remove acpi_build_processor_ssdt

[Glauber Costa]

From: Glauber Costa <gcosta@redhat.com>

It is now present in the dsdt, provided by acpi-dst.dsl, so having it
in here confuses things.

Signed-off-by: Glauber Costa <gcosta@redhat.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 bios/rombios32.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 3c7757b..cf6610d 100644
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -1251,9 +1251,9 @@ struct rsdt_descriptor_rev1
 {
 	ACPI_TABLE_HEADER_DEF                           /* ACPI common table header */
 #ifdef BX_QEMU
-	uint32_t                             table_offset_entry [4]; /* Array of pointers to other */
-#else
 	uint32_t                             table_offset_entry [3]; /* Array of pointers to other */
+#else
+	uint32_t                             table_offset_entry [2]; /* Array of pointers to other */
 #endif
 			 /* ACPI tables */
 } __attribute__((__packed__));
@@ -1494,12 +1494,12 @@ void acpi_bios_init(void)
     struct fadt_descriptor_rev1 *fadt;
     struct facs_descriptor_rev1 *facs;
     struct multiple_apic_table *madt;
-    uint8_t *dsdt, *ssdt;
+    uint8_t *dsdt;
 #ifdef BX_QEMU
     struct acpi_20_hpet *hpet;
     uint32_t hpet_addr;
 #endif
-    uint32_t base_addr, rsdt_addr, fadt_addr, addr, facs_addr, dsdt_addr, ssdt_addr;
+    uint32_t base_addr, rsdt_addr, fadt_addr, addr, facs_addr, dsdt_addr;
     uint32_t acpi_tables_size, madt_addr, madt_size;
     int i;
 
@@ -1573,9 +1573,8 @@ void acpi_bios_init(void)
     memset(rsdt, 0, sizeof(*rsdt));
     rsdt->table_offset_entry[0] = cpu_to_le32(fadt_addr);
     rsdt->table_offset_entry[1] = cpu_to_le32(madt_addr);
-    rsdt->table_offset_entry[2] = cpu_to_le32(ssdt_addr);
 #ifdef BX_QEMU
-    rsdt->table_offset_entry[3] = cpu_to_le32(hpet_addr);
+    rsdt->table_offset_entry[2] = cpu_to_le32(hpet_addr);
 #endif
     acpi_build_table_header((struct acpi_table_header *)rsdt,
                             "RSDT", sizeof(*rsdt), 1);
-- 
1.5.6.6

[Qemu-devel] Re: [Bochs-developers] [PATCH 1/2] create acpi cpu definitions

[Sebastian Herbszt]

Glauber Costa wrote:
> This comes directly from kvm-userspace. It creates
> the necessary infrastructure for cpu hotplug, by
> creating _MAT and _STA entries in cpu devices,
> and by allowing notifications to the guest to happen

Is there a cpu hotplug specification? I would like to read up
on the needed changes.

> note that now, we have to fill acpi tables with MAX_CPUS,
> instead of smp_cpus, otherwise we'll never be able to grow
> the cpu number.

Looking at the mptable_init() change below this also applys to
mptable, right?

> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
> bios/acpi-dsdt.dsl |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> bios/rombios.h     |    2 +
> bios/rombios32.c   |   72 +++++-------------------------
> 3 files changed, 141 insertions(+), 61 deletions(-)
> 
> diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
> index d35886d..9376179 100644
> --- a/bios/acpi-dsdt.dsl
> +++ b/bios/acpi-dsdt.dsl
> @@ -25,6 +25,120 @@ DefinitionBlock (
>     0x1                 // OEM Revision
>     )
> {
> +   Scope (\_PR)
> +   {
> + OperationRegion(PRST, SystemIO, 0xaf00, 32)

At least Bochs doesn't have anything listening on port 0xaf00.
Can you please #ifdef BX_QEMU at least some of the changes?

> + Field (PRST, ByteAcc, NoLock, Preserve)
> + {
> + PRS, 256
> + }
> +
> + Name(PRSS, Buffer(32){}) /* shadow CPU status bitmask */
> + Name(SSVL, 0)
> +
> + Method(CRST, 1) {
> + If (LEqual(SSVL, 0)) {
> + Store(PRS, PRSS) /* read CPUs status bitmaks from HW */
> + Store(1, SSVL)
> +                }
> + ShiftRight(Arg0, 3, Local1)
> + Store(DerefOf(Index(PRSS, Local1)), Local2)
> +         Return(And(Local2, ShiftLeft(1, And(Arg0, 0x7))))
> + }
> +
> +#define gen_processor(nr, name)             \
> + Processor (CPU##name, nr, 0x0000b010, 0x06) {                       \
> +            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 (CRST(nr)) { Return(PREN) }                              \
> +                Else { Return(PRDS) }                                       \
> +            }                                                               \
> +            Method (_STA) {                                                 \
> +                If (CRST(nr)) { Return(0xF) }                               \
> +                Else { Return(0x9) }                                        \
> +            }                                                               \
> +        }                                                                   \
> +
> +
> + gen_processor(0, 0)
> + gen_processor(1, 1)
> + gen_processor(2, 2)
> + gen_processor(3, 3)
> + gen_processor(4, 4)
> + gen_processor(5, 5)
> + gen_processor(6, 6)
> + gen_processor(7, 7)
> + gen_processor(8, 8)
> + gen_processor(9, 9)
> + gen_processor(10, A)
> + gen_processor(11, B)
> + gen_processor(12, C)
> + gen_processor(13, D)
> + gen_processor(14, E)
> +
> + Method (NTFY, 2) {
> +#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)
> + Return(One)
> + }
> +
> + /* Works on 8 bit quentity.
> +         * Arg1 - Shadow status bits
> +         * Arg2 - Current status bits
> + */

quentity? quantity?

> +        Method(PR1, 3) {
> +     Xor(Arg1, Arg2, Local0) /* figure out what chaged */
> +     ShiftLeft(Arg0, 3, Local1)
> +            While (LNotEqual(Local0, Zero)) {
> + If (And(Local0, 1)) {      /* if staus have changed */
> +                    if(And(Arg2, 1)) {     /* check previous status */
> +                 Store(3, Local3)
> +     } Else {
> +                 Store(1, Local3)
> +             }
> +     NTFY(Local1, Local3)
> +                }
> + ShiftRight(Local0, 1, Local0)
> + ShiftRight(Arg2, 1, Arg2)
> + Increment(Local1)
> +     }
> +     Return(One)
> + }
> +
> + Method(PRSC, 0) {
> + Store(Buffer(32){}, Local0)
> + Store(PRS, Local0) /* read CPUs status bitmask into Local0 */
> + Store(Zero, Local1)
> + /* loop over bitmask byte by byte to see what have chaged */
> + While(LLess(Local1, 32)) {
> + Store(DerefOf(Index(Local0, Local1)), Local2)
> + Store(DerefOf(Index(PRSS, Local1)), Local3)
> + PR1(Local1, Local2, Local3)
> + Increment(Local1)
> +                }
> + Store(Local0, PRSS) /* store curr satust bitmask into shadow */
> + Return(One)
> + }
> +    }
> +
>     Scope (\)
>     {
>         /* Debug Output */
> @@ -591,4 +705,18 @@ DefinitionBlock (
>         Zero,  /* reserved */
>         Zero   /* reserved */
>     })
> +    Scope (\_GPE)
> +    {
> +        Name(_HID, "ACPI0006")
> +
> +        Method(_L00) {
> +            Return(0x01)
> +        }
> +        Method(_L01) {
> +            Return(0x01)
> +        }
> +        Method(_L02) {
> +            Return(\_PR.PRSC())
> +        }
> +    }
> }
> diff --git a/bios/rombios.h b/bios/rombios.h
> index 361b958..07ace35 100644
> --- a/bios/rombios.h
> +++ b/bios/rombios.h
> @@ -58,6 +58,8 @@
> #define SMB_IO_BASE       0xb100
> #define SMP_MSR_ADDR      0x0510
> 
> +#define MAX_CPUS 256
> +
>   // Define the application NAME
> #if defined(BX_QEMU)
> #  define BX_APPNAME "QEMU"
> diff --git a/bios/rombios32.c b/bios/rombios32.c
> index 2b084b2..3c7757b 100644
> --- a/bios/rombios32.c
> +++ b/bios/rombios32.c
> @@ -1098,20 +1098,22 @@ static void mptable_init(void)
>     putstr(&q, "0.1         "); /* vendor id */
>     putle32(&q, 0); /* OEM table ptr */
>     putle16(&q, 0); /* OEM table size */
> -    putle16(&q, smp_cpus + 18); /* entry count */
> +    putle16(&q, MAX_CPUS + 18); /* entry count */

A system might not have MAX_CPUS available, but we set all
processor entries to "enabled" below.

>     putle32(&q, 0xfee00000); /* local APIC addr */
>     putle16(&q, 0); /* ext table length */
>     putb(&q, 0); /* ext table checksum */
>     putb(&q, 0); /* reserved */
> 
> -    for(i = 0; i < smp_cpus; i++) {
> +    for(i = 0; i < MAX_CPUS ; i++) {
>         putb(&q, 0); /* entry type = processor */
>         putb(&q, i); /* APIC id */
>         putb(&q, 0x11); /* local APIC version number */
>         if (i == 0)
>             putb(&q, 3); /* cpu flags: enabled, bootstrap cpu */
> -        else
> +        else if (i < smp_cpus)
>             putb(&q, 1); /* cpu flags: enabled */
> + else
> +            putb(&q, 1); /* cpu flags: disabled */

Do we have to distinguish those two? Both set "cpu flags: en" to 1.
The comment saying "disabled" is somewhat misleading since the cpu flag
is set to enabled.

>         if (cpuid_signature) {
>             putle32(&q, cpuid_signature);
>             putle32(&q, cpuid_features);
> @@ -1484,57 +1486,6 @@ static void acpi_build_table_header(struct acpi_table_header *h,
>     h->checksum = acpi_checksum((void *)h, len);
> }
> 
> -int acpi_build_processor_ssdt(uint8_t *ssdt)
> -{
> -    uint8_t *ssdt_ptr = ssdt;
> -    int i, length;
> -    int acpi_cpus = smp_cpus > 0xff ? 0xff : smp_cpus;
> -
> -    ssdt_ptr[9] = 0; // checksum;
> -    ssdt_ptr += sizeof(struct acpi_table_header);
> -
> -    // caluculate the length of processor block and scope block excluding PkgLength
> -    length = 0x0d * acpi_cpus + 4;
> -
> -    // build processor scope header
> -    *(ssdt_ptr++) = 0x10; // ScopeOp
> -    if (length <= 0x3e) {
> -        *(ssdt_ptr++) = length + 1;
> -    } else {
> -        *(ssdt_ptr++) = 0x7F;
> -        *(ssdt_ptr++) = (length + 2) >> 6;
> -    }
> -    *(ssdt_ptr++) = '_'; // Name
> -    *(ssdt_ptr++) = 'P';
> -    *(ssdt_ptr++) = 'R';
> -    *(ssdt_ptr++) = '_';
> -
> -    // build object for each processor
> -    for(i=0;i<acpi_cpus;i++) {
> -        *(ssdt_ptr++) = 0x5B; // ProcessorOp
> -        *(ssdt_ptr++) = 0x83;
> -        *(ssdt_ptr++) = 0x0B; // Length
> -        *(ssdt_ptr++) = 'C';  // Name (CPUxx)
> -        *(ssdt_ptr++) = 'P';
> -        if ((i & 0xf0) != 0)
> -            *(ssdt_ptr++) = (i >> 4) < 0xa ? (i >> 4) + '0' : (i >> 4) + 'A' - 0xa;
> -        else
> -            *(ssdt_ptr++) = 'U';
> -        *(ssdt_ptr++) = (i & 0xf) < 0xa ? (i & 0xf) + '0' : (i & 0xf) + 'A' - 0xa;
> -        *(ssdt_ptr++) = i;
> -        *(ssdt_ptr++) = 0x10; // Processor block address
> -        *(ssdt_ptr++) = 0xb0;
> -        *(ssdt_ptr++) = 0;
> -        *(ssdt_ptr++) = 0;
> -        *(ssdt_ptr++) = 6;    // Processor block length
> -    }
> -
> -    acpi_build_table_header((struct acpi_table_header *)ssdt,
> -                            "SSDT", ssdt_ptr - ssdt, 1);
> -
> -    return ssdt_ptr - ssdt;
> -}
> -
> /* base_addr must be a multiple of 4KB */
> void acpi_bios_init(void)
> {
> @@ -1582,14 +1533,10 @@ void acpi_bios_init(void)
>     dsdt = (void *)(addr);
>     addr += sizeof(AmlCode);
> 
> -    ssdt_addr = addr;
> -    ssdt = (void *)(addr);
> -    addr += acpi_build_processor_ssdt(ssdt);
> -
>     addr = (addr + 7) & ~7;
>     madt_addr = addr;
>     madt_size = sizeof(*madt) +
> -        sizeof(struct madt_processor_apic) * smp_cpus +
> +        sizeof(struct madt_processor_apic) * MAX_CPUS +
> #ifdef BX_QEMU
>         sizeof(struct madt_io_apic) + sizeof(struct madt_int_override);
> #else
> @@ -1677,12 +1624,15 @@ void acpi_bios_init(void)
>         madt->local_apic_address = cpu_to_le32(0xfee00000);
>         madt->flags = cpu_to_le32(1);
>         apic = (void *)(madt + 1);
> -        for(i=0;i<smp_cpus;i++) {
> +        for(i=0;i< MAX_CPUS;i++) {
>             apic->type = APIC_PROCESSOR;
>             apic->length = sizeof(*apic);
>             apic->processor_id = i;
>             apic->local_apic_id = i;
> -            apic->flags = cpu_to_le32(1);
> +            if (i < smp_cpus)
> +                apic->flags = cpu_to_le32(1);
> +            else
> +                apic->flags = 0;
>             apic++;
>         }
>         io_apic = (void *)apic;

CPU entries below smp_cpus are set to "enabled" others to "unusable".
This differs from the mptable where all cpus are set to "enabled". Should 
mptable_init() be changed accordingly?

- Sebastian

[Qemu-devel] Re: [Bochs-developers] [PATCH 1/2] create acpi cpu definitions

[Brendan Trotter]

Hi,

On Sun, Apr 19, 2009 at 7:20 AM, Sebastian Herbszt <herbszt@gmx.de> wrote:
> Glauber Costa wrote:
>> This comes directly from kvm-userspace. It creates
>> the necessary infrastructure for cpu hotplug, by
>> creating _MAT and _STA entries in cpu devices,
>> and by allowing notifications to the guest to happen
>
> Is there a cpu hotplug specification? I would like to read up
> on the needed changes.

There isn't any CPU hotplug specification for 80x86.

The "Processor Local APIC" structure in ACPI's tables has an
"enabled/disabled" flag (just like some other structures in ACPI
tables). Hotplug CPUs that aren't present may not be listed at all (no
"disabled" entry), and "Processor Local APIC" entries may be disabled
for any number of other reasons (including a BIOS that uses fixed size
tables, that supports more CPUs than the motherboard). The only thing
an OS can assume about disabled "Processor Local APIC" entries is that
the OS must not attempt to use the CPU.

For a comparison, the ACPI specification does include support for
hotplug RAM. In this case (for ACPI 3.0) the "int 0x15, eax = 0xE820"
BIOS function returns entries with an enabled/disabled flag where
"disabled" entries must be ignored; and there's a completely separate
structure (the "Memory Affinity Structure") which contains information
about areas that are used for hot-plug RAM, which has it's one
enabled/disabled flag *and* a separate hotpluggable/not hotpluggable
flag. From this, it seems logical that if ACPI ever does support
hot-plug CPUs, then they'll use a separate structure or a separate
flag to indicate if a CPU is hot pluggable or not, and the existing
"enabled/disabled" flag will retain it's current (use/don't use)
meaning.

For some reason (unknown to me) some Linux developers made wild
assumptions about disabled "Processor Local APIC" entries, and now
they're inventing fictitious hardware to support their unfounded
assumptions.

Please, correct me if I'm wrong...


Cheers,

Brendan

[Qemu-devel] Re: [Bochs-developers] [PATCH 1/2] create acpi cpu definitions

[Gleb Natapov]

On Sun, Apr 19, 2009 at 11:01:09AM +0930, Brendan Trotter wrote:
> Hi,
> 
> On Sun, Apr 19, 2009 at 7:20 AM, Sebastian Herbszt <herbszt@gmx.de> wrote:
> > Glauber Costa wrote:
> >> This comes directly from kvm-userspace. It creates
> >> the necessary infrastructure for cpu hotplug, by
> >> creating _MAT and _STA entries in cpu devices,
> >> and by allowing notifications to the guest to happen
> >
> > Is there a cpu hotplug specification? I would like to read up
> > on the needed changes.
> 
> There isn't any CPU hotplug specification for 80x86.
> 
> The "Processor Local APIC" structure in ACPI's tables has an
> "enabled/disabled" flag (just like some other structures in ACPI
> tables). Hotplug CPUs that aren't present may not be listed at all (no
> "disabled" entry), and "Processor Local APIC" entries may be disabled
> for any number of other reasons (including a BIOS that uses fixed size
> tables, that supports more CPUs than the motherboard). The only thing
> an OS can assume about disabled "Processor Local APIC" entries is that
> the OS must not attempt to use the CPU.
> 
> For a comparison, the ACPI specification does include support for
> hotplug RAM. In this case (for ACPI 3.0) the "int 0x15, eax = 0xE820"
> BIOS function returns entries with an enabled/disabled flag where
> "disabled" entries must be ignored; and there's a completely separate
> structure (the "Memory Affinity Structure") which contains information
> about areas that are used for hot-plug RAM, which has it's one
> enabled/disabled flag *and* a separate hotpluggable/not hotpluggable
> flag. From this, it seems logical that if ACPI ever does support
> hot-plug CPUs, then they'll use a separate structure or a separate
> flag to indicate if a CPU is hot pluggable or not, and the existing
> "enabled/disabled" flag will retain it's current (use/don't use)
> meaning.
> 
> For some reason (unknown to me) some Linux developers made wild
> assumptions about disabled "Processor Local APIC" entries, and now
> they're inventing fictitious hardware to support their unfounded
> assumptions.
> 
> Please, correct me if I'm wrong...
> 
Windows 2008 supports CPU hot plug (but not unplug IIRC). How they do it
if there is not specification about how it should work on 80x86?

--
			Gleb.

[Qemu-devel] Re: [Bochs-developers] [PATCH 1/2] create acpi cpu definitions

[Brendan Trotter]

Hi,

On Sun, Apr 19, 2009 at 8:42 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Sun, Apr 19, 2009 at 11:01:09AM +0930, Brendan Trotter wrote:
>> For some reason (unknown to me) some Linux developers made wild
>> assumptions about disabled "Processor Local APIC" entries, and now
>> they're inventing fictitious hardware to support their unfounded
>> assumptions.
>>
>> Please, correct me if I'm wrong...
>>
> Windows 2008 supports CPU hot plug (but not unplug IIRC). How they do it
> if there is not specification about how it should work on 80x86?

Unfortunately, "no specification" typically means hardware
manufacturers are free to make up their own way of doing it - for all
I know Windows 2008 needs a special driver for each system.

I've been unable to find any 80x86 hardware that supports hot-add and
hot-remove of CPUs, and therefore I haven't been able to attempt to
find out how this hardware notifies the OS of capabilities, changes,
etc. The closest thing I did find is redundant CPUs, but I couldn't
find anything with details on how this works, and don't know if the OS
itself is made aware of it at all (the chipset and firmware might
replaced a failing CPU with a spare/redundant CPU without the OS's
knowledge).

I did find a little information for hot add/remove for Itanium
systems, but nothing very detailed. From
"http://www.itaniumsolutions.org/resources/focus_on_dynamic_hardware_partitioning__nec_microsoft_intel":
   "The NEC Express5800/1000 Series of servers, utilizes the Intel
Itanium Machine Check Architecture (for error handling), and adds
another layer of intelligence through a service processor and related
software called the GlobalMaster (managed via either a GUI or CLI), as
shown in Figure 1. The GlobalMaster also communicates with Microsoft
Windows Server 2008 to convey to the operating system that a hot add
(of processors, memory, or I/O) or hot replace (of processors and
memory) is taking place."

This sounds like a lot more involved than just misappropriating a flag
in an ACPI table...



Cheers,

Brendan

[Qemu-devel] Re: [Bochs-developers] [PATCH 1/2] create acpi cpu definitions

[Sebastian Herbszt]

Gleb Natapov wrote:
> Windows 2008 supports CPU hot plug (but not unplug IIRC). How they do it
> if there is not specification about how it should work on 80x86?

Any clue if cpu hotplug support implemented by this patch was tested with
Linux and/or Windows 2008 guests?

- Sebastian

[Qemu-devel] Re: [Bochs-developers] [PATCH 1/2] create acpi cpu definitions

[Gleb Natapov]

On Sun, Apr 19, 2009 at 11:55:57PM +0200, Sebastian Herbszt wrote:
> Gleb Natapov wrote:
>> Windows 2008 supports CPU hot plug (but not unplug IIRC). How they do it
>> if there is not specification about how it should work on 80x86?
>
> Any clue if cpu hotplug support implemented by this patch was tested with
> Linux and/or Windows 2008 guests?
>
Not this one exactly, but I tested Linux and Windows 2008 cpu hotplug
with KVM. Linux works. Windows 2008: if I try to unplug cpu (which is not
supported by win2008) it notice this and complains that device does not
support unplug. Hot plug does not work, win2008 don't notice new CPU.

--
			Gleb.