On 09/29/2014 05:47 PM, Igor Mammedov wrote:
> On Mon, 29 Sep 2014 17:22:08 +0800
> Gu Zheng <guz.f...@cn.fujitsu.com> wrote:
>
>> Hi Igor,
>> On 09/26/2014 09:37 PM, Igor Mammedov wrote:
>>
>>> On Wed, 17 Sep 2014 09:24:03 +0800
>>> Gu Zheng <guz.f...@cn.fujitsu.com> 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 <guz.f...@cn.fujitsu.com>
>>>> ---
>>>> 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?
> Could you point out what dependecies are there?
E.g.
Allocation smi_irq for PIIX4PMState dependents on the valid *first_cpu*.
smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
/* TODO: Populate SPD eeprom data. */
smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
gsi[9], *smi_irq,
kvm_enabled(), fw_cfg, &piix4_pm);
Thanks,
Gu
>
>>
>> Thanks,
>> Gu
>>
>>>
>>>> }
>>>> memory_region_init_io(&gpe_cpu->io, owner,
>>>> &AcpiCpuHotplug_ops, gpe_cpu, "acpi-cpu-hotplug",
>>>> ACPI_GPE_PROC_LEN);
>>>
>>> .
>>>
>>
>>
>
> .
>
