diff mbox

[11/33] pc: acpi: cpuhp-legacy: switch ProcessorID to possible_cpus idx

Message ID 1463496205-251412-12-git-send-email-imammedo@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Igor Mammedov May 17, 2016, 2:43 p.m. UTC
In legacy cpu-hotplug ProcessorID == APIC ID is used
in MADT and cpu-hotplug AML. It was fine as both
are 8bit and unique. Spec depricated Processor()
with corresponding ProcessorID and advises to use
Device() and UID instead of it.

However UID is just 32bit and it can't fit ARM's
arch_id(MPIDR) which is 64bit. Also in case of
sparse arch_id() distribution, managment/lookup
of maps by arch_id(APIC ID/MPIDR) becomes complex
and expensive.

In preparation to common CPU hotplug with ARM
and to simplify lookup in possible_cpus[] map
switch ProcessorID to possible_cpus index in
MADT.

Legacy cpu-hotplug considerations:
HW interface of it is APIC ID based bitmask so
it's impossible to change, also CPON package in
AML also APIC ID based as well all the methods.

To avoid massive rewrite of AML keep is so and
just break assumption that ProcessorID == APIC ID,
ammending CPU_MAT_METHOD to accept APIC ID and
possible_cpus index, it needs them both to patch
MADT entry template. Also switch to possible_cpus
index Processor(ProcessorID) AML.
That way changes to MADT/AML are minimal and kept
inside AML/MADT not affecting external interfaces.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/cpu_hotplug.c | 23 +++++++++++++----------
 hw/i386/acpi-build.c  |  2 +-
 2 files changed, 14 insertions(+), 11 deletions(-)

Comments

Marcel Apfelbaum May 30, 2016, 6:39 p.m. UTC | #1
On 05/17/2016 05:43 PM, Igor Mammedov wrote:
> In legacy cpu-hotplug ProcessorID == APIC ID is used
> in MADT and cpu-hotplug AML. It was fine as both
> are 8bit and unique. Spec depricated Processor()
> with corresponding ProcessorID and advises to use
> Device() and UID instead of it.
>
> However UID is just 32bit and it can't fit ARM's
> arch_id(MPIDR) which is 64bit. Also in case of
> sparse arch_id() distribution, managment/lookup
> of maps by arch_id(APIC ID/MPIDR) becomes complex
> and expensive.
>
> In preparation to common CPU hotplug with ARM
> and to simplify lookup in possible_cpus[] map
> switch ProcessorID to possible_cpus index in
> MADT.
>
> Legacy cpu-hotplug considerations:
> HW interface of it is APIC ID based bitmask so
> it's impossible to change, also CPON package in
> AML also APIC ID based as well all the methods.
>
> To avoid massive rewrite of AML keep is so and
> just break assumption that ProcessorID == APIC ID,
> ammending CPU_MAT_METHOD to accept APIC ID and
> possible_cpus index, it needs them both to patch
> MADT entry template. Also switch to possible_cpus
> index Processor(ProcessorID) AML.
> That way changes to MADT/AML are minimal and kept
> inside AML/MADT not affecting external interfaces.
>

I vaguely understood the explanation, I'll let Eduardo have a look :)

Thanks,
Marcel

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/acpi/cpu_hotplug.c | 23 +++++++++++++----------
>   hw/i386/acpi-build.c  |  2 +-
>   2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index 36ea6c2..9d71d2f 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -99,7 +99,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>       int i, apic_idx;
>       Aml *sb_scope = aml_scope("_SB");
>       uint8_t madt_tmpl[8] = {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0};
> -    Aml *cpu_id = aml_arg(0);
> +    Aml *cpu_id = aml_arg(1);
> +    Aml *apic_id = aml_arg(0);
>       Aml *cpu_on = aml_local(0);
>       Aml *madt = aml_local(1);
>       Aml *cpus_map = aml_name(CPU_ON_BITMAP);
> @@ -111,30 +112,31 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>
>       /*
>        * _MAT method - creates an madt apic buffer
> -     * cpu_id = Arg0 = Processor ID = Local APIC ID
> +     * apic_id = Arg0 = Local APIC ID
> +     * cpu_id  = Arg1 = Processor ID
>        * cpu_on = Local0 = CPON flag for this cpu
>        * madt = Local1 = Buffer (in madt apic form) to return
>        */
> -    method = aml_method(CPU_MAT_METHOD, 1, AML_NOTSERIALIZED);
> +    method = aml_method(CPU_MAT_METHOD, 2, AML_NOTSERIALIZED);
>       aml_append(method,
> -        aml_store(aml_derefof(aml_index(cpus_map, cpu_id)), cpu_on));
> +        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
>       aml_append(method,
>           aml_store(aml_buffer(sizeof(madt_tmpl), madt_tmpl), madt));
>       /* Update the processor id, lapic id, and enable/disable status */
>       aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(2))));
> -    aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(3))));
> +    aml_append(method, aml_store(apic_id, aml_index(madt, aml_int(3))));
>       aml_append(method, aml_store(cpu_on, aml_index(madt, aml_int(4))));
>       aml_append(method, aml_return(madt));
>       aml_append(sb_scope, method);
>
>       /*
>        * _STA method - return ON status of cpu
> -     * cpu_id = Arg0 = Processor ID = Local APIC ID
> +     * apic_id = Arg0 = Local APIC ID
>        * cpu_on = Local0 = CPON flag for this cpu
>        */
>       method = aml_method(CPU_STATUS_METHOD, 1, AML_NOTSERIALIZED);
>       aml_append(method,
> -        aml_store(aml_derefof(aml_index(cpus_map, cpu_id)), cpu_on));
> +        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
>       if_ctx = aml_if(cpu_on);
>       {
>           aml_append(if_ctx, aml_return(aml_int(0xF)));
> @@ -243,11 +245,12 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>
>           assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
>
> -        dev = aml_processor(apic_id, 0, 0, "CP%.02X", apic_id);
> +        dev = aml_processor(i, 0, 0, "CP%.02X", apic_id);
>
>           method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
>           aml_append(method,
> -            aml_return(aml_call1(CPU_MAT_METHOD, aml_int(apic_id))));
> +            aml_return(aml_call2(CPU_MAT_METHOD, aml_int(apic_id), aml_int(i))
> +        ));
>           aml_append(dev, method);
>
>           method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> @@ -268,7 +271,7 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>       /* build this code:
>        *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
>        */
> -    /* Arg0 = Processor ID = APIC ID */
> +    /* Arg0 = APIC ID */
>       method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
>       for (i = 0; i < apic_ids->len; i++) {
>           int apic_id = apic_ids->cpus[i].arch_id;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b33fec9..768918f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -354,7 +354,7 @@ build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms)
>
>           apic->type = ACPI_APIC_PROCESSOR;
>           apic->length = sizeof(*apic);
> -        apic->processor_id = apic_id;
> +        apic->processor_id = i;
>           apic->local_apic_id = apic_id;
>           if (apic_ids->cpus[i].cpu != NULL) {
>               apic->flags = cpu_to_le32(1);
>
Igor Mammedov May 31, 2016, 1:03 p.m. UTC | #2
On Mon, 30 May 2016 21:39:50 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 05/17/2016 05:43 PM, Igor Mammedov wrote:
> > In legacy cpu-hotplug ProcessorID == APIC ID is used
> > in MADT and cpu-hotplug AML. It was fine as both
> > are 8bit and unique. Spec depricated Processor()
> > with corresponding ProcessorID and advises to use
> > Device() and UID instead of it.
> >
> > However UID is just 32bit and it can't fit ARM's
> > arch_id(MPIDR) which is 64bit. Also in case of
> > sparse arch_id() distribution, managment/lookup
> > of maps by arch_id(APIC ID/MPIDR) becomes complex
> > and expensive.
> >
> > In preparation to common CPU hotplug with ARM
> > and to simplify lookup in possible_cpus[] map
> > switch ProcessorID to possible_cpus index in
> > MADT.
> >
> > Legacy cpu-hotplug considerations:
> > HW interface of it is APIC ID based bitmask so
> > it's impossible to change, also CPON package in
> > AML also APIC ID based as well all the methods.
> >
> > To avoid massive rewrite of AML keep is so and
> > just break assumption that ProcessorID == APIC ID,
> > ammending CPU_MAT_METHOD to accept APIC ID and
> > possible_cpus index, it needs them both to patch
> > MADT entry template. Also switch to possible_cpus
> > index Processor(ProcessorID) AML.
> > That way changes to MADT/AML are minimal and kept
> > inside AML/MADT not affecting external interfaces.
> >  
> 
> I vaguely understood the explanation, I'll let Eduardo have a look :)
Long story short, patch helps to avoid having
different versions of MADT generation code on x86,
and in future use common function to build MADT
for ARM (generalization should come with ARM's
cpu-hotplug support).

> 
> Thanks,
> Marcel
> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   hw/acpi/cpu_hotplug.c | 23 +++++++++++++----------
> >   hw/i386/acpi-build.c  |  2 +-
> >   2 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > index 36ea6c2..9d71d2f 100644
> > --- a/hw/acpi/cpu_hotplug.c
> > +++ b/hw/acpi/cpu_hotplug.c
> > @@ -99,7 +99,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> >       int i, apic_idx;
> >       Aml *sb_scope = aml_scope("_SB");
> >       uint8_t madt_tmpl[8] = {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0};
> > -    Aml *cpu_id = aml_arg(0);
> > +    Aml *cpu_id = aml_arg(1);
> > +    Aml *apic_id = aml_arg(0);
> >       Aml *cpu_on = aml_local(0);
> >       Aml *madt = aml_local(1);
> >       Aml *cpus_map = aml_name(CPU_ON_BITMAP);
> > @@ -111,30 +112,31 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> >
> >       /*
> >        * _MAT method - creates an madt apic buffer
> > -     * cpu_id = Arg0 = Processor ID = Local APIC ID
> > +     * apic_id = Arg0 = Local APIC ID
> > +     * cpu_id  = Arg1 = Processor ID
> >        * cpu_on = Local0 = CPON flag for this cpu
> >        * madt = Local1 = Buffer (in madt apic form) to return
> >        */
> > -    method = aml_method(CPU_MAT_METHOD, 1, AML_NOTSERIALIZED);
> > +    method = aml_method(CPU_MAT_METHOD, 2, AML_NOTSERIALIZED);
> >       aml_append(method,
> > -        aml_store(aml_derefof(aml_index(cpus_map, cpu_id)), cpu_on));
> > +        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
> >       aml_append(method,
> >           aml_store(aml_buffer(sizeof(madt_tmpl), madt_tmpl), madt));
> >       /* Update the processor id, lapic id, and enable/disable status */
> >       aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(2))));
> > -    aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(3))));
> > +    aml_append(method, aml_store(apic_id, aml_index(madt, aml_int(3))));
> >       aml_append(method, aml_store(cpu_on, aml_index(madt, aml_int(4))));
> >       aml_append(method, aml_return(madt));
> >       aml_append(sb_scope, method);
> >
> >       /*
> >        * _STA method - return ON status of cpu
> > -     * cpu_id = Arg0 = Processor ID = Local APIC ID
> > +     * apic_id = Arg0 = Local APIC ID
> >        * cpu_on = Local0 = CPON flag for this cpu
> >        */
> >       method = aml_method(CPU_STATUS_METHOD, 1, AML_NOTSERIALIZED);
> >       aml_append(method,
> > -        aml_store(aml_derefof(aml_index(cpus_map, cpu_id)), cpu_on));
> > +        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
> >       if_ctx = aml_if(cpu_on);
> >       {
> >           aml_append(if_ctx, aml_return(aml_int(0xF)));
> > @@ -243,11 +245,12 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> >
> >           assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
> >
> > -        dev = aml_processor(apic_id, 0, 0, "CP%.02X", apic_id);
> > +        dev = aml_processor(i, 0, 0, "CP%.02X", apic_id);
> >
> >           method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
> >           aml_append(method,
> > -            aml_return(aml_call1(CPU_MAT_METHOD, aml_int(apic_id))));
> > +            aml_return(aml_call2(CPU_MAT_METHOD, aml_int(apic_id), aml_int(i))
> > +        ));
> >           aml_append(dev, method);
> >
> >           method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> > @@ -268,7 +271,7 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> >       /* build this code:
> >        *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
> >        */
> > -    /* Arg0 = Processor ID = APIC ID */
> > +    /* Arg0 = APIC ID */
> >       method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
> >       for (i = 0; i < apic_ids->len; i++) {
> >           int apic_id = apic_ids->cpus[i].arch_id;
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b33fec9..768918f 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -354,7 +354,7 @@ build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms)
> >
> >           apic->type = ACPI_APIC_PROCESSOR;
> >           apic->length = sizeof(*apic);
> > -        apic->processor_id = apic_id;
> > +        apic->processor_id = i;
> >           apic->local_apic_id = apic_id;
> >           if (apic_ids->cpus[i].cpu != NULL) {
> >               apic->flags = cpu_to_le32(1);
> >  
>
diff mbox

Patch

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 36ea6c2..9d71d2f 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -99,7 +99,8 @@  void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
     int i, apic_idx;
     Aml *sb_scope = aml_scope("_SB");
     uint8_t madt_tmpl[8] = {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0};
-    Aml *cpu_id = aml_arg(0);
+    Aml *cpu_id = aml_arg(1);
+    Aml *apic_id = aml_arg(0);
     Aml *cpu_on = aml_local(0);
     Aml *madt = aml_local(1);
     Aml *cpus_map = aml_name(CPU_ON_BITMAP);
@@ -111,30 +112,31 @@  void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
 
     /*
      * _MAT method - creates an madt apic buffer
-     * cpu_id = Arg0 = Processor ID = Local APIC ID
+     * apic_id = Arg0 = Local APIC ID
+     * cpu_id  = Arg1 = Processor ID
      * cpu_on = Local0 = CPON flag for this cpu
      * madt = Local1 = Buffer (in madt apic form) to return
      */
-    method = aml_method(CPU_MAT_METHOD, 1, AML_NOTSERIALIZED);
+    method = aml_method(CPU_MAT_METHOD, 2, AML_NOTSERIALIZED);
     aml_append(method,
-        aml_store(aml_derefof(aml_index(cpus_map, cpu_id)), cpu_on));
+        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
     aml_append(method,
         aml_store(aml_buffer(sizeof(madt_tmpl), madt_tmpl), madt));
     /* Update the processor id, lapic id, and enable/disable status */
     aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(2))));
-    aml_append(method, aml_store(cpu_id, aml_index(madt, aml_int(3))));
+    aml_append(method, aml_store(apic_id, aml_index(madt, aml_int(3))));
     aml_append(method, aml_store(cpu_on, aml_index(madt, aml_int(4))));
     aml_append(method, aml_return(madt));
     aml_append(sb_scope, method);
 
     /*
      * _STA method - return ON status of cpu
-     * cpu_id = Arg0 = Processor ID = Local APIC ID
+     * apic_id = Arg0 = Local APIC ID
      * cpu_on = Local0 = CPON flag for this cpu
      */
     method = aml_method(CPU_STATUS_METHOD, 1, AML_NOTSERIALIZED);
     aml_append(method,
-        aml_store(aml_derefof(aml_index(cpus_map, cpu_id)), cpu_on));
+        aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on));
     if_ctx = aml_if(cpu_on);
     {
         aml_append(if_ctx, aml_return(aml_int(0xF)));
@@ -243,11 +245,12 @@  void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
 
         assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
 
-        dev = aml_processor(apic_id, 0, 0, "CP%.02X", apic_id);
+        dev = aml_processor(i, 0, 0, "CP%.02X", apic_id);
 
         method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
         aml_append(method,
-            aml_return(aml_call1(CPU_MAT_METHOD, aml_int(apic_id))));
+            aml_return(aml_call2(CPU_MAT_METHOD, aml_int(apic_id), aml_int(i))
+        ));
         aml_append(dev, method);
 
         method = aml_method("_STA", 0, AML_NOTSERIALIZED);
@@ -268,7 +271,7 @@  void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
     /* build this code:
      *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
      */
-    /* Arg0 = Processor ID = APIC ID */
+    /* Arg0 = APIC ID */
     method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
     for (i = 0; i < apic_ids->len; i++) {
         int apic_id = apic_ids->cpus[i].arch_id;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b33fec9..768918f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -354,7 +354,7 @@  build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms)
 
         apic->type = ACPI_APIC_PROCESSOR;
         apic->length = sizeof(*apic);
-        apic->processor_id = apic_id;
+        apic->processor_id = i;
         apic->local_apic_id = apic_id;
         if (apic_ids->cpus[i].cpu != NULL) {
             apic->flags = cpu_to_le32(1);