[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 <address@hidden>
> ---
> 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