Hi Shameer, On 3/26/20 12:14 PM, Shameerali Kolothum Thodi wrote: > Hi Eric, > >> -----Original Message----- >> From: Auger Eric [mailto:[email protected]] >> Sent: 26 March 2020 11:01 >> To: Shameerali Kolothum Thodi <[email protected]>; >> [email protected]; [email protected] >> Cc: [email protected]; [email protected]; [email protected]; >> xuwei (O) <[email protected]>; Zengtao (B) <[email protected]>; >> Linuxarm <[email protected]>; Anshuman Khandual >> <[email protected]> >> Subject: Re: [RFC v1] arm/virt: Add memory hot remove support >> >> Hi Shameer, >> >> On 3/18/20 1:37 PM, Shameer Kolothum wrote: >>> This adds support for memory hot remove on arm/virt that >>> uses acpi ged device. >> >> I gave this a try and it works fine if the PCDIMM slot was initially >> hotplugged: >> (QEMU) object-add qom-type=memory-backend-ram id=mem1 >> props.size=4294967296 >> {"return": {}} >> (QEMU) device_add driver=pc-dimm id=pcdimm1 memdev=mem1 >> (QEMU) device_del id=pcdimm1 >> {"return": {}} >> >> on guest I can see: >> [ 82.466321] Offlined Pages 262144 >> [ 82.541712] Offlined Pages 262144 >> [ 82.589236] Offlined Pages 262144 >> [ 82.969166] Offlined Pages 262144 >> >> However I noticed that if qemu is launched directly with >> >> -m 16G,maxmem=32G,slots=2 \ >> -object memory-backend-ram,id=mem1,size=4G \ >> -device pc-dimm,memdev=mem1,id=dimm1,driver=pc-dimm -device >> >> and then in the qmp shell: >> (QEMU) device_del id=dimm1 >> >> the hot-unplug fails in guest: >> >> [ 78.897407] Offlined Pages 262144 >> [ 79.260811] Offlined Pages 262144 >> [ 79.308105] Offlined Pages 262144 >> [ 79.333675] page:fffffe00137d1f40 refcount:1 mapcount:0 >> mapping:ffff0004ea9f20b1 index:0xaaab11c6e >> [ 79.335927] anon flags: 0x17ffff8000080024(uptodate|active|swapbacked) >> [ 79.337571] raw: 17ffff8000080024 dead000000000100 >> dead000000000122 >> ffff0004ea9f20b1 >> [ 79.339502] raw: 0000000aaab11c6e 0000000000000000 00000001ffffffff >> ffff0004fd4e3000 >> [ 79.341701] page dumped because: unmovable page >> [ 79.342887] page->mem_cgroup:ffff0004fd4e3000 >> [ 79.354729] page:fffffe00137d1f40 refcount:1 mapcount:0 >> mapping:ffff0004ea9f20b1 index:0xaaab11c6e >> [ 79.357012] anon flags: 0x17ffff8000080024(uptodate|active|swapbacked) >> [ 79.358658] raw: 17ffff8000080024 dead000000000100 >> dead000000000122 >> ffff0004ea9f20b1 >> [ 79.360611] raw: 0000000aaab11c6e 0000000000000000 00000001ffffffff >> ffff0004fd4e3000 >> [ 79.362560] page dumped because: unmovable page >> [ 79.363742] page->mem_cgroup:ffff0004fd4e3000 >> [ 79.368636] memory memory20: Offline failed. >> >> I did not expect this. The PCDIMM slot in that case does not seem to be >> interpreted as a hot-unpluggable one (?). I added Anshuman in cc. > > Could you please try adding "movable_node" to qemu guest kernel command line > params. > This will prevent any kernel allocation from hotplugable memory nodes which I > think is > causing the behavior you are seeing.
Effectively, when adding the movable_node option in the guest kernel parameters, I get the following traces: [ 29.581418] Offlined Pages 262144 [ 29.663605] Offlined Pages 262144 [ 29.714225] Offlined Pages 262144 [ 30.222953] Offlined Pages 262144 [ 30.314288] Built 1 zonelists, mobility grouping on. Total pages: 4076898 [ 30.316067] Policy zone: Normal Thanks Eric > > Thanks, > Shameer > > >> Thanks >> >> Eric >> >> >> >>> >>> Signed-off-by: Shameer Kolothum <[email protected]> >>> --- >>> -RFC because linux kernel support for mem hot remove is just queued >>> for 5.7[1]. >>> -Tested with guest kernel 5.6-rc5 + [1] >>> >>> 1. https://patchwork.kernel.org/cover/11419301/ >>> --- >>> hw/acpi/generic_event_device.c | 28 +++++++++++++++++ >>> hw/arm/virt.c | 56 >> ++++++++++++++++++++++++++++++++-- >>> 2 files changed, 82 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/acpi/generic_event_device.c >> b/hw/acpi/generic_event_device.c >>> index 021ed2bf23..3e28c110fa 100644 >>> --- a/hw/acpi/generic_event_device.c >>> +++ b/hw/acpi/generic_event_device.c >>> @@ -182,6 +182,32 @@ 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)) { >>> + 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); >>> @@ -286,6 +312,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 94f93dda54..91974e4e80 100644 >>> --- a/hw/arm/virt.c >>> +++ b/hw/arm/virt.c >>> @@ -2096,11 +2096,62 @@ 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; >>> + } >>> + >>> + 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)); >>> + 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, >>> @@ -2181,6 +2232,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->auto_enable_numa_with_memhp = true; >>> mc->default_ram_id = "mach-virt.ram"; >>> >
