Re: [Qemu-devel] [PATCH 10/21] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 23 Apr 2013 15:43:51 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> wrote:
> > * introduce processor status bitmask visible to guest at 0xaf00 addr,
> >   where ACPI asl code expects it
> > * set bit corresponding to APIC ID in processor status bitmask on
> >   receiving CPU hot-plug notification
> > * trigger CPU hot-plug SCI, to notify guest about CPU hot-plug event
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> This is wrong (or at least supperfluous)
> 
> > +static int piix4_init_cpu_status(Object *obj, void *opaque)
> > +{
> > +    struct cpu_status *g = (struct cpu_status *)opaque;
> > +    Object *cpu_obj = object_dynamic_cast(obj, TYPE_CPU);
> > +
> > +    if (cpu_obj) {
> > +        struct Error *error = NULL;
> 
> we set error to NULL
> 
> > +        CPUClass *k = CPU_GET_CLASS(cpu_obj);
> > +        int64_t id = k->get_arch_id(CPU(cpu_obj));
> > +
> > +        if (error) {
> 
> and without touching error we test if it is != NULL.
> 
> 
> something is missing here?

It's leftover from previous versions, all error handing should in this
function should be dropped, leaving only assert().

> 
> > +            fprintf(stderr, "failed to initilize CPU status for ACPI:
> > %s\n",
> > +                    error_get_pretty(error));
> > +            error_free(error);
> > +            abort();
> 
> Can't we return an error code at this point?  I guess no,  but asking
> will not hurt.
> 
> > +        }
> > +        g_assert((id / 8) < PIIX4_PROC_LEN);
> > +        g->sts[id / 8] |= (1 << (id % 8));
> > +    }
> > +    return object_child_foreach(obj, piix4_init_cpu_status, opaque);
> > +}
> > +
> >  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> >                                  PCIHotplugState state);
> >  
> > @@ -600,6 +704,13 @@ static void
> > piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, &s->io_pci);
> >      pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
> > +
> > +    piix4_init_cpu_status(qdev_get_machine(), &s->gpe_cpu);
> > +    memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s,
> > "apci-cpu-hotplug",
> > +                          PIIX4_PROC_LEN);
> > +    memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
> > +    s->cpu_added_notifier.notify = piix4_cpu_added_req;
> > +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> >  }
> >  
> >  static void enable_device(PIIX4PMState *s, int slot)
>