On Tue, Nov 25, 2025 at 11:17:03 -0800, Nathan Chen via Devel wrote:
> Add qemu CLI support for EGM memory device model:
> - Specify EGM device path to memory-backend-file object
> - Support acpi-egm-memory object with id, pci-dev, and
> node attributes
> - Consolidate all acpi-egm-memory objects' memory into
> a single memory-backend-file per EGM chardev
> specified.
>
> Signed-off-by: Ian May <[email protected]>
> Signed-off-by: Nathan Chen <[email protected]>
> ---
> src/qemu/qemu_alias.c | 7 +-
> src/qemu/qemu_capabilities.c | 2 +
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_command.c | 158 ++++++++++++++++++++++++++++++---
> src/qemu/qemu_domain.c | 13 ++-
> src/qemu/qemu_domain_address.c | 3 +
> src/qemu/qemu_driver.c | 1 +
> src/qemu/qemu_hotplug.c | 1 +
> src/qemu/qemu_monitor_json.c | 1 +
> src/qemu/qemu_postparse.c | 1 +
> src/qemu/qemu_process.c | 2 +
> src/qemu/qemu_validate.c | 6 ++
> 12 files changed, 180 insertions(+), 16 deletions(-)
Note that I'm replying to this patch just due to the issue with qemu
capabilities. This is not a full review.
[...]
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index f180844e66..3eb12235f4 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -730,6 +730,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
> syntax-check */
> QEMU_CAPS_DISK_TIMED_STATS, /* timed stats support ('stats-intervals'
> property of disk frontends) */
> QEMU_CAPS_QUERY_ACCELERATORS, /* query-accelerators command */
> QEMU_CAPS_MSHV, /* -accel mshv */
> + QEMU_CAPS_DEVICE_ACPI_EGM_MEMORY, /* For using extended GPU memory */
>
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
For any further postin please separate the addition of the capability
along with the detection and any change to the detected capabilities
into a separate patch.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b69fe23236..33848aa781 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3573,12 +3593,17 @@ qemuBuildMemoryDimmBackendStr(virCommand *cmd,
> virDomainMemoryDef *mem,
> virDomainDef *def,
> virQEMUDriverConfig *cfg,
> - qemuDomainObjPrivate *priv)
> + qemuDomainObjPrivate *priv,
> + GHashTable *egmBackends)
> {
> g_autoptr(virJSONValue) props = NULL;
> g_autoptr(virJSONValue) tcProps = NULL;
> virBitmap *nodemask = NULL;
> g_autofree char *alias = NULL;
> + unsigned long long originalSize = 0;
> + bool isEGM = (mem->model == VIR_DOMAIN_MEMORY_MODEL_EGM);
> + bool shouldCreateBackend = true;
> + qemuEGMBackendInfo *egmInfo = NULL;
>
> if (!mem->info.alias) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -3588,19 +3613,65 @@ qemuBuildMemoryDimmBackendStr(virCommand *cmd,
>
> alias = g_strdup_printf("mem%s", mem->info.alias);
>
> - if (qemuBuildMemoryBackendProps(&props, alias, cfg, priv,
> - def, mem, true, false, &nodemask) < 0)
> - return -1;
> + /* Handle EGM shared backend logic */
> + if (isEGM && egmBackends) {
> + const char *egmPath = mem->source.egm.path;
> + egmInfo = g_hash_table_lookup(egmBackends, egmPath);
>
> - if (qemuBuildThreadContextProps(&tcProps, &props, def, priv, nodemask) <
> 0)
> - return -1;
> + if (egmInfo) {
> + alias = g_strdup(egmInfo->alias);
> + if (egmInfo->created) {
> + /* Backend already created, skip backend creation */
> + shouldCreateBackend = false;
> + } else {
> + /* First device for this path - temporarily use accumulated
> size */
> + originalSize = mem->size;
> + mem->size = egmInfo->totalSize;
> + egmInfo->created = true;
> + }
> + }
> + }
>
> - if (tcProps &&
> - qemuBuildObjectCommandlineFromJSON(cmd, tcProps) < 0)
> - return -1;
> + if (shouldCreateBackend) {
> + /* Use existing function unchanged */
> + if (qemuBuildMemoryBackendProps(&props, alias, cfg, priv,
> + def, mem, true, false, &nodemask) <
> 0) {
> + if (originalSize > 0)
> + mem->size = originalSize; /* Restore on error */
> + return -1;
> + }
>
> - if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
> - return -1;
> + /* Restore original size after backend props are built */
> + if (originalSize > 0)
> + mem->size = originalSize;
> +
> + if (qemuBuildThreadContextProps(&tcProps, &props, def, priv,
> nodemask) < 0)
> + return -1;
> +
> + if (tcProps &&
> + qemuBuildObjectCommandlineFromJSON(cmd, tcProps) < 0)
> + return -1;
> +
> + if (qemuBuildObjectCommandlineFromJSON(cmd, props) < 0)
> + return -1;
> + }
> +
> + if (isEGM) {
> + g_autofree char *egmObjStr = NULL;
> + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> +
> + virBufferAsprintf(&buf, "acpi-egm-memory,id=%s", mem->info.alias);
> +
> + if (mem->target.egm.pciDev)
> + virBufferAsprintf(&buf, ",pci-dev=%s", mem->target.egm.pciDev);
> +
> + if (mem->targetNode >= 0)
> + virBufferAsprintf(&buf, ",node=%d", mem->targetNode);
> +
> + egmObjStr = virBufferContentAndReset(&buf);
> +
> + virCommandAddArgList(cmd, "-object", egmObjStr, NULL);
> + }
>
> return 0;
> }
[...]
> @@ -7835,6 +7910,37 @@ qemuBuildNumaCommandLine(virQEMUDriverConfig *cfg,
> hmat = true;
> }
>
> + /* Pre-scan EGM devices to group by path and calculate total sizes */
> + egmBackends = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> +
> (GDestroyNotify)qemuEGMBackendInfoFree);
For creating hash tables use virHashNew, which uses our hashing
function.
> +
> + for (i = 0; i < def->nmems; i++) {
> + if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_EGM) {
> + const char *egmPath = def->mems[i]->source.egm.path;
> + qemuEGMBackendInfo *info = g_hash_table_lookup(egmBackends,
> egmPath);
> +
> + if (!info) {
> + info = g_new0(qemuEGMBackendInfo, 1);
> + info->alias = g_strdup_printf("memegm%zu", egmBackendCount);
> + egmBackendCount++;
> + info->totalSize = def->mems[i]->size;
> + info->created = false;
> + info->firstMem = def->mems[i];
> + g_hash_table_insert(egmBackends, g_strdup(egmPath), info);
> + } else {
> + info->totalSize += def->mems[i]->size;
> + }
> + }
> + }
> +
> + /* Build the actual backend and device objects */
> + for (i = 0; i < def->nmems; i++) {
> + if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_EGM) {
> + if (qemuBuildMemoryDimmBackendStr(cmd, def->mems[i], def, cfg,
> priv, egmBackends) < 0)
> + goto cleanup;
> + }
> + }
> +
> nodeBackends = g_new0(virJSONValue *, ncells);
> nodemask = g_new0(virBitmap *, ncells);
>
> @@ -7870,8 +7976,18 @@ qemuBuildNumaCommandLine(virQEMUDriverConfig *cfg,
> for (i = 0; i < ncells; i++) {
> ssize_t initiator = virDomainNumaGetNodeInitiator(def->numa, i);
> unsigned long long memSize =
> virDomainNumaGetNodeMemorySize(def->numa, i);
> + bool egmBacked = false;
> + size_t k;
> +
> + for (k = 0; k < def->nmems; k++) {
> + if (def->mems[k]->model == VIR_DOMAIN_MEMORY_MODEL_EGM &&
> + def->mems[k]->targetNode == (int)i) {
> + egmBacked = true;
> + break;
> + }
> + }
>
> - if (needBackend && memSize > 0) {
> + if (needBackend && memSize > 0 && !egmBacked) {
> g_autoptr(virJSONValue) tcProps = NULL;
>
> if (qemuBuildThreadContextProps(&tcProps, &nodeBackends[i],
> @@ -7901,7 +8017,15 @@ qemuBuildNumaCommandLine(virQEMUDriverConfig *cfg,
>
> if (memSize > 0) {
> if (needBackend) {
> - virBufferAsprintf(&buf, ",memdev=ram-node%zu", i);
> + if (egmBacked) {
> + /* Look up the actual backend alias for EGM */
> + const char *egmPath = def->mems[k]->source.egm.path;
> + qemuEGMBackendInfo *egmInfo =
> g_hash_table_lookup(egmBackends, egmPath);
> + const char *backendAlias = egmInfo ? egmInfo->alias :
> def->mems[k]->info.alias;
> + virBufferAsprintf(&buf, ",memdev=%s", backendAlias);
> + } else {
> + virBufferAsprintf(&buf, ",memdev=ram-node%zu", i);
> + }
> } else {
> virBufferAsprintf(&buf, ",mem=%llu", memSize / 1024);
> }
Hmm 'qemuBuildMemoryDimmBackendStr' and 'qemuBuildNumaCommandLine' are
getting quite out of hand these patches (they're kind of a mess already
in the current state) and all of that to support some niche hardware.
Please consider refactoring the code first to simplify it. This will
be a nightmare to review otherwise.
