Re: [Qemu-devel] [PATCH V3 7/7] acpi/cpu-hotplug: introduce help function to keep bit setting in one place

[Gu Zheng]

Hi Igor,
On 09/26/2014 09:37 PM, Igor Mammedov wrote:

> On Wed, 17 Sep 2014 09:24:03 +0800
> Gu Zheng <address@hidden> wrote:
> 
>> Introduce help function acpi_set_local_sts() to simplify acpi_cpu_plug_cb
>> and acpi_cpu_hotplug_init, so that we can keep bit setting in one place.
>>
>> Signed-off-by: Gu Zheng <address@hidden>
>> ---
>>  hw/acpi/cpu_hotplug.c |   22 +++++++++++++++-------
>>  1 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
>> index 7629686..d42c961 100644
>> --- a/hw/acpi/cpu_hotplug.c
>> +++ b/hw/acpi/cpu_hotplug.c
>> @@ -36,8 +36,8 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
>>      },
>>  };
>>  
>> -void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
>> -                      AcpiCpuHotplug *g, CPUState *cpu, Error **errp)
>> +static void acpi_set_local_sts(AcpiCpuHotplug *g, CPUState *cpu,
>> +                               Error **errp)
>>  {
>>      CPUClass *k = CPU_GET_CLASS(cpu);
>>      int64_t cpu_id;
>> @@ -48,9 +48,18 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
>>          return;
>>      }
>>  
>> -    ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS;
>>      g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
>> +}
>>  
>> +void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq,
>> +                      AcpiCpuHotplug *g, CPUState *cpu, Error **errp)
>> +{
>> +    acpi_set_local_sts(g, cpu, errp);
>> +    if (*errp != NULL) {
>> +        return;
>> +    }
>> +
> 
>> +    ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS;
> ^^^ shouldn't be set for coldplugged CPUs along with IRQ below
> 
>>      acpi_update_sci(ar, irq);
>>  }
>>  
>> @@ -60,11 +69,10 @@ void acpi_cpu_hotplug_init(MemoryRegion *parent, Object 
>> *owner,
>>      CPUState *cpu;
>>  
>>      CPU_FOREACH(cpu) {
>> -        CPUClass *cc = CPU_GET_CLASS(cpu);
>> -        int64_t id = cc->get_arch_id(cpu);
>> +        Error *local_err = NULL;
>>  
>> -        g_assert((id / 8) < ACPI_GPE_PROC_LEN);
>> -        gpe_cpu->sts[id / 8] |= (1 << (id % 8));
>> +        acpi_set_local_sts(gpe_cpu, cpu, &local_err);
>> +        g_assert(local_err == NULL);
> Wouldn't acpi_cpu_plug_cb() do everything this FOREACH does?
> I've suggest to drop CPU_FOREACH() altogether.

I got your meaning: Make acpi_cpu_plug_cb serve all CPUs (both startup and 
hotpluged)
, and only triggers sci irq for hotpluged ones, so that we can drop the 
duplicated
operations in acpi_cpu_hotplug_init.
But one problem is that pcms->acpi_dev was registered after startup CPUs set 
realized, so
acpi_cpu_plug_cb can not serve startup CPUs.
I tried to switch the order, but it failed, because there are some internal 
dependences.
Now, I'm trying to split the startup CPUs init into two steps (init and 
realize), and
register pcms->acpi_dev between the two steps.
What's your opinion?

Thanks,
Gu

> 
>>      }
>>      memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
>>                            gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> 
> .
>