RE: [PATCH v3] arm/virt: Add memory hot remove support
Hi Eric, > -Original Message- > From: Auger Eric [mailto:eric.au...@redhat.com] > Sent: 18 June 2020 15:42 > To: Shameerali Kolothum Thodi ; > qemu-devel@nongnu.org; qemu-...@nongnu.org > Cc: peter.mayd...@linaro.org; m...@redhat.com; Linuxarm > ; xuwei (O) ; Zengtao (B) > ; imamm...@redhat.com > Subject: Re: [PATCH v3] arm/virt: Add memory hot remove support > > Hi Shameer, > > On 6/18/20 2:21 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 > > --- > > 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, > >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(>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 = _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..a981dc9f1c 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(errp, > local_err? otherwise no need to propagate? That’s right. I will change that. But since we do check for vms->acpi_dev in virt_memory_pre_plug(), do we really need to check this here? I can't think of getting here without first hitting _pre_plug(). Anyway hw/i386/pc.c has got checks in both the places, so I will keep it. > > + "memory hotplug is not enabled: missing acpi-ged > device"); > > +goto out; > > +} > > + > > +if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) { > > +error_setg(_err, > > + "nvdimm device hot unplug is not supported yet."); > > +goto out; > > +} > > + > > +hotplug_handler_unplug_request(HOTPLUG_HANDLER(vms->acpi_dev), > dev, > > +
Re: [PATCH v3] arm/virt: Add memory hot remove support
Hi Shameer, On 6/18/20 2:21 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 > --- > 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, >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(>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 = _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..a981dc9f1c 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(errp, local_err? otherwise no need to propagate? > + "memory hotplug is not enabled: missing acpi-ged device"); > +goto out; > +} > + > +if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) { > +error_setg(_err, > + "nvdimm device hot unplug is not supported yet."); > +goto out; > +} > + > +hotplug_handler_unplug_request(HOTPLUG_HANDLER(vms->acpi_dev), dev, > + _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, _err); > +if (local_err) { > +goto out; > +} > + > +pc_dimm_unplug(PC_DIMM(dev), MACHINE(vms)); > +object_property_set_bool(OBJECT(dev), false, "realized", NULL); Any reason why you did not use qdev_unrealize(dev) as in pc_dimm_unplug()? > + > + 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)) { > +
Re: [PATCH v3] arm/virt: Add memory hot remove support
Patchew URL: https://patchew.org/QEMU/20200618122129.7704-1-shameerali.kolothum.th...@huawei.com/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === CC qga/main.o CC qga/commands-posix.o CC qga/channel-posix.o /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) CC qga/qapi-generated/qga-qapi-visit.o CC qga/qapi-generated/qga-qapi-types.o CC qga/qapi-generated/qga-qapi-commands.o --- GEN docs/interop/qemu-ga-ref.html GEN docs/interop/qemu-ga-ref.txt GEN docs/interop/qemu-ga-ref.7 /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKqemu-ga LINKqemu-keymap /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) AS pc-bios/optionrom/multiboot.o AS pc-bios/optionrom/linuxboot.o CC pc-bios/optionrom/linuxboot_dma.o AS pc-bios/optionrom/kvmvapic.o /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) AS pc-bios/optionrom/pvh.o CC pc-bios/optionrom/pvh_main.o LINKivshmem-client --- SIGNpc-bios/optionrom/linuxboot.bin SIGNpc-bios/optionrom/linuxboot_dma.bin SIGNpc-bios/optionrom/kvmvapic.bin /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKqemu-storage-daemon /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) BUILD pc-bios/optionrom/pvh.img BUILD pc-bios/optionrom/pvh.raw SIGNpc-bios/optionrom/pvh.bin /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKqemu-img /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKqemu-io LINKqemu-edid /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) LINKfsdev/virtfs-proxy-helper LINKscsi/qemu-pr-helper LINKqemu-bridge-helper LINKvirtiofsd /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o) /usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from
[PATCH v3] arm/virt: Add memory hot remove support
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 --- 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, >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(>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 = _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..a981dc9f1c 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(errp, + "memory hotplug is not enabled: missing acpi-ged device"); +goto out; +} + +if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) { +error_setg(_err, + "nvdimm device hot unplug is not supported yet."); +goto out; +} + +hotplug_handler_unplug_request(HOTPLUG_HANDLER(vms->acpi_dev), dev, + _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, _err); +if (local_err) { +goto out; +} + +pc_dimm_unplug(PC_DIMM(dev), MACHINE(vms)); +object_property_set_bool(OBJECT(dev), false, "realized", NULL); + + 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 =