On 7/28/22, 7:21 AM, "Peter Krempa" <pkrempa@redhat.com> wrote:
>On Thu, Jul 28, 2022 at 16:05:08 +0200, Michal Prívozník wrote:
>> On 7/28/22 10:15, Peter Krempa wrote:
>> > On Wed, Jul 27, 2022 at 12:34:54 +0200, Michal Privoznik wrote:
>> >> From: Haibin Huang <haibin.huang@intel.com>
>> >>
>> >> Generate the QMP command for query-sgx-capabilities and the command
>> >> return SGX capabilities from QMP.
>> >>
>> >> {"execute":"query-sgx-capabilities"}
>> >>
>> >> the right reply:
>> >> {"return":
>> >> {
>> >> "sgx": true,
>> >> "section-size": 197132288,
>> >> "flc": true
>> >> }
>> >> }
>> >>
>> >> the error reply:
>> >> {"error":
>> >> {"class": "GenericError", "desc": "SGX is not enabled in KVM"}
>> >> }
>> >>
>> >> Signed-off-by: Haibin Huang <haibin.huang@intel.com>
>> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> >> ---
>> >> src/qemu/qemu_monitor.c | 10 ++++
>> >> src/qemu/qemu_monitor.h | 3 +
>> >> src/qemu/qemu_monitor_json.c | 107 +++++++++++++++++++++++++++++++++++
>> >> src/qemu/qemu_monitor_json.h | 4 ++
>> >> 4 files changed, 124 insertions(+)
>> >
>> > [...]
>> >
>> >> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> >> index 941596563a..b045efa203 100644
>> >> --- a/src/qemu/qemu_monitor_json.c
>> >> +++ b/src/qemu/qemu_monitor_json.c
>> >> @@ -6395,6 +6395,113 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitor *mon,
>> >> return 1;
>> >> }
>> >>
>> >> +
>> >> +/**
>> >> + * qemuMonitorJSONGetSGXCapabilities:
>> >> + * @mon: qemu monitor object
>> >> + * @capabilities: pointer to pointer to a SGX capability structure to be filled
>> >> + *
>> >> + * This function queries and fills in INTEL's SGX platform-specific data.
>> >> + * Note that from QEMU's POV both -object sgx-epc and query-sgx-capabilities
>> >> + * can be present even if SGX is not available, which basically leaves us with
>> >> + * checking for JSON "GenericError" in order to differentiate between compiled-in
>> >> + * support and actual SGX support on the platform.
>> >> + *
>> >> + * Returns: -1 on error,
>> >> + * 0 if SGX is not supported, and
>> >> + * 1 if SGX is supported on the platform.
>> >> + */
>> >> +int
>> >> +qemuMonitorJSONGetSGXCapabilities(qemuMonitor *mon,
>> >> + virSGXCapability **capabilities)
>> >> +{
>> >> + g_autoptr(virJSONValue) cmd = NULL;
>> >> + g_autoptr(virJSONValue) reply = NULL;
>> >> + g_autoptr(virSGXCapability) capability = NULL;
>> >> + virJSONValue *sections = NULL;
>> >> + virJSONValue *caps;
>> >> + bool flc = false;
>> >> + bool sgx1 = false;
>> >> + bool sgx2 = false;
>> >
>> > These temporary booleans feel a bit redundant ...
>> >
>> >> + unsigned long long section_size = 0;
>> >> + unsigned long long size;
>> >> + size_t i;
>> >> +
>> >> + *capabilities = NULL;
>> >> + capability = g_new0(virSGXCapability, 1);
>> >> +
>> >> + if (!(cmd = qemuMonitorJSONMakeCommand("query-sgx-capabilities", NULL)))
>> >> + return -1;
>> >> +
>> >> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
>> >> + return -1;
>> >> +
>> >> + /* QEMU has only compiled-in support of SGX */
>> >> + if (qemuMonitorJSONHasError(reply, "GenericError"))
>> >> + return 0;
>> >> +
>> >> + if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>> >> + return -1;
>> >> +
>> >> + caps = virJSONValueObjectGetObject(reply, "return");
>> >> +
>> >> + if (virJSONValueObjectGetBoolean(caps, "flc", &flc) < 0) {
>> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> >> + _("query-sgx-capabilities reply was missing 'flc' field"));
>> >> + return -1;
>> >> + }
>> >> + capability->flc = flc;
>> >
>> > Because you assign the value directly back to the struct. Passing the
>> > pointer to the field in the struct directly to
>> > virJSONValueObjectGetBoolean avoids the need.
>> >
>> >> +
>> >> + if (virJSONValueObjectGetBoolean(caps, "sgx1", &sgx1) < 0) {
>> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> >> + _("query-sgx-capabilities reply was missing 'sgx1' field"));
>> >> + return -1;
>> >> + }
>> >> + capability->sgx1 = sgx1;
>> >> +
>> >> + if (virJSONValueObjectGetBoolean(caps, "sgx2", &sgx2) < 0) {
>> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> >> + _("query-sgx-capabilities reply was missing 'sgx2' field"));
>> >> + return -1;
>> >> + }
>> >> + capability->sgx2 = sgx2;
>> >> +
>> >> + if (virJSONValueObjectGetNumberUlong(caps, "section-size", §ion_size) < 0) {
>> >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> >> + _("query-sgx-capabilities reply was missing 'section-size' field"));
>> >> + return -1;
>> >> + }
>> >> + capability->section_size = section_size / 1024;
>> >
>> > The 'section-size' field is marked as deprecated in the QMP schema. Thus
>> > we must not report error if it vanishes.
>> >
>> > Is there any reason to extract it in the first place?
>> >
>> > If yes, the code must be fixed to handle the possibility properly.
>>
>> The idea is that this allows us to work with qemu-6.2.0 and qemu-7.0.0;
>> The former reports section-size only, the latter marked it obsolete and
>> reports array of 'sections' so that sections per NUMA node can be
>> reported. Now, section-size is nothing but a sum of individual per NUMA
>> node sections. So I guess we can do the summation once QEMU stops
>> reporting it.
>
>Either way, we must not report an error if it is not present, because
>we'd specifically be adding code that will break in the future.
>
>> NB, presence of per NUMA node sections (this code below) is then used
>> when generating cmd line, because qemu-7.0.0 requires slightly different
>> cmd line (due to those NUMA nodes).
>>
>> Alternatively, we may pronounce qemu-6.2.0 not worth supporting and aim
>> on 7.0.0 only and not deal with deprecated interface at all (i.e. don't
>> parse/report aggregated sum).
>
>I'm definitely for skipping 6.2 if possible rather than have code which
>is going to work for one release only.
I see, let me drop 6.2 support and only aim on 7.0 in v15 patch.
@Michal, do you have any updated for v14 patches? If yes, I can rework on
top of your changes and submit for review.
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/sgx_rework ?
Or any way you preferred for this collaboration in this case?
Thanks,
Lin.