On Mon, 29 Sep 2014 17:44:27 +0800
Gu Zheng <guz.f...@cn.fujitsu.com> wrote:
> 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);
Looks like it would be too much trouble for not much gain.
Lets keep this patch as is.
> Thanks,
> Gu
>
> >
> >>
> >> Thanks,
> >> Gu
> >>
> >>>
> >>>> }
> >>>> memory_region_init_io(&gpe_cpu->io, owner,
> >>>> &AcpiCpuHotplug_ops, gpe_cpu, "acpi-cpu-hotplug",
> >>>> ACPI_GPE_PROC_LEN);
> >>>
> >>> .
> >>>
> >>
> >>
> >
> > .
> >
>
>
>
