Hi Igor,
Thanks for the detailed review. Inline.
On 6/1/2026 4:50 PM, Igor Mammedov wrote:
On Wed, 27 May 2026 15:42:15 +0800
fanhuang <[email protected]> wrote:
Introduce a TYPE_MEMORY_DEVICE subclass `spm-memory` for boot-time
SOFT_RESERVED memory exposed to the guest with a per-device NUMA
proximity domain.
The device targets accelerator memory (HBM and similar) that the
firmware hands to the guest OS as SOFT_RESERVED memory, so a driver
in the guest -- rather than the kernel's general allocator -- owns
the range. Per-device NUMA placement matches the natural shape of
multiple HBM blocks (one block == one driver claim == one PXM).
[...]
CONFIG_SPM_MEMORY is selected by the i386 PC and Q35 machines
(same as DIMM).
this pass is mostly high level review.
patch is doing too much things at once,
Suggest to split it on several pieces,
1. introducing spm-memory boiler plate code
2. SRAT mangling
3. adding E820 entry
Will split as (1) spm-memory boilerplate, (2) SRAT mangling, and
(3) E820 + pc.c plug-handler integration; MAINTAINERS becomes (4).
MAINTAINERS gets new file entries under the existing "Memory devices"
stanza.
Signed-off-by: FangSheng Huang <[email protected]>
---
MAINTAINERS | 2 +
a separate patch, pls.
Yes, will be patch (4) on its own.
[...]
+ cursor = ms->device_memory->base;
+ end = cursor + memory_region_size(&ms->device_memory->mr);
+
+ object_child_foreach_recursive(qdev_get_machine(),
+ collect_spm_ranges_cb, ranges);
it's not an objection, but could we do better here, i.e. idea would be:
instead of full machine scan, take ms->device_memory and go over
children regions -> pick only SPM device owned ones.
Noted -- I'll stay with the current full-scan pattern for v9.
The subregion's owner is the backend rather than the device, so
a clean device_memory walk would need an extra backend->device
reverse lookup. Happy to add that if you'd prefer.
- /*
- * Entry is required for Windows to enable memory hotplug in OS
- * and for Linux to enable SWIOTLB when booted with less than
- * 4G of RAM. Windows works better if the entry sets proximity
- * to the highest NUMA node in the machine.
- * Memory devices may override proximity set by this entry,
- * providing _PXM method if necessary.
- */
don't just delete comment,as it still stands true. we should keep reminder
why we adding place holder(s) and its quirks.
Will keep an adapted version of the comment alongside the new
partition logic.
[...]
+static void spm_memory_realize(DeviceState *dev, Error **errp)
+{
+ ERRP_GUARD();
+ SpmMemoryDevice *spm = SPM_MEMORY(dev);
+ MachineState *ms = MACHINE(qdev_get_machine());
pls do not use machine from device proper code.
we do have plug handlers that provide it at the time when necessary.
Will remove the MachineState reference; machine-level work moves
to the pc.c plug handler (see below).
+
+ if (phase_check(PHASE_MACHINE_READY)) {
+ error_setg(errp, "spm-memory: hotplug is not supported "
+ "(boot-time-only device)");
+ return;
+ }
shouldn't be necessary, dc->hotpluggable in class init should be sufficient.
Confirmed -- will drop the check.
+ spm_memory_check_node_exclusive(spm, ms, errp);
+ if (*errp) {
+ return;
+ }
As far as I understood fro previous discussions, so far it's our
own precaution.
I'd drop that, well, if you find a spec requiring it then
it should be a separate patch pointing to spec (or something else that
justifies it).
No spec to cite. Will drop the check and the associated helpers
in v9.
+
+ memory_device_pre_plug(MEMORY_DEVICE(spm), ms, errp);
+ if (*errp) {
+ return;
+ }
+
+ host_memory_backend_set_mapped(spm->hostmem, true);
+ memory_device_plug(MEMORY_DEVICE(spm), ms);
That's basically code duplication,
that doesn't belong to realize_fn, see how it's used by other devices.
The gist is mapping into address space, generic checks, machine related
steps go into machine handlers.
Will move pre_plug / plug / set_mapped into pc_spm_memory_pre_plug /
pc_spm_memory_plug in hw/i386/pc.c, hooked via the existing
pc_machine_device_{pre_,}plug_cb dispatch (same pattern as pc-dimm).
+
+ QLIST_INSERT_HEAD(&spm_memory_list, spm, next);
Don't use global list, unless you have to, see below.
Goes away with the plug-handler move below.
+
+ if (!spm_machine_done_registered) {
+ spm_machine_done_notifier.notify = spm_memory_machine_done;
+ qemu_add_machine_init_done_notifier(&spm_machine_done_notifier);
+ spm_machine_done_registered = true;
+ }
e820 part should also go to machine specific plug handler,
that will also hel with getting rid of spm_memory_list.
That also should let you get rid of adding machine_done handler,
the machine plug handler, would do the job instead (and much earlier).
Yes -- e820_add_entry moves into pc_spm_memory_plug(), eliminating
both the global list and the machine-init-done notifier.
[...]
Will respin and post v9 shortly.
Best regards,
FangSheng Huang (Jerry)
