diff mbox series

[v4] arm/virt: Add memory hot remove support

Message ID 20200622124157.20360-1-shameerali.kolothum.thodi@huawei.com
State Superseded
Headers show
Series [v4] arm/virt: Add memory hot remove support | expand

Commit Message

Shameerali Kolothum Thodi June 22, 2020, 12:41 p.m. UTC
This adds support for memory(pc-dimm) hot remove on arm/virt that
uses acpi ged device.

NVDIMM hot removal is not yet supported.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

---
v2 --> v3
  -Addressed Eric's comments on v3. 
v2 --> v3
  -Addressed Eric's review comment and added check for NVDIMM.
RFC v1 --> v2
  -Rebased on top of latest Qemu master.
  -Dropped "RFC" and tested with kernel 5.7-rc6
---
 hw/acpi/generic_event_device.c | 29 ++++++++++++++++
 hw/arm/virt.c                  | 62 ++++++++++++++++++++++++++++++++--
 2 files changed, 89 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Eric Auger June 23, 2020, 10:14 a.m. UTC | #1
Hi Shameer,

On 6/22/20 2:41 PM, Shameer Kolothum wrote:
> This adds support for memory(pc-dimm) hot remove on arm/virt that

> uses acpi ged device.

> 

> NVDIMM hot removal is not yet supported.

> 

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Works fine for me when passing "movable_node" in the guest kernel
params. I am able to hotplug and hotremove PCDIMM slots several times

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Tested-by: Eric Auger <eric.auger@redhat.com>


Thanks

Eric
> ---

> v2 --> v3

>   -Addressed Eric's comments on v3. 

> v2 --> v3

>   -Addressed Eric's review comment and added check for NVDIMM.

> RFC v1 --> v2

>   -Rebased on top of latest Qemu master.

>   -Dropped "RFC" and tested with kernel 5.7-rc6

> ---

>  hw/acpi/generic_event_device.c | 29 ++++++++++++++++

>  hw/arm/virt.c                  | 62 ++++++++++++++++++++++++++++++++--

>  2 files changed, 89 insertions(+), 2 deletions(-)

> 

> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c

> index 1cb34111e5..b8abdefa1c 100644

> --- a/hw/acpi/generic_event_device.c

> +++ b/hw/acpi/generic_event_device.c

> @@ -193,6 +193,33 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,

>      }

>  }

>  

> +static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,

> +                                       DeviceState *dev, Error **errp)

> +{

> +    AcpiGedState *s = ACPI_GED(hotplug_dev);

> +

> +    if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&

> +                       !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {

> +        acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);

> +    } else {

> +        error_setg(errp, "acpi: device unplug request for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

> +}

> +

> +static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,

> +                               DeviceState *dev, Error **errp)

> +{

> +    AcpiGedState *s = ACPI_GED(hotplug_dev);

> +

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> +        acpi_memory_unplug_cb(&s->memhp_state, dev, errp);

> +    } else {

> +        error_setg(errp, "acpi: device unplug for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

> +}

> +

>  static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)

>  {

>      AcpiGedState *s = ACPI_GED(adev);

> @@ -318,6 +345,8 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)

>      dc->vmsd = &vmstate_acpi_ged;

>  

>      hc->plug = acpi_ged_device_plug_cb;

> +    hc->unplug_request = acpi_ged_unplug_request_cb;

> +    hc->unplug = acpi_ged_unplug_cb;

>  

>      adevc->send_event = acpi_ged_send_event;

>  }

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c

> index caceb1e4a0..80ef2d7b7a 100644

> --- a/hw/arm/virt.c

> +++ b/hw/arm/virt.c

> @@ -2177,11 +2177,68 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,

>      }

>  }

>  

> +static void virt_dimm_unplug_request(HotplugHandler *hotplug_dev,

> +                                     DeviceState *dev, Error **errp)

> +{

> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);

> +    Error *local_err = NULL;

> +

> +    if (!vms->acpi_dev) {

> +        error_setg(&local_err,

> +                   "memory hotplug is not enabled: missing acpi-ged device");

> +        goto out;

> +    }

> +

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {

> +        error_setg(&local_err,

> +                   "nvdimm device hot unplug is not supported yet.");

> +        goto out;

> +    }

> +

> +    hotplug_handler_unplug_request(HOTPLUG_HANDLER(vms->acpi_dev), dev,

> +                                   &local_err);

> +out:

> +    error_propagate(errp, local_err);

> +}

> +

> +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,

> +                             DeviceState *dev, Error **errp)

> +{

> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);

> +    Error *local_err = NULL;

> +

> +    hotplug_handler_unplug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &local_err);

> +    if (local_err) {

> +        goto out;

> +    }

> +

> +    pc_dimm_unplug(PC_DIMM(dev), MACHINE(vms));

> +    qdev_unrealize(dev);

> +

> +out:

> +    error_propagate(errp, local_err);

> +}

> +

>  static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,

>                                            DeviceState *dev, Error **errp)

>  {

> -    error_setg(errp, "device unplug request for unsupported device"

> -               " type: %s", object_get_typename(OBJECT(dev)));

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> +        virt_dimm_unplug_request(hotplug_dev, dev, errp);

> +    } else {

> +        error_setg(errp, "device unplug request for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

> +}

> +

> +static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,

> +                                          DeviceState *dev, Error **errp)

> +{

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> +        virt_dimm_unplug(hotplug_dev, dev, errp);

> +    } else {

> +        error_setg(errp, "virt: device unplug for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

>  }

>  

>  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,

> @@ -2262,6 +2319,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)

>      hc->pre_plug = virt_machine_device_pre_plug_cb;

>      hc->plug = virt_machine_device_plug_cb;

>      hc->unplug_request = virt_machine_device_unplug_request_cb;

> +    hc->unplug = virt_machine_device_unplug_cb;

>      mc->numa_mem_supported = true;

>      mc->nvdimm_supported = true;

>      mc->auto_enable_numa_with_memhp = true;

>
Peter Maydell June 23, 2020, 10:37 a.m. UTC | #2
On Tue, 23 Jun 2020 at 11:15, Auger Eric <eric.auger@redhat.com> wrote:
>

> Hi Shameer,

>

> On 6/22/20 2:41 PM, Shameer Kolothum wrote:

> > This adds support for memory(pc-dimm) hot remove on arm/virt that

> > uses acpi ged device.

> >

> > NVDIMM hot removal is not yet supported.

> >

> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> Works fine for me when passing "movable_node" in the guest kernel

> params. I am able to hotplug and hotremove PCDIMM slots several times

>

> Reviewed-by: Eric Auger <eric.auger@redhat.com>

> Tested-by: Eric Auger <eric.auger@redhat.com>


Thanks for the review, Eric.

Applied to target-arm.next, thanks.

-- PMM
Igor Mammedov June 24, 2020, 2:08 p.m. UTC | #3
On Mon, 22 Jun 2020 13:41:57 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This adds support for memory(pc-dimm) hot remove on arm/virt that

> uses acpi ged device.

> 

> NVDIMM hot removal is not yet supported.

> 

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> ---

> v2 --> v3

>   -Addressed Eric's comments on v3. 

> v2 --> v3

>   -Addressed Eric's review comment and added check for NVDIMM.

> RFC v1 --> v2

>   -Rebased on top of latest Qemu master.

>   -Dropped "RFC" and tested with kernel 5.7-rc6

> ---

>  hw/acpi/generic_event_device.c | 29 ++++++++++++++++

>  hw/arm/virt.c                  | 62 ++++++++++++++++++++++++++++++++--

>  2 files changed, 89 insertions(+), 2 deletions(-)

> 

> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c

> index 1cb34111e5..b8abdefa1c 100644

> --- a/hw/acpi/generic_event_device.c

> +++ b/hw/acpi/generic_event_device.c

> @@ -193,6 +193,33 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,

>      }

>  }

>  

> +static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,

> +                                       DeviceState *dev, Error **errp)

> +{

> +    AcpiGedState *s = ACPI_GED(hotplug_dev);

> +

> +    if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&

> +                       !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {

> +        acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);

> +    } else {

> +        error_setg(errp, "acpi: device unplug request for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

> +}

> +

> +static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,

> +                               DeviceState *dev, Error **errp)

> +{

> +    AcpiGedState *s = ACPI_GED(hotplug_dev);

> +

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> +        acpi_memory_unplug_cb(&s->memhp_state, dev, errp);

> +    } else {

> +        error_setg(errp, "acpi: device unplug for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

> +}

> +

>  static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)

>  {

>      AcpiGedState *s = ACPI_GED(adev);

> @@ -318,6 +345,8 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)

>      dc->vmsd = &vmstate_acpi_ged;

>  

>      hc->plug = acpi_ged_device_plug_cb;

> +    hc->unplug_request = acpi_ged_unplug_request_cb;

> +    hc->unplug = acpi_ged_unplug_cb;

>  

>      adevc->send_event = acpi_ged_send_event;

>  }

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c

> index caceb1e4a0..80ef2d7b7a 100644

> --- a/hw/arm/virt.c

> +++ b/hw/arm/virt.c

> @@ -2177,11 +2177,68 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,

>      }

>  }

>  

> +static void virt_dimm_unplug_request(HotplugHandler *hotplug_dev,

> +                                     DeviceState *dev, Error **errp)

> +{

> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);

> +    Error *local_err = NULL;

> +

> +    if (!vms->acpi_dev) {

> +        error_setg(&local_err,

> +                   "memory hotplug is not enabled: missing acpi-ged device");

> +        goto out;

> +    }

> +

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {

> +        error_setg(&local_err,

> +                   "nvdimm device hot unplug is not supported yet.");

> +        goto out;

> +    }

> +

> +    hotplug_handler_unplug_request(HOTPLUG_HANDLER(vms->acpi_dev), dev,

> +                                   &local_err);

> +out:

> +    error_propagate(errp, local_err);

> +}

> +

> +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,

> +                             DeviceState *dev, Error **errp)

> +{

> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);

> +    Error *local_err = NULL;

> +

> +    hotplug_handler_unplug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &local_err);

> +    if (local_err) {

> +        goto out;

> +    }

> +

> +    pc_dimm_unplug(PC_DIMM(dev), MACHINE(vms));

> +    qdev_unrealize(dev);


doesn't pc_dimm_unplug() do unrealize already?
(/me wonders why it doesn't explode here,
are we leaking a refference somewhere so dimm is still alive?)

> +

> +out:

> +    error_propagate(errp, local_err);

> +}

> +

>  static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,

>                                            DeviceState *dev, Error **errp)

>  {

> -    error_setg(errp, "device unplug request for unsupported device"

> -               " type: %s", object_get_typename(OBJECT(dev)));

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> +        virt_dimm_unplug_request(hotplug_dev, dev, errp);

> +    } else {

> +        error_setg(errp, "device unplug request for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

> +}

> +

> +static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,

> +                                          DeviceState *dev, Error **errp)

> +{

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> +        virt_dimm_unplug(hotplug_dev, dev, errp);

> +    } else {

> +        error_setg(errp, "virt: device unplug for unsupported device"

> +                   " type: %s", object_get_typename(OBJECT(dev)));

> +    }

>  }

>  

>  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,

> @@ -2262,6 +2319,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)

>      hc->pre_plug = virt_machine_device_pre_plug_cb;

>      hc->plug = virt_machine_device_plug_cb;

>      hc->unplug_request = virt_machine_device_unplug_request_cb;

> +    hc->unplug = virt_machine_device_unplug_cb;

>      mc->numa_mem_supported = true;

>      mc->nvdimm_supported = true;

>      mc->auto_enable_numa_with_memhp = true;
Shameerali Kolothum Thodi June 24, 2020, 4:50 p.m. UTC | #4
> -----Original Message-----

> From: Igor Mammedov [mailto:imammedo@redhat.com]

> Sent: 24 June 2020 15:09

> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;

> peter.maydell@linaro.org; mst@redhat.com; Linuxarm

> <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>;

> eric.auger@redhat.com; Zengtao (B) <prime.zeng@hisilicon.com>

> Subject: Re: [PATCH v4] arm/virt: Add memory hot remove support

> 

> On Mon, 22 Jun 2020 13:41:57 +0100

> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> 

> > This adds support for memory(pc-dimm) hot remove on arm/virt that

> > uses acpi ged device.

> >

> > NVDIMM hot removal is not yet supported.

> >

> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> > ---

> > v2 --> v3

> >   -Addressed Eric's comments on v3.

> > v2 --> v3

> >   -Addressed Eric's review comment and added check for NVDIMM.

> > RFC v1 --> v2

> >   -Rebased on top of latest Qemu master.

> >   -Dropped "RFC" and tested with kernel 5.7-rc6

> > ---

> >  hw/acpi/generic_event_device.c | 29 ++++++++++++++++

> >  hw/arm/virt.c                  | 62

> ++++++++++++++++++++++++++++++++--

> >  2 files changed, 89 insertions(+), 2 deletions(-)

> >

> > diff --git a/hw/acpi/generic_event_device.c

> b/hw/acpi/generic_event_device.c

> > index 1cb34111e5..b8abdefa1c 100644

> > --- a/hw/acpi/generic_event_device.c

> > +++ b/hw/acpi/generic_event_device.c

> > @@ -193,6 +193,33 @@ static void


[...]

> > +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,

> > +                             DeviceState *dev, Error **errp)

> > +{

> > +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);

> > +    Error *local_err = NULL;

> > +

> > +    hotplug_handler_unplug(HOTPLUG_HANDLER(vms->acpi_dev), dev,

> &local_err);

> > +    if (local_err) {

> > +        goto out;

> > +    }

> > +

> > +    pc_dimm_unplug(PC_DIMM(dev), MACHINE(vms));

> > +    qdev_unrealize(dev);

> 

> doesn't pc_dimm_unplug() do unrealize already?

> (/me wonders why it doesn't explode here,

> are we leaking a refference somewhere so dimm is still alive?)


Does it? From a quick look at the code it is not obvious.

pc_dimm_unplug()
  memory_device_unplug()
    memory_region_del_subregion()
  vmstate_unregister_ram()
    qemu_ram_unset_idstr()
    qemu_ram_unset_migratable()

If it does, then we may need to fix x86/ppc as well.

Thanks,
Shameer

> > +

> > +out:

> > +    error_propagate(errp, local_err);

> > +}

> > +

> >  static void virt_machine_device_unplug_request_cb(HotplugHandler

> *hotplug_dev,

> >                                            DeviceState *dev, Error

> **errp)

> >  {

> > -    error_setg(errp, "device unplug request for unsupported device"

> > -               " type: %s", object_get_typename(OBJECT(dev)));

> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> > +        virt_dimm_unplug_request(hotplug_dev, dev, errp);

> > +    } else {

> > +        error_setg(errp, "device unplug request for unsupported device"

> > +                   " type: %s", object_get_typename(OBJECT(dev)));

> > +    }

> > +}

> > +

> > +static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,

> > +                                          DeviceState *dev, Error

> **errp)

> > +{

> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> > +        virt_dimm_unplug(hotplug_dev, dev, errp);

> > +    } else {

> > +        error_setg(errp, "virt: device unplug for unsupported device"

> > +                   " type: %s", object_get_typename(OBJECT(dev)));

> > +    }

> >  }

> >

> >  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState

> *machine,

> > @@ -2262,6 +2319,7 @@ static void virt_machine_class_init(ObjectClass

> *oc, void *data)

> >      hc->pre_plug = virt_machine_device_pre_plug_cb;

> >      hc->plug = virt_machine_device_plug_cb;

> >      hc->unplug_request = virt_machine_device_unplug_request_cb;

> > +    hc->unplug = virt_machine_device_unplug_cb;

> >      mc->numa_mem_supported = true;

> >      mc->nvdimm_supported = true;

> >      mc->auto_enable_numa_with_memhp = true;
Peter Maydell June 24, 2020, 8:05 p.m. UTC | #5
On Wed, 24 Jun 2020 at 17:50, Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@huawei.com> wrote:
>

>

>

> > -----Original Message-----

> > From: Igor Mammedov [mailto:imammedo@redhat.com]

> > doesn't pc_dimm_unplug() do unrealize already?

> > (/me wonders why it doesn't explode here,

> > are we leaking a refference somewhere so dimm is still alive?)

>

> Does it? From a quick look at the code it is not obvious.

>

> pc_dimm_unplug()

>   memory_device_unplug()

>     memory_region_del_subregion()

>   vmstate_unregister_ram()

>     qemu_ram_unset_idstr()

>     qemu_ram_unset_migratable()

>

> If it does, then we may need to fix x86/ppc as well.


In any case this patch is now in master, so if you determine
that it needs a fix please send that as a followup patch.

thanks
-- PMM
Igor Mammedov June 25, 2020, 7:22 p.m. UTC | #6
On Wed, 24 Jun 2020 16:50:11 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> > -----Original Message-----

> > From: Igor Mammedov [mailto:imammedo@redhat.com]

> > Sent: 24 June 2020 15:09

> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;

> > peter.maydell@linaro.org; mst@redhat.com; Linuxarm

> > <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>;

> > eric.auger@redhat.com; Zengtao (B) <prime.zeng@hisilicon.com>

> > Subject: Re: [PATCH v4] arm/virt: Add memory hot remove support

> > 

> > On Mon, 22 Jun 2020 13:41:57 +0100

> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> >   

> > > This adds support for memory(pc-dimm) hot remove on arm/virt that

> > > uses acpi ged device.

> > >

> > > NVDIMM hot removal is not yet supported.

> > >

> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> > > ---

> > > v2 --> v3

> > >   -Addressed Eric's comments on v3.

> > > v2 --> v3

> > >   -Addressed Eric's review comment and added check for NVDIMM.

> > > RFC v1 --> v2

> > >   -Rebased on top of latest Qemu master.

> > >   -Dropped "RFC" and tested with kernel 5.7-rc6

> > > ---

> > >  hw/acpi/generic_event_device.c | 29 ++++++++++++++++

> > >  hw/arm/virt.c                  | 62  

> > ++++++++++++++++++++++++++++++++--  

> > >  2 files changed, 89 insertions(+), 2 deletions(-)

> > >

> > > diff --git a/hw/acpi/generic_event_device.c  

> > b/hw/acpi/generic_event_device.c  

> > > index 1cb34111e5..b8abdefa1c 100644

> > > --- a/hw/acpi/generic_event_device.c

> > > +++ b/hw/acpi/generic_event_device.c

> > > @@ -193,6 +193,33 @@ static void  

> 

> [...]

> 

> > > +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,

> > > +                             DeviceState *dev, Error **errp)

> > > +{

> > > +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);

> > > +    Error *local_err = NULL;

> > > +

> > > +    hotplug_handler_unplug(HOTPLUG_HANDLER(vms->acpi_dev), dev,  

> > &local_err);  

> > > +    if (local_err) {

> > > +        goto out;

> > > +    }

> > > +

> > > +    pc_dimm_unplug(PC_DIMM(dev), MACHINE(vms));

> > > +    qdev_unrealize(dev);  

> > 

> > doesn't pc_dimm_unplug() do unrealize already?

> > (/me wonders why it doesn't explode here,

> > are we leaking a refference somewhere so dimm is still alive?)  

> 

> Does it? From a quick look at the code it is not obvious.

> 

> pc_dimm_unplug()

>   memory_device_unplug()

>     memory_region_del_subregion()

>   vmstate_unregister_ram()

>     qemu_ram_unset_idstr()

>     qemu_ram_unset_migratable()

> 

> If it does, then we may need to fix x86/ppc as well.

you are right, it's correct.
I mixed it up with:
 pc_memory_unplug()
     pc_dimm_unplug(PC_DIMM(dev), MACHINE(pcms));
     object_property_set_bool(OBJECT(dev), false, "realized", &error_abort);


> 

> Thanks,

> Shameer

> 

> > > +

> > > +out:

> > > +    error_propagate(errp, local_err);

> > > +}

> > > +

> > >  static void virt_machine_device_unplug_request_cb(HotplugHandler  

> > *hotplug_dev,  

> > >                                            DeviceState *dev, Error  

> > **errp)  

> > >  {

> > > -    error_setg(errp, "device unplug request for unsupported device"

> > > -               " type: %s", object_get_typename(OBJECT(dev)));

> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> > > +        virt_dimm_unplug_request(hotplug_dev, dev, errp);

> > > +    } else {

> > > +        error_setg(errp, "device unplug request for unsupported device"

> > > +                   " type: %s", object_get_typename(OBJECT(dev)));

> > > +    }

> > > +}

> > > +

> > > +static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,

> > > +                                          DeviceState *dev, Error  

> > **errp)  

> > > +{

> > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {

> > > +        virt_dimm_unplug(hotplug_dev, dev, errp);

> > > +    } else {

> > > +        error_setg(errp, "virt: device unplug for unsupported device"

> > > +                   " type: %s", object_get_typename(OBJECT(dev)));

> > > +    }

> > >  }

> > >

> > >  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState  

> > *machine,  

> > > @@ -2262,6 +2319,7 @@ static void virt_machine_class_init(ObjectClass  

> > *oc, void *data)  

> > >      hc->pre_plug = virt_machine_device_pre_plug_cb;

> > >      hc->plug = virt_machine_device_plug_cb;

> > >      hc->unplug_request = virt_machine_device_unplug_request_cb;

> > > +    hc->unplug = virt_machine_device_unplug_cb;

> > >      mc->numa_mem_supported = true;

> > >      mc->nvdimm_supported = true;

> > >      mc->auto_enable_numa_with_memhp = true;  

>
diff mbox series

Patch

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 1cb34111e5..b8abdefa1c 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -193,6 +193,33 @@  static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
     }
 }
 
+static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
+{
+    AcpiGedState *s = ACPI_GED(hotplug_dev);
+
+    if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
+                       !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
+        acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);
+    } else {
+        error_setg(errp, "acpi: device unplug request for unsupported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
+}
+
+static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,
+                               DeviceState *dev, Error **errp)
+{
+    AcpiGedState *s = ACPI_GED(hotplug_dev);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
+    } else {
+        error_setg(errp, "acpi: device unplug for unsupported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
+}
+
 static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
 {
     AcpiGedState *s = ACPI_GED(adev);
@@ -318,6 +345,8 @@  static void acpi_ged_class_init(ObjectClass *class, void *data)
     dc->vmsd = &vmstate_acpi_ged;
 
     hc->plug = acpi_ged_device_plug_cb;
+    hc->unplug_request = acpi_ged_unplug_request_cb;
+    hc->unplug = acpi_ged_unplug_cb;
 
     adevc->send_event = acpi_ged_send_event;
 }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index caceb1e4a0..80ef2d7b7a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2177,11 +2177,68 @@  static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
     }
 }
 
+static void virt_dimm_unplug_request(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+    Error *local_err = NULL;
+
+    if (!vms->acpi_dev) {
+        error_setg(&local_err,
+                   "memory hotplug is not enabled: missing acpi-ged device");
+        goto out;
+    }
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+        error_setg(&local_err,
+                   "nvdimm device hot unplug is not supported yet.");
+        goto out;
+    }
+
+    hotplug_handler_unplug_request(HOTPLUG_HANDLER(vms->acpi_dev), dev,
+                                   &local_err);
+out:
+    error_propagate(errp, local_err);
+}
+
+static void virt_dimm_unplug(HotplugHandler *hotplug_dev,
+                             DeviceState *dev, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+    Error *local_err = NULL;
+
+    hotplug_handler_unplug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    pc_dimm_unplug(PC_DIMM(dev), MACHINE(vms));
+    qdev_unrealize(dev);
+
+out:
+    error_propagate(errp, local_err);
+}
+
 static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
-    error_setg(errp, "device unplug request for unsupported device"
-               " type: %s", object_get_typename(OBJECT(dev)));
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        virt_dimm_unplug_request(hotplug_dev, dev, errp);
+    } else {
+        error_setg(errp, "device unplug request for unsupported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
+}
+
+static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        virt_dimm_unplug(hotplug_dev, dev, errp);
+    } else {
+        error_setg(errp, "virt: device unplug for unsupported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
 }
 
 static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
@@ -2262,6 +2319,7 @@  static void virt_machine_class_init(ObjectClass *oc, void *data)
     hc->pre_plug = virt_machine_device_pre_plug_cb;
     hc->plug = virt_machine_device_plug_cb;
     hc->unplug_request = virt_machine_device_unplug_request_cb;
+    hc->unplug = virt_machine_device_unplug_cb;
     mc->numa_mem_supported = true;
     mc->nvdimm_supported = true;
     mc->auto_enable_numa_with_memhp = true;