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

[Igor Mammedov]

On Mon, 29 Sep 2014 17:22:08 +0800
Gu Zheng <address@hidden> wrote:

> 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?
Could you point out what dependecies are there?

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