Re: [Qemu-devel] [RESEND PATCH v3 1/8] acpi, piix4: Add memory hot unplug support for piix4.

[Tang Chen]

Hi Igor,

On 09/04/2014 08:15 PM, Igor Mammedov wrote:
> On Wed, 27 Aug 2014 16:08:32 +0800
> Tang Chen <address@hidden> wrote:
>
> > From: Hu Tao <address@hidden>
> >
> > Implement acpi_memory_unplug_cb(), sending an sci to guest to trigger
> > memory hot-remove, and call it in piix4_device_unplug_cb().
> >
> > Signed-off-by: Hu Tao <address@hidden>
> > Signed-off-by: Tang Chen <address@hidden>
> > ---
> >   hw/acpi/memory_hotplug.c         | 25 +++++++++++++++++++++++++
> >   hw/acpi/piix4.c                  |  3 +++
> >   include/hw/acpi/memory_hotplug.h |  2 ++
> >   3 files changed, 30 insertions(+)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index ed39241..c310b44 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -195,6 +195,31 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, 
> > MemHotplugState *mem_st,
> >       return;
> >   }
> >   
> > +void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> > +                           DeviceState *dev, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
> might be worth to use PC_DIMM_SLOT_PROP here, and while at it
> do the same to acpi_memory_plug_cb() as well in separate patch.

Followed.

> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    if (slot >= mem_st->dev_count) {
> > +        char *dev_path = object_get_canonical_path(OBJECT(dev));
> > +        error_setg(errp, "acpi_memory_plug_cb: "
> > +                   "device [%s] returned invalid memory slot[%d]",
> > +                   dev_path, slot);
> > +        g_free(dev_path);
> > +        return;
> > +    }
> move 3 above blocks into separate function to reduce code duplication
> with acpi_memory_plug_cb(), perhaps something like following:
>
> MemStatus *
> acpi_memory_get_slot_status_descriptor(MemHotplugState *mem_st,
>                                         DeviceState *dev, Error **errp);

Followed.

> > +
> place here also missing removal signaling for selected mdev,
> perhaps it's due to wrong patch ordering

It is not a wrong ordering of patches. I meant to define mdev->is_removing
in patch 6/8. But it is OK to define it here. So followed.

> > +    /* do ACPI magic */
> > +    ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> > +    acpi_update_sci(ar, irq);
> worth to make this block a separate function and reuse in
> acpi_memory_plug_cb()

Followed.

Thanks.

> > +}
> > +
> >   static const VMStateDescription vmstate_memhp_sts = {
> >       .name = "memory hotplug device state",
> >       .version_id = 1,
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index b72b34e..c1d5187 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -362,6 +362,9 @@ static void piix4_device_unplug_cb(HotplugHandler 
> > *hotplug_dev,
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> >           acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
> >                                       errp);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > +        acpi_memory_unplug_cb(&s->ar, s->irq, &s->acpi_memory_hotplug, dev,
> > +                              errp);
> >       } else {
> >           error_setg(errp, "acpi: device unplug request for not supported 
> > device"
> >                      " type: %s", object_get_typename(OBJECT(dev)));
> > diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> > index 7bbf8a0..fc6b868 100644
> > --- a/include/hw/acpi/memory_hotplug.h
> > +++ b/include/hw/acpi/memory_hotplug.h
> > @@ -28,6 +28,8 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> >   
> >   void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> >                            DeviceState *dev, Error **errp);
> > +void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> > +                           DeviceState *dev, Error **errp);
> >   
> >   extern const VMStateDescription vmstate_memory_hotplug;
> >   #define VMSTATE_MEMORY_HOTPLUG(memhp, state) \
> .