[PATCH v14 15/15] qemu: Add command-line to generate SGX EPC memory backend
Peter Krempa
pkrempa at redhat.com
Thu Jul 28 13:11:24 UTC 2022
On Wed, Jul 27, 2022 at 12:35:01 +0200, Michal Privoznik wrote:
> From: Lin Yang <lin.a.yang at intel.com>
>
> According to the result parsing from xml, add the argument of
> SGX EPC memory backend into QEMU command line.
>
> With NUMA config:
>
> #qemu-system-x86_64 \
> ...... \
> -object '{"qom-type":"memory-backend-epc","id":"memepc0","prealloc":true,"size":67108864,"host-nodes":[0,1],"policy":"bind"}' \
> -object '{"qom-type":"memory-backend-epc","id":"memepc1","prealloc":true,"size":16777216,"host-nodes":[2,3],"policy":"bind"}' \
> -machine sgx-epc.0.memdev=memepc0,sgx-epc.0.node=0,sgx-epc.1.memdev=memepc1,sgx-epc.1.node=1
>
> Without NUMA config:
>
> #qemu-system-x86_64 \
> ...... \
> -object '{"qom-type":"memory-backend-epc","id":"memepc0","prealloc":true,"size":67108864}' \
> -object '{"qom-type":"memory-backend-epc","id":"memepc1","prealloc":true,"size":16777216}' \
> -machine sgx-epc.0.memdev=memepc0,sgx-epc.1.memdev=memepc1
>
> Signed-off-by: Lin Yang <lin.a.yang at intel.com>
> Signed-off-by: Haibin Huang <haibin.huang at intel.com>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
> src/qemu/qemu_alias.c | 3 +-
> src/qemu/qemu_command.c | 82 +++++++++++++++++--
> src/qemu/qemu_monitor_json.c | 41 ++++++++--
> .../sgx-epc-numa.x86_64-latest.args | 40 +++++++++
> .../sgx-epc.x86_64-6.2.0.args | 37 +++++++++
> tests/qemuxml2argvtest.c | 3 +
> 6 files changed, 190 insertions(+), 16 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/sgx-epc-numa.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/sgx-epc.x86_64-6.2.0.args
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f3f6870c58..304a2ae6d1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
[...]
> @@ -3733,7 +3737,6 @@ qemuBuildMemoryBackendProps(virJSONValue **backendProps,
>
> if (systemMemory)
> disableCanonicalPath = true;
> -
> } else if (useHugepage || mem->nvdimmPath || memAccess ||
Spurious whitespace change.
> def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
[...]
> @@ -7094,8 +7100,52 @@ qemuAppendDomainMemoryMachineParams(virBuffer *buf,
> virBufferAddLit(buf, ",mem-merge=off");
>
> for (i = 0; i < def->nmems; i++) {
> - if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> - virBufferAddLit(buf, ",nvdimm=on");
> + int targetNode = def->mems[i]->targetNode;
> +
> + switch (def->mems[i]->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> + if (!nvdimmAdded) {
> + virBufferAddLit(buf, ",nvdimm=on");
> + nvdimmAdded = true;
> + }
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
> + /* add sgx epc memory to -machine parameter */
> +
> + if (targetNode < 0) {
> + /* set NUMA target node to 0 by default if user doesn't
> + * specify it. */
> + targetNode = 0;
> + }
> +
> + if (sgxCaps->nsections == 0) {
> + /* Assume QEMU cannot specify guest NUMA node for each SGX EPC,
> + * because it doesn't provide EPC NUMA info
> + */
The previous patches in this series spend quite a lot of effort in
extracting the section sizes, but they are not used for anything related
to the actual command generation besides checking the presence.
What is the point of the size reported by qemu then? Is the user
supposed to use it somehow? If there are restrictions, e.g. by requiring
the user to set the size as multiple of the value reported in
capabilities I'd expect that the code would validate that in such case.
> + if (targetNode > 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("can't add SGX EPC for guest node '%d' because this QEMU version didn't provide SGX EPC NUMA info"),
> + targetNode);
> + return -1;
> + }
> +
> + virBufferAsprintf(buf, ",sgx-epc.%d.memdev=mem%s",
> + epcNum, def->mems[i]->info.alias);
> + } else {
> + /* The .node attribute is required since QEMU provide EPC NUMA info */
> + virBufferAsprintf(buf, ",sgx-epc.%d.memdev=mem%s,sgx-epc.%d.node=%d",
> + epcNum, def->mems[i]->info.alias, epcNum, targetNode);
> + }
> +
> + epcNum++;
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> break;
> }
> }
[...]
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index b045efa203..c41a6355ba 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -7575,13 +7575,25 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon,
> return -1;
> }
>
> - /* While 'id' attribute is marked as optional in QEMU's QAPI
> - * specification, Libvirt always sets it. Thus we can fail if not
> - * present. */
> - if (!(devalias = virJSONValueObjectGetString(dimminfo, "id"))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("dimm memory info data is missing 'id'"));
> - return -1;
> + if (STREQ(type, "dimm") || STREQ(type, "nvdimm") || STREQ(type, "virtio-mem")) {
> + /* While 'id' attribute is marked as optional in QEMU's QAPI
> + * specification, Libvirt always sets it. Thus we can fail if not
> + * present. */
> + if (!(devalias = virJSONValueObjectGetString(dimminfo, "id"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("dimm memory info data is missing 'id'"));
> + return -1;
> + }
> + } else if (STREQ(type, "sgx-epc")) {
> + if (!(devalias = virJSONValueObjectGetString(dimminfo, "memdev"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("sgx-epc memory info data is missing 'memdev'"));
> + return -1;
This code is really looking for the alias of the device frontend, so
that we can match it later on, so looking for the alias of the backend
seems wrong.
Based on the comment below I dont' think this code should even handle
SGX related objects at all.
> + }
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s memory device info is not handled yet"), type);
> + return -1;
> }
>
> meminfo = g_new0(qemuMonitorMemoryDeviceInfo, 1);
> @@ -7625,6 +7637,21 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon,
> _("malformed/missing size in virtio memory info"));
> return -1;
> }
> + } else if (STREQ(type, "sgx-epc")) {
> + /* sgx-epc memory devices */
> + if (virJSONValueObjectGetNumberUlong(dimminfo, "memaddr",
> + &meminfo->address) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed/missing memaddr in sgx-epc memory info"));
In patch 11/15 in code added to virDomainMemoryDefValidate it's
explicitly forbidden to provide address for the 'sgx-epc' memory device,
thus I don't really understand why you collect it here. Can you please
clarify?
The code adding the machine parameters in
qemuAppendDomainMemoryMachineParams doesn't support passing address to
qemu, so if this was needed e.g. for migration it wouldn't even work.
> + return -1;
> + }
> +
> + if (virJSONValueObjectGetNumberUlong(dimminfo, "size",
> + &meminfo->size) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed/missing size in sgx-epc memory info"));
> + return -1;
> + }
> } else {
> /* type not handled yet */
> continue;
[...]
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 05537d9e96..7a4c09b172 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3470,6 +3470,9 @@ mymain(void)
> /* HVF guests should not work on Linux with KVM */
> DO_TEST_CAPS_LATEST_PARSE_ERROR("hvf-x86_64-q35-headless");
>
> + DO_TEST_CAPS_VER("sgx-epc", "6.2.0");
> + DO_TEST_CAPS_LATEST("sgx-epc-numa");
As noted before, you need to make this test pinned to 7.0.0 or else it
will break when I re-generate capabilities as my box does not support
SGX.
More information about the libvir-list
mailing list