[PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX

[PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX

[Yang Zhong]

This patchset supply HMP/QMP interfaces to monitor and Libvirt, with
those interfaces, we can check the SGX info from VM side or check
host SGX capabilities from Libvirt side.

This patchset is splitted from below link(from patch26 to patch30):
https://patchew.org/QEMU/20210719112136.57018-1-yang.zhong@intel.com/

The rest patches are being pulled by Paolo's below link and this new
patchset is based on it.
https://gitlab.com/bonzini/qemu.git tags/for-upstream

Changelog:
=========
v1-->v2:
  - Squashed patch1~patch3 to new patch1(Philippe).
  - Removed the bitops definitions patch, and used the MAKE_64BIT_MASK()
    to handle(Philippe).
  - Removed the 'Fix coredump issue in non-x86 platform' patch and new
    fix in the sgx_get_info()(Paolo).

Qemu SGX-->v1:
patch1~patch5:
  - Moved HMP/QMP from ./monitor to target/i386/monitor.c(Paolo)
  - Added one include/hw/i386/sgx.h to include APIs to avoid "TARGET_XXXX"
    poisoned issue in the ./qapi/qapi-types-misc-target.h.
  - Removed the stubs/sgx-stubs file for non-X86 build issues.
  - Moved the jason definitions from qapi/misc.jason to qapi/misc-target.json, and
    Changed version 6.1 to 6.2.
patch 6:
  - This new patch to fix coredump issue in non-x86 platform.
patch 7:
  - Pure cleanup patch, the issue was caused by cherry-pick tool.


Yang Zhong (3):
  monitor: Add HMP and QMP interfaces
  qmp: Add the qmp_query_sgx_capabilities()
  pc: Cleanup the SGX definitions

 hmp-commands-info.hx         | 15 ++++++
 hw/i386/sgx.c                | 95 ++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h         | 11 +++--
 include/hw/i386/sgx.h        | 12 +++++
 include/monitor/hmp-target.h |  1 +
 qapi/misc-target.json        | 61 +++++++++++++++++++++++
 target/i386/monitor.c        | 41 ++++++++++++++++
 tests/qtest/qmp-cmd-test.c   |  2 +
 8 files changed, 233 insertions(+), 5 deletions(-)
 create mode 100644 include/hw/i386/sgx.h

[PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Yang Zhong]

The QMP and HMP interfaces can be used by monitor or QMP tools to retrieve
the SGX information from VM side when SGX is enabled on Intel platform.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 hmp-commands-info.hx         | 15 +++++++++++++
 hw/i386/sgx.c                | 29 ++++++++++++++++++++++++
 include/hw/i386/sgx.h        | 11 +++++++++
 include/monitor/hmp-target.h |  1 +
 qapi/misc-target.json        | 43 ++++++++++++++++++++++++++++++++++++
 target/i386/monitor.c        | 36 ++++++++++++++++++++++++++++++
 tests/qtest/qmp-cmd-test.c   |  1 +
 7 files changed, 136 insertions(+)
 create mode 100644 include/hw/i386/sgx.h

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 27206ac049..4c966e8a6b 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -877,3 +877,18 @@ SRST
   ``info dirty_rate``
     Display the vcpu dirty rate information.
 ERST
+
+#if defined(TARGET_I386)
+    {
+        .name       = "sgx",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show intel SGX information",
+        .cmd        = hmp_info_sgx,
+    },
+#endif
+
+SRST
+  ``info sgx``
+    Show intel SGX information.
+ERST
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 02fa6487c3..8a32d62d7e 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -17,6 +17,35 @@
 #include "monitor/qdev.h"
 #include "qapi/error.h"
 #include "exec/address-spaces.h"
+#include "hw/i386/sgx.h"
+
+SGXInfo *sgx_get_info(void)
+{
+    SGXInfo *info = NULL;
+    X86MachineState *x86ms;
+    PCMachineState *pcms =
+        (PCMachineState *)object_dynamic_cast(qdev_get_machine(),
+                                              TYPE_PC_MACHINE);
+    if (!pcms) {
+        return NULL;
+    }
+
+    x86ms = X86_MACHINE(pcms);
+    if (!x86ms->sgx_epc_list) {
+        return NULL;
+    }
+
+    SGXEPCState *sgx_epc = &pcms->sgx_epc;
+    info = g_new0(SGXInfo, 1);
+
+    info->sgx = true;
+    info->sgx1 = true;
+    info->sgx2 = true;
+    info->flc = true;
+    info->section_size = sgx_epc->size;
+
+    return info;
+}
 
 int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
diff --git a/include/hw/i386/sgx.h b/include/hw/i386/sgx.h
new file mode 100644
index 0000000000..ea8672f8eb
--- /dev/null
+++ b/include/hw/i386/sgx.h
@@ -0,0 +1,11 @@
+#ifndef QEMU_SGX_H
+#define QEMU_SGX_H
+
+#include "qom/object.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qapi/qapi-types-misc-target.h"
+
+SGXInfo *sgx_get_info(void);
+
+#endif
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 60fc92722a..dc53add7ee 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -49,5 +49,6 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict);
 void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
 void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
+void hmp_info_sgx(Monitor *mon, const QDict *qdict);
 
 #endif /* MONITOR_HMP_TARGET_H */
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 3b05ad3dbf..e2a347cc23 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -333,3 +333,46 @@
 { 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
   'returns': 'SevAttestationReport',
   'if': 'TARGET_I386' }
+
+##
+# @SGXInfo:
+#
+# Information about intel Safe Guard eXtension (SGX) support
+#
+# @sgx: true if SGX is supported
+#
+# @sgx1: true if SGX1 is supported
+#
+# @sgx2: true if SGX2 is supported
+#
+# @flc: true if FLC is supported
+#
+# @section-size: The EPC section size for guest
+#
+# Since: 6.2
+##
+{ 'struct': 'SGXInfo',
+  'data': { 'sgx': 'bool',
+            'sgx1': 'bool',
+            'sgx2': 'bool',
+            'flc': 'bool',
+            'section-size': 'uint64'},
+   'if': 'TARGET_I386' }
+
+##
+# @query-sgx:
+#
+# Returns information about SGX
+#
+# Returns: @SGXInfo
+#
+# Since: 6.2
+#
+# Example:
+#
+# -> { "execute": "query-sgx" }
+# <- { "return": { "sgx": true, "sgx1" : true, "sgx2" : true,
+#                  "flc": true, "section-size" : 0 } }
+#
+##
+{ 'command': 'query-sgx', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 119211f0b0..0f1b48b4f8 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -35,6 +35,7 @@
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qapi-commands-misc.h"
 #include "hw/i386/pc.h"
+#include "hw/i386/sgx.h"
 
 /* Perform linear address sign extension */
 static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
@@ -763,3 +764,38 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
 {
     return sev_get_attestation_report(mnonce, errp);
 }
+
+SGXInfo *qmp_query_sgx(Error **errp)
+{
+    SGXInfo *info;
+
+    info = sgx_get_info();
+    if (!info) {
+        error_setg(errp, "SGX features are not available");
+        return NULL;
+    }
+
+    return info;
+}
+
+void hmp_info_sgx(Monitor *mon, const QDict *qdict)
+{
+    SGXInfo *info = qmp_query_sgx(NULL);
+
+    if (info && info->sgx) {
+        monitor_printf(mon, "SGX support: %s\n",
+                       info->sgx ? "enabled" : "disabled");
+        monitor_printf(mon, "SGX1 support: %s\n",
+                       info->sgx1 ? "enabled" : "disabled");
+        monitor_printf(mon, "SGX2 support: %s\n",
+                       info->sgx2 ? "enabled" : "disabled");
+        monitor_printf(mon, "FLC support: %s\n",
+                       info->flc ? "enabled" : "disabled");
+        monitor_printf(mon, "size: %" PRIu64 "\n",
+                       info->section_size);
+    } else {
+        monitor_printf(mon, "SGX is not enabled\n");
+    }
+
+    qapi_free_SGXInfo(info);
+}
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index c98b78d033..b75f3364f3 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -100,6 +100,7 @@ static bool query_is_ignored(const char *cmd)
         /* Success depends on Host or Hypervisor SEV support */
         "query-sev",
         "query-sev-capabilities",
+        "query-sgx",
         NULL
     };
     int i;

[PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities()

[Yang Zhong]

Libvirt can use qmp_query_sgx_capabilities() to get the host
sgx capabilities to decide how to allocate SGX EPC size to VM.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 hw/i386/sgx.c              | 66 ++++++++++++++++++++++++++++++++++++++
 include/hw/i386/sgx.h      |  1 +
 qapi/misc-target.json      | 18 +++++++++++
 target/i386/monitor.c      |  5 +++
 tests/qtest/qmp-cmd-test.c |  1 +
 5 files changed, 91 insertions(+)

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 8a32d62d7e..1be2670c84 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -18,6 +18,72 @@
 #include "qapi/error.h"
 #include "exec/address-spaces.h"
 #include "hw/i386/sgx.h"
+#include "sysemu/hw_accel.h"
+
+#define SGX_MAX_EPC_SECTIONS            8
+#define SGX_CPUID_EPC_INVALID           0x0
+
+/* A valid EPC section. */
+#define SGX_CPUID_EPC_SECTION           0x1
+#define SGX_CPUID_EPC_MASK              0xF
+
+static uint64_t sgx_calc_section_metric(uint64_t low, uint64_t high)
+{
+    return (low & MAKE_64BIT_MASK(12, 31 - 12 + 1)) +
+           ((high & MAKE_64BIT_MASK(0, 19 - 0 + 1)) << 32);
+}
+
+static uint64_t sgx_calc_host_epc_section_size(void)
+{
+    uint32_t i, type;
+    uint32_t eax, ebx, ecx, edx;
+    uint64_t size = 0;
+
+    for (i = 0; i < SGX_MAX_EPC_SECTIONS; i++) {
+        host_cpuid(0x12, i + 2, &eax, &ebx, &ecx, &edx);
+
+        type = eax & SGX_CPUID_EPC_MASK;
+        if (type == SGX_CPUID_EPC_INVALID) {
+            break;
+        }
+
+        if (type != SGX_CPUID_EPC_SECTION) {
+            break;
+        }
+
+        size += sgx_calc_section_metric(ecx, edx);
+    }
+
+    return size;
+}
+
+SGXInfo *sgx_get_capabilities(Error **errp)
+{
+    SGXInfo *info = NULL;
+    uint32_t eax, ebx, ecx, edx;
+
+    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
+    if (fd < 0) {
+        error_setg(errp, "SGX is not enabled in KVM");
+        return NULL;
+    }
+
+    info = g_new0(SGXInfo, 1);
+    host_cpuid(0x7, 0, &eax, &ebx, &ecx, &edx);
+
+    info->sgx = ebx & (1U << 2) ? true : false;
+    info->flc = ecx & (1U << 30) ? true : false;
+
+    host_cpuid(0x12, 0, &eax, &ebx, &ecx, &edx);
+    info->sgx1 = eax & (1U << 0) ? true : false;
+    info->sgx2 = eax & (1U << 1) ? true : false;
+
+    info->section_size = sgx_calc_host_epc_section_size();
+
+    close(fd);
+
+    return info;
+}
 
 SGXInfo *sgx_get_info(void)
 {
diff --git a/include/hw/i386/sgx.h b/include/hw/i386/sgx.h
index ea8672f8eb..28437cffc6 100644
--- a/include/hw/i386/sgx.h
+++ b/include/hw/i386/sgx.h
@@ -7,5 +7,6 @@
 #include "qapi/qapi-types-misc-target.h"
 
 SGXInfo *sgx_get_info(void);
+SGXInfo *sgx_get_capabilities(Error **errp);
 
 #endif
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index e2a347cc23..594fbd1577 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -376,3 +376,21 @@
 #
 ##
 { 'command': 'query-sgx', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
+
+##
+# @query-sgx-capabilities:
+#
+# Returns information from host SGX capabilities
+#
+# Returns: @SGXInfo
+#
+# Since: 6.2
+#
+# Example:
+#
+# -> { "execute": "query-sgx-capabilities" }
+# <- { "return": { "sgx": true, "sgx1" : true, "sgx2" : true,
+#                  "flc": true, "section-size" : 0 } }
+#
+##
+{ 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 0f1b48b4f8..23a6dc3b7d 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -799,3 +799,8 @@ void hmp_info_sgx(Monitor *mon, const QDict *qdict)
 
     qapi_free_SGXInfo(info);
 }
+
+SGXInfo *qmp_query_sgx_capabilities(Error **errp)
+{
+    return sgx_get_capabilities(errp);
+}
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index b75f3364f3..1af2f74c28 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -101,6 +101,7 @@ static bool query_is_ignored(const char *cmd)
         "query-sev",
         "query-sev-capabilities",
         "query-sgx",
+        "query-sgx-capabilities",
         NULL
     };
     int i;

[PATCH v2 3/3] pc: Cleanup the SGX definitions

[Yang Zhong]

This patch only cleanup SGX definitions in the the pc.h file.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 include/hw/i386/pc.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a5ae380b4b..4c77489961 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -195,17 +195,18 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry);
 
+/* sgx.c */
+void pc_machine_init_sgx_epc(PCMachineState *pcms);
+/* hostmem-epc.c */
+void sgx_memory_backend_reset(HostMemoryBackend *backend, int fd,
+                              Error **errp);
+
 extern GlobalProperty pc_compat_6_1[];
 extern const size_t pc_compat_6_1_len;
 
 extern GlobalProperty pc_compat_6_0[];
 extern const size_t pc_compat_6_0_len;
 
-/* sgx-epc.c */
-void pc_machine_init_sgx_epc(PCMachineState *pcms);
-void sgx_memory_backend_reset(HostMemoryBackend *backend, int fd,
-                              Error **errp);
-
 extern GlobalProperty pc_compat_5_2[];
 extern const size_t pc_compat_5_2_len;
 

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Philippe Mathieu-Daudé]

On 9/10/21 12:22 PM, Yang Zhong wrote:
> The QMP and HMP interfaces can be used by monitor or QMP tools to retrieve
> the SGX information from VM side when SGX is enabled on Intel platform.
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  hmp-commands-info.hx         | 15 +++++++++++++
>  hw/i386/sgx.c                | 29 ++++++++++++++++++++++++
>  include/hw/i386/sgx.h        | 11 +++++++++
>  include/monitor/hmp-target.h |  1 +
>  qapi/misc-target.json        | 43 ++++++++++++++++++++++++++++++++++++
>  target/i386/monitor.c        | 36 ++++++++++++++++++++++++++++++
>  tests/qtest/qmp-cmd-test.c   |  1 +
>  7 files changed, 136 insertions(+)
>  create mode 100644 include/hw/i386/sgx.h

> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 3b05ad3dbf..e2a347cc23 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -333,3 +333,46 @@
>  { 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
>    'returns': 'SevAttestationReport',
>    'if': 'TARGET_I386' }
> +
> +##
> +# @SGXInfo:
> +#
> +# Information about intel Safe Guard eXtension (SGX) support
> +#
> +# @sgx: true if SGX is supported
> +#
> +# @sgx1: true if SGX1 is supported
> +#
> +# @sgx2: true if SGX2 is supported
> +#
> +# @flc: true if FLC is supported
> +#
> +# @section-size: The EPC section size for guest
> +#
> +# Since: 6.2
> +##
> +{ 'struct': 'SGXInfo',
> +  'data': { 'sgx': 'bool',
> +            'sgx1': 'bool',
> +            'sgx2': 'bool',
> +            'flc': 'bool',
> +            'section-size': 'uint64'},
> +   'if': 'TARGET_I386' }

Is it possible to restrict it to KVM? Maybe:

     'if': { 'all': ['TARGET_I386', 'CONFIG_KVM'] } },

? (I'm not sure).

Re: [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities()

[Philippe Mathieu-Daudé]

On 9/10/21 12:22 PM, Yang Zhong wrote:
> Libvirt can use qmp_query_sgx_capabilities() to get the host
> sgx capabilities to decide how to allocate SGX EPC size to VM.
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  hw/i386/sgx.c              | 66 ++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/sgx.h      |  1 +
>  qapi/misc-target.json      | 18 +++++++++++
>  target/i386/monitor.c      |  5 +++
>  tests/qtest/qmp-cmd-test.c |  1 +
>  5 files changed, 91 insertions(+)
> 
> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index 8a32d62d7e..1be2670c84 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -18,6 +18,72 @@
>  #include "qapi/error.h"
>  #include "exec/address-spaces.h"
>  #include "hw/i386/sgx.h"
> +#include "sysemu/hw_accel.h"
> +
> +#define SGX_MAX_EPC_SECTIONS            8
> +#define SGX_CPUID_EPC_INVALID           0x0
> +
> +/* A valid EPC section. */
> +#define SGX_CPUID_EPC_SECTION           0x1
> +#define SGX_CPUID_EPC_MASK              0xF
> +
> +static uint64_t sgx_calc_section_metric(uint64_t low, uint64_t high)
> +{
> +    return (low & MAKE_64BIT_MASK(12, 31 - 12 + 1)) +
> +           ((high & MAKE_64BIT_MASK(0, 19 - 0 + 1)) << 32);

Thanks for using MAKE_64BIT_MASK.

Can we have #definitions instead of these magic values please?

> +}
> +
> +static uint64_t sgx_calc_host_epc_section_size(void)
> +{
> +    uint32_t i, type;
> +    uint32_t eax, ebx, ecx, edx;
> +    uint64_t size = 0;
> +
> +    for (i = 0; i < SGX_MAX_EPC_SECTIONS; i++) {
> +        host_cpuid(0x12, i + 2, &eax, &ebx, &ecx, &edx);
> +
> +        type = eax & SGX_CPUID_EPC_MASK;
> +        if (type == SGX_CPUID_EPC_INVALID) {
> +            break;
> +        }
> +
> +        if (type != SGX_CPUID_EPC_SECTION) {
> +            break;
> +        }
> +
> +        size += sgx_calc_section_metric(ecx, edx);
> +    }
> +
> +    return size;
> +}
> +
> +SGXInfo *sgx_get_capabilities(Error **errp)
> +{
> +    SGXInfo *info = NULL;
> +    uint32_t eax, ebx, ecx, edx;
> +
> +    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
> +    if (fd < 0) {
> +        error_setg(errp, "SGX is not enabled in KVM");
> +        return NULL;
> +    }
> +
> +    info = g_new0(SGXInfo, 1);
> +    host_cpuid(0x7, 0, &eax, &ebx, &ecx, &edx);
> +
> +    info->sgx = ebx & (1U << 2) ? true : false;
> +    info->flc = ecx & (1U << 30) ? true : false;
> +
> +    host_cpuid(0x12, 0, &eax, &ebx, &ecx, &edx);
> +    info->sgx1 = eax & (1U << 0) ? true : false;
> +    info->sgx2 = eax & (1U << 1) ? true : false;
> +
> +    info->section_size = sgx_calc_host_epc_section_size();
> +
> +    close(fd);
> +
> +    return info;
> +}

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Daniel P. Berrangé]

On Fri, Sep 10, 2021 at 06:22:56PM +0800, Yang Zhong wrote:
> The QMP and HMP interfaces can be used by monitor or QMP tools to retrieve
> the SGX information from VM side when SGX is enabled on Intel platform.
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  hmp-commands-info.hx         | 15 +++++++++++++
>  hw/i386/sgx.c                | 29 ++++++++++++++++++++++++
>  include/hw/i386/sgx.h        | 11 +++++++++
>  include/monitor/hmp-target.h |  1 +
>  qapi/misc-target.json        | 43 ++++++++++++++++++++++++++++++++++++
>  target/i386/monitor.c        | 36 ++++++++++++++++++++++++++++++
>  tests/qtest/qmp-cmd-test.c   |  1 +
>  7 files changed, 136 insertions(+)
>  create mode 100644 include/hw/i386/sgx.h

> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index 02fa6487c3..8a32d62d7e 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -17,6 +17,35 @@
>  #include "monitor/qdev.h"
>  #include "qapi/error.h"
>  #include "exec/address-spaces.h"
> +#include "hw/i386/sgx.h"
> +
> +SGXInfo *sgx_get_info(void)

I'd suggest this should have an 'Error **errp'

> +{
> +    SGXInfo *info = NULL;
> +    X86MachineState *x86ms;
> +    PCMachineState *pcms =
> +        (PCMachineState *)object_dynamic_cast(qdev_get_machine(),
> +                                              TYPE_PC_MACHINE);
> +    if (!pcms) {

  error_setg(errp, "SGX is only available for x86 PC machines");

> +        return NULL;
> +    }
> +
> +    x86ms = X86_MACHINE(pcms);
> +    if (!x86ms->sgx_epc_list) {

  error_setg(errp "SGX is not enabled on this machine");

> +        return NULL;
> +    }
> +
> +    SGXEPCState *sgx_epc = &pcms->sgx_epc;
> +    info = g_new0(SGXInfo, 1);
> +
> +    info->sgx = true;
> +    info->sgx1 = true;
> +    info->sgx2 = true;
> +    info->flc = true;
> +    info->section_size = sgx_epc->size;
> +
> +    return info;
> +}



> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 119211f0b0..0f1b48b4f8 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -35,6 +35,7 @@
>  #include "qapi/qapi-commands-misc-target.h"
>  #include "qapi/qapi-commands-misc.h"
>  #include "hw/i386/pc.h"
> +#include "hw/i386/sgx.h"
>  
>  /* Perform linear address sign extension */
>  static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> @@ -763,3 +764,38 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
>  {
>      return sev_get_attestation_report(mnonce, errp);
>  }
> +
> +SGXInfo *qmp_query_sgx(Error **errp)
> +{
> +    SGXInfo *info;
> +
> +    info = sgx_get_info();

Pass errp into this

> +    if (!info) {
> +        error_setg(errp, "SGX features are not available");

And get rid of this.

> +        return NULL;
> +    }
> +
> +    return info;
> +}
> +
> +void hmp_info_sgx(Monitor *mon, const QDict *qdict)
> +{

  g_autoptr(Error) err = NULL

> +    SGXInfo *info = qmp_query_sgx(NULL);

Pass in &err not NULL;

Also  declare it with  'g_autoptr(SGXInfo) info = ...'

And then

   if (err) {
      monitor_printf(mon, "%s\n", error_get_pretty(err);
      return;
   }

> +
> +    if (info && info->sgx) {
> +        monitor_printf(mon, "SGX support: %s\n",
> +                       info->sgx ? "enabled" : "disabled");
> +        monitor_printf(mon, "SGX1 support: %s\n",
> +                       info->sgx1 ? "enabled" : "disabled");
> +        monitor_printf(mon, "SGX2 support: %s\n",
> +                       info->sgx2 ? "enabled" : "disabled");
> +        monitor_printf(mon, "FLC support: %s\n",
> +                       info->flc ? "enabled" : "disabled");
> +        monitor_printf(mon, "size: %" PRIu64 "\n",
> +                       info->section_size);
> +    } else {
> +        monitor_printf(mon, "SGX is not enabled\n");
> +    }

Now you can remove the if/else and just print the result

> +
> +    qapi_free_SGXInfo(info);

This can be dropped with the g_autoptr usage


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX

[Paolo Bonzini]

On 10/09/21 12:22, Yang Zhong wrote:
> This patchset supply HMP/QMP interfaces to monitor and Libvirt, with
> those interfaces, we can check the SGX info from VM side or check
> host SGX capabilities from Libvirt side.
> 
> This patchset is splitted from below link(from patch26 to patch30):
> https://patchew.org/QEMU/20210719112136.57018-1-yang.zhong@intel.com/
> 
> The rest patches are being pulled by Paolo's below link and this new
> patchset is based on it.
> https://gitlab.com/bonzini/qemu.git tags/for-upstream

Queued 1-2, thanks.

For patch 3, I would like to learn more about the whole reset part of 
the series.  Would it be possible to handle it in the kernel without 
having to reopen the vEPC device (there's a possible race where you 
can't reopen and it's a huge mess)?

Paolo

Re: [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX

[Daniel P. Berrangé]

On Fri, Sep 10, 2021 at 04:15:21PM +0200, Paolo Bonzini wrote:
> On 10/09/21 12:22, Yang Zhong wrote:
> > This patchset supply HMP/QMP interfaces to monitor and Libvirt, with
> > those interfaces, we can check the SGX info from VM side or check
> > host SGX capabilities from Libvirt side.
> > 
> > This patchset is splitted from below link(from patch26 to patch30):
> > https://patchew.org/QEMU/20210719112136.57018-1-yang.zhong@intel.com/
> > 
> > The rest patches are being pulled by Paolo's below link and this new
> > patchset is based on it.
> > https://gitlab.com/bonzini/qemu.git tags/for-upstream
> 
> Queued 1-2, thanks.

I had just posted a bunch of comments on patch 1 ...


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

Il ven 10 set 2021, 16:22 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:

> > Queued 1-2, thanks.
>
> I had just posted a bunch of comments on patch 1 ...
>

Sorry, I had already queued it a few hours earlier and just noticed I
hadn't sent out the message.

I have some updates to the main series too, so I might just pick up your
suggestions and send out everything next week or on Sunday.

Paolo


>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 1877 bytes --]

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Daniel P. Berrangé]

On Fri, Sep 10, 2021 at 02:46:00PM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 10, 2021 at 06:22:56PM +0800, Yang Zhong wrote:
> > The QMP and HMP interfaces can be used by monitor or QMP tools to retrieve
> > the SGX information from VM side when SGX is enabled on Intel platform.
> > 
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > ---
> >  hmp-commands-info.hx         | 15 +++++++++++++
> >  hw/i386/sgx.c                | 29 ++++++++++++++++++++++++
> >  include/hw/i386/sgx.h        | 11 +++++++++
> >  include/monitor/hmp-target.h |  1 +
> >  qapi/misc-target.json        | 43 ++++++++++++++++++++++++++++++++++++
> >  target/i386/monitor.c        | 36 ++++++++++++++++++++++++++++++
> >  tests/qtest/qmp-cmd-test.c   |  1 +
> >  7 files changed, 136 insertions(+)
> >  create mode 100644 include/hw/i386/sgx.h
> 
> > diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> > index 02fa6487c3..8a32d62d7e 100644
> > --- a/hw/i386/sgx.c
> > +++ b/hw/i386/sgx.c
> > @@ -17,6 +17,35 @@
> >  #include "monitor/qdev.h"
> >  #include "qapi/error.h"
> >  #include "exec/address-spaces.h"
> > +#include "hw/i386/sgx.h"
> > +
> > +SGXInfo *sgx_get_info(void)
> 
> I'd suggest this should have an 'Error **errp'
> 
> > +{
> > +    SGXInfo *info = NULL;
> > +    X86MachineState *x86ms;
> > +    PCMachineState *pcms =
> > +        (PCMachineState *)object_dynamic_cast(qdev_get_machine(),
> > +                                              TYPE_PC_MACHINE);
> > +    if (!pcms) {
> 
>   error_setg(errp, "SGX is only available for x86 PC machines");
> 
> > +        return NULL;
> > +    }
> > +
> > +    x86ms = X86_MACHINE(pcms);
> > +    if (!x86ms->sgx_epc_list) {
> 
>   error_setg(errp "SGX is not enabled on this machine");
> 
> > +        return NULL;
> > +    }
> > +
> > +    SGXEPCState *sgx_epc = &pcms->sgx_epc;
> > +    info = g_new0(SGXInfo, 1);
> > +
> > +    info->sgx = true;
> > +    info->sgx1 = true;
> > +    info->sgx2 = true;
> > +    info->flc = true;
> > +    info->section_size = sgx_epc->size;
> > +
> > +    return info;
> > +}
> 
> 
> 
> > diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> > index 119211f0b0..0f1b48b4f8 100644
> > --- a/target/i386/monitor.c
> > +++ b/target/i386/monitor.c
> > @@ -35,6 +35,7 @@
> >  #include "qapi/qapi-commands-misc-target.h"
> >  #include "qapi/qapi-commands-misc.h"
> >  #include "hw/i386/pc.h"
> > +#include "hw/i386/sgx.h"
> >  
> >  /* Perform linear address sign extension */
> >  static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> > @@ -763,3 +764,38 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
> >  {
> >      return sev_get_attestation_report(mnonce, errp);
> >  }
> > +
> > +SGXInfo *qmp_query_sgx(Error **errp)
> > +{
> > +    SGXInfo *info;
> > +
> > +    info = sgx_get_info();
> 
> Pass errp into this
> 
> > +    if (!info) {
> > +        error_setg(errp, "SGX features are not available");
> 
> And get rid of this.
> 
> > +        return NULL;
> > +    }
> > +
> > +    return info;
> > +}
> > +
> > +void hmp_info_sgx(Monitor *mon, const QDict *qdict)
> > +{
> 
>   g_autoptr(Error) err = NULL

I was mistaken here - Error shouldn't use g_autoptr, just

   Error err = NULL;

> 
> > +    SGXInfo *info = qmp_query_sgx(NULL);
> 
> Pass in &err not NULL;
> 
> Also  declare it with  'g_autoptr(SGXInfo) info = ...'
> 
> And then
> 
>    if (err) {
>       monitor_printf(mon, "%s\n", error_get_pretty(err);

Then use the simpler:

    error_report_err(err);

which prints + frees 'err'

>       return;
>    }
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities()

[Yang Zhong]

On Fri, Sep 10, 2021 at 02:41:08PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/10/21 12:22 PM, Yang Zhong wrote:
> > Libvirt can use qmp_query_sgx_capabilities() to get the host
> > sgx capabilities to decide how to allocate SGX EPC size to VM.
> > 
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > ---
> >  hw/i386/sgx.c              | 66 ++++++++++++++++++++++++++++++++++++++
> >  include/hw/i386/sgx.h      |  1 +
> >  qapi/misc-target.json      | 18 +++++++++++
> >  target/i386/monitor.c      |  5 +++
> >  tests/qtest/qmp-cmd-test.c |  1 +
> >  5 files changed, 91 insertions(+)
> > 
> > diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> > index 8a32d62d7e..1be2670c84 100644
> > --- a/hw/i386/sgx.c
> > +++ b/hw/i386/sgx.c
> > @@ -18,6 +18,72 @@
> >  #include "qapi/error.h"
> >  #include "exec/address-spaces.h"
> >  #include "hw/i386/sgx.h"
> > +#include "sysemu/hw_accel.h"
> > +
> > +#define SGX_MAX_EPC_SECTIONS            8
> > +#define SGX_CPUID_EPC_INVALID           0x0
> > +
> > +/* A valid EPC section. */
> > +#define SGX_CPUID_EPC_SECTION           0x1
> > +#define SGX_CPUID_EPC_MASK              0xF
> > +
> > +static uint64_t sgx_calc_section_metric(uint64_t low, uint64_t high)
> > +{
> > +    return (low & MAKE_64BIT_MASK(12, 31 - 12 + 1)) +
> > +           ((high & MAKE_64BIT_MASK(0, 19 - 0 + 1)) << 32);
> 
> Thanks for using MAKE_64BIT_MASK.
> 
> Can we have #definitions instead of these magic values please?
>

  Thanks, i will do the below definitions:

  /* The start bit of the EPC section */
  #define ECX_START                       12
  #define EDX_START                       0
  
  static uint64_t sgx_calc_section_metric(uint64_t low, uint64_t high)
  {
      return (low & MAKE_64BIT_MASK(ECX_START, 20)) +
             ((high & MAKE_64BIT_MASK(EDX_START, 20)) << 32);
  } 


  Yang

 
> > +}
> > +
> > +static uint64_t sgx_calc_host_epc_section_size(void)
> > +{
> > +    uint32_t i, type;
> > +    uint32_t eax, ebx, ecx, edx;
> > +    uint64_t size = 0;
> > +
> > +    for (i = 0; i < SGX_MAX_EPC_SECTIONS; i++) {
> > +        host_cpuid(0x12, i + 2, &eax, &ebx, &ecx, &edx);
> > +
> > +        type = eax & SGX_CPUID_EPC_MASK;
> > +        if (type == SGX_CPUID_EPC_INVALID) {
> > +            break;
> > +        }
> > +
> > +        if (type != SGX_CPUID_EPC_SECTION) {
> > +            break;
> > +        }
> > +
> > +        size += sgx_calc_section_metric(ecx, edx);
> > +    }
> > +
> > +    return size;
> > +}
> > +
> > +SGXInfo *sgx_get_capabilities(Error **errp)
> > +{
> > +    SGXInfo *info = NULL;
> > +    uint32_t eax, ebx, ecx, edx;
> > +
> > +    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
> > +    if (fd < 0) {
> > +        error_setg(errp, "SGX is not enabled in KVM");
> > +        return NULL;
> > +    }
> > +
> > +    info = g_new0(SGXInfo, 1);
> > +    host_cpuid(0x7, 0, &eax, &ebx, &ecx, &edx);
> > +
> > +    info->sgx = ebx & (1U << 2) ? true : false;
> > +    info->flc = ecx & (1U << 30) ? true : false;
> > +
> > +    host_cpuid(0x12, 0, &eax, &ebx, &ecx, &edx);
> > +    info->sgx1 = eax & (1U << 0) ? true : false;
> > +    info->sgx2 = eax & (1U << 1) ? true : false;
> > +
> > +    info->section_size = sgx_calc_host_epc_section_size();
> > +
> > +    close(fd);
> > +
> > +    return info;
> > +}

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Yang Zhong]

On Fri, Sep 10, 2021 at 02:39:04PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/10/21 12:22 PM, Yang Zhong wrote:
> > The QMP and HMP interfaces can be used by monitor or QMP tools to retrieve
> > the SGX information from VM side when SGX is enabled on Intel platform.
> > 
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > ---
> >  hmp-commands-info.hx         | 15 +++++++++++++
> >  hw/i386/sgx.c                | 29 ++++++++++++++++++++++++
> >  include/hw/i386/sgx.h        | 11 +++++++++
> >  include/monitor/hmp-target.h |  1 +
> >  qapi/misc-target.json        | 43 ++++++++++++++++++++++++++++++++++++
> >  target/i386/monitor.c        | 36 ++++++++++++++++++++++++++++++
> >  tests/qtest/qmp-cmd-test.c   |  1 +
> >  7 files changed, 136 insertions(+)
> >  create mode 100644 include/hw/i386/sgx.h
> 
> > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > index 3b05ad3dbf..e2a347cc23 100644
> > --- a/qapi/misc-target.json
> > +++ b/qapi/misc-target.json
> > @@ -333,3 +333,46 @@
> >  { 'command': 'query-sev-attestation-report', 'data': { 'mnonce': 'str' },
> >    'returns': 'SevAttestationReport',
> >    'if': 'TARGET_I386' }
> > +
> > +##
> > +# @SGXInfo:
> > +#
> > +# Information about intel Safe Guard eXtension (SGX) support
> > +#
> > +# @sgx: true if SGX is supported
> > +#
> > +# @sgx1: true if SGX1 is supported
> > +#
> > +# @sgx2: true if SGX2 is supported
> > +#
> > +# @flc: true if FLC is supported
> > +#
> > +# @section-size: The EPC section size for guest
> > +#
> > +# Since: 6.2
> > +##
> > +{ 'struct': 'SGXInfo',
> > +  'data': { 'sgx': 'bool',
> > +            'sgx1': 'bool',
> > +            'sgx2': 'bool',
> > +            'flc': 'bool',
> > +            'section-size': 'uint64'},
> > +   'if': 'TARGET_I386' }
> 
> Is it possible to restrict it to KVM? Maybe:
> 
>      'if': { 'all': ['TARGET_I386', 'CONFIG_KVM'] } },
> 
> ? (I'm not sure).

  Philippe, i tried this definition, which is feasible.
  This seems more accurate for sgx in the kvm of i386 platform. thanks!

  Yang
  

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Yang Zhong]

On Mon, Sep 13, 2021 at 10:35:42AM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 10, 2021 at 02:46:00PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Sep 10, 2021 at 06:22:56PM +0800, Yang Zhong wrote:
> > > The QMP and HMP interfaces can be used by monitor or QMP tools to retrieve
> > > the SGX information from VM side when SGX is enabled on Intel platform.
> > > 
> > > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > > ---
> > >  hmp-commands-info.hx         | 15 +++++++++++++
> > >  hw/i386/sgx.c                | 29 ++++++++++++++++++++++++
> > >  include/hw/i386/sgx.h        | 11 +++++++++
> > >  include/monitor/hmp-target.h |  1 +
> > >  qapi/misc-target.json        | 43 ++++++++++++++++++++++++++++++++++++
> > >  target/i386/monitor.c        | 36 ++++++++++++++++++++++++++++++
> > >  tests/qtest/qmp-cmd-test.c   |  1 +
> > >  7 files changed, 136 insertions(+)
> > >  create mode 100644 include/hw/i386/sgx.h
> > 
> > > diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> > > index 02fa6487c3..8a32d62d7e 100644
> > > --- a/hw/i386/sgx.c
> > > +++ b/hw/i386/sgx.c
> > > @@ -17,6 +17,35 @@
> > >  #include "monitor/qdev.h"
> > >  #include "qapi/error.h"
> > >  #include "exec/address-spaces.h"
> > > +#include "hw/i386/sgx.h"
> > > +
> > > +SGXInfo *sgx_get_info(void)
> > 
> > I'd suggest this should have an 'Error **errp'

    Thanks, the new version will add this variable. thanks!


> > 
> > > +{
> > > +    SGXInfo *info = NULL;
> > > +    X86MachineState *x86ms;
> > > +    PCMachineState *pcms =
> > > +        (PCMachineState *)object_dynamic_cast(qdev_get_machine(),
> > > +                                              TYPE_PC_MACHINE);
> > > +    if (!pcms) {
> > 
> >   error_setg(errp, "SGX is only available for x86 PC machines");
> > 

      Yes, i will add this, thanks!


> > > +        return NULL;
> > > +    }
> > > +
> > > +    x86ms = X86_MACHINE(pcms);
> > > +    if (!x86ms->sgx_epc_list) {
> > 
> >   error_setg(errp "SGX is not enabled on this machine");
> > 
 
      Ditto, thanks!


> > > +        return NULL;
> > > +    }
> > > +
> > > +    SGXEPCState *sgx_epc = &pcms->sgx_epc;
> > > +    info = g_new0(SGXInfo, 1);
> > > +
> > > +    info->sgx = true;
> > > +    info->sgx1 = true;
> > > +    info->sgx2 = true;
> > > +    info->flc = true;
> > > +    info->section_size = sgx_epc->size;
> > > +
> > > +    return info;
> > > +}
> > 
> > 
> > 
> > > diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> > > index 119211f0b0..0f1b48b4f8 100644
> > > --- a/target/i386/monitor.c
> > > +++ b/target/i386/monitor.c
> > > @@ -35,6 +35,7 @@
> > >  #include "qapi/qapi-commands-misc-target.h"
> > >  #include "qapi/qapi-commands-misc.h"
> > >  #include "hw/i386/pc.h"
> > > +#include "hw/i386/sgx.h"
> > >  
> > >  /* Perform linear address sign extension */
> > >  static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> > > @@ -763,3 +764,38 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
> > >  {
> > >      return sev_get_attestation_report(mnonce, errp);
> > >  }
> > > +
> > > +SGXInfo *qmp_query_sgx(Error **errp)
> > > +{
> > > +    SGXInfo *info;
> > > +
> > > +    info = sgx_get_info();
> > 
> > Pass errp into this

    Thanks, i will add this.


> > 
> > > +    if (!info) {
> > > +        error_setg(errp, "SGX features are not available");
> > 
> > And get rid of this.

    Yes, i will remove this, thanks!


> > 
> > > +        return NULL;
> > > +    }
> > > +
> > > +    return info;
> > > +}
> > > +
> > > +void hmp_info_sgx(Monitor *mon, const QDict *qdict)
> > > +{
> > 
> >   g_autoptr(Error) err = NULL
> 
> I was mistaken here - Error shouldn't use g_autoptr, just
> 
>    Error err = NULL;


    Yes, i will use this new defintion to handle it. thanks!


> 
> > 
> > > +    SGXInfo *info = qmp_query_sgx(NULL);
> > 
> > Pass in &err not NULL;
> > 
> > Also  declare it with  'g_autoptr(SGXInfo) info = ...'
> > 
> > And then
> > 
> >    if (err) {
> >       monitor_printf(mon, "%s\n", error_get_pretty(err);
> 
> Then use the simpler:
> 
>     error_report_err(err);
> 
> which prints + frees 'err'
> 

  Thanks, the new code like below:
  
  Error *err = NULL;

  SGXInfo *info = qmp_query_sgx(&err);
  if (err) {
      error_report_err(err);
      return;
  }

  ......

  I will send V3, please help review again, thanks!


  Yang



> >       return;
> >    }
> > 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Paolo Bonzini]

On 13/09/21 12:37, Yang Zhong wrote:
>>> +{ 'struct': 'SGXInfo',
>>> +  'data': { 'sgx': 'bool',
>>> +            'sgx1': 'bool',
>>> +            'sgx2': 'bool',
>>> +            'flc': 'bool',
>>> +            'section-size': 'uint64'},
>>> +   'if': 'TARGET_I386' }
>>
>> Is it possible to restrict it to KVM? Maybe:
>>
>>       'if': { 'all': ['TARGET_I386', 'CONFIG_KVM'] } },
>>
>> ? (I'm not sure).
> 
>    Philippe, i tried this definition, which is feasible.
>    This seems more accurate for sgx in the kvm of i386 platform. thanks!

The definition is needed in the stubs as well (cross-compilation 
currently fails due to missing sgx_get_{info,capabilities}), so I think 
this doesn't work.

Paolo

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Paolo Bonzini]

On 13/09/21 11:35, Daniel P. Berrangé wrote:
>>    g_autoptr(Error) err = NULL
> I was mistaken here - Error shouldn't use g_autoptr, just
> 
>     Error err = NULL;
> 
>>> +    SGXInfo *info = qmp_query_sgx(NULL);
>> Pass in &err not NULL;
>>
>> Also  declare it with  'g_autoptr(SGXInfo) info = ...'
>>
>> And then
>>
>>     if (err) {
>>        monitor_printf(mon, "%s\n", error_get_pretty(err);
> Then use the simpler:
> 
>      error_report_err(err);

Indeed.

That said, more long term (but this is something Coccinelle could help 
with) perhaps error_report_err should not free the error, and instead we 
should use g_autoptr(Error) in the callers.  I don't like functions that 
do not have free in their name and yet free a pointer...

Paolo

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Yang Zhong]

On Mon, Sep 13, 2021 at 01:56:13PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 13, 2021 at 02:48:37PM +0200, Paolo Bonzini wrote:
> > On 13/09/21 11:35, Daniel P. Berrangé wrote:
> > > >    g_autoptr(Error) err = NULL
> > > I was mistaken here - Error shouldn't use g_autoptr, just
> > > 
> > >     Error err = NULL;
> > > 
> > > > > +    SGXInfo *info = qmp_query_sgx(NULL);
> > > > Pass in &err not NULL;
> > > > 
> > > > Also  declare it with  'g_autoptr(SGXInfo) info = ...'
> > > > 
> > > > And then
> > > > 
> > > >     if (err) {
> > > >        monitor_printf(mon, "%s\n", error_get_pretty(err);
> > > Then use the simpler:
> > > 
> > >      error_report_err(err);
> > 
> > Indeed.
> > 
> > That said, more long term (but this is something Coccinelle could help with)
> > perhaps error_report_err should not free the error, and instead we should
> > use g_autoptr(Error) in the callers.  I don't like functions that do not
> > have free in their name and yet free a pointer...
> 
> Yes, this error_report_err surprises me every 6 months when I
> come to deal with it. So I think using g_autoptr would be a
> nice replacement, with no additional burden in terms of lines
> of code in callers too.
>

  Do we need call qapi_free_SGXInfo(info) here?

  In previous code design, the code like below:

  SGXInfo *info = qmp_query_sgx(&err);
  ......
  qapi_free_SGXInfo(info);


  Yang

 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Daniel P. Berrangé]

On Mon, Sep 13, 2021 at 02:48:37PM +0200, Paolo Bonzini wrote:
> On 13/09/21 11:35, Daniel P. Berrangé wrote:
> > >    g_autoptr(Error) err = NULL
> > I was mistaken here - Error shouldn't use g_autoptr, just
> > 
> >     Error err = NULL;
> > 
> > > > +    SGXInfo *info = qmp_query_sgx(NULL);
> > > Pass in &err not NULL;
> > > 
> > > Also  declare it with  'g_autoptr(SGXInfo) info = ...'
> > > 
> > > And then
> > > 
> > >     if (err) {
> > >        monitor_printf(mon, "%s\n", error_get_pretty(err);
> > Then use the simpler:
> > 
> >      error_report_err(err);
> 
> Indeed.
> 
> That said, more long term (but this is something Coccinelle could help with)
> perhaps error_report_err should not free the error, and instead we should
> use g_autoptr(Error) in the callers.  I don't like functions that do not
> have free in their name and yet free a pointer...

Yes, this error_report_err surprises me every 6 months when I
come to deal with it. So I think using g_autoptr would be a
nice replacement, with no additional burden in terms of lines
of code in callers too.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Daniel P. Berrangé]

On Mon, Sep 13, 2021 at 08:52:28PM +0800, Yang Zhong wrote:
> On Mon, Sep 13, 2021 at 01:56:13PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 13, 2021 at 02:48:37PM +0200, Paolo Bonzini wrote:
> > > On 13/09/21 11:35, Daniel P. Berrangé wrote:
> > > > >    g_autoptr(Error) err = NULL
> > > > I was mistaken here - Error shouldn't use g_autoptr, just
> > > > 
> > > >     Error err = NULL;
> > > > 
> > > > > > +    SGXInfo *info = qmp_query_sgx(NULL);
> > > > > Pass in &err not NULL;
> > > > > 
> > > > > Also  declare it with  'g_autoptr(SGXInfo) info = ...'
> > > > > 
> > > > > And then
> > > > > 
> > > > >     if (err) {
> > > > >        monitor_printf(mon, "%s\n", error_get_pretty(err);
> > > > Then use the simpler:
> > > > 
> > > >      error_report_err(err);
> > > 
> > > Indeed.
> > > 
> > > That said, more long term (but this is something Coccinelle could help with)
> > > perhaps error_report_err should not free the error, and instead we should
> > > use g_autoptr(Error) in the callers.  I don't like functions that do not
> > > have free in their name and yet free a pointer...
> > 
> > Yes, this error_report_err surprises me every 6 months when I
> > come to deal with it. So I think using g_autoptr would be a
> > nice replacement, with no additional burden in terms of lines
> > of code in callers too.
> >
> 
>   Do we need call qapi_free_SGXInfo(info) here?
> 
>   In previous code design, the code like below:
> 
>   SGXInfo *info = qmp_query_sgx(&err);
>   ......
>   qapi_free_SGXInfo(info);

I suggested "g_autoptr(SGXInfo) info" for the declaration to avoid
the need for qapi_free_SGXInfo calls


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Yang Zhong]

On Mon, Sep 13, 2021 at 02:24:56PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 13, 2021 at 08:52:28PM +0800, Yang Zhong wrote:
> > On Mon, Sep 13, 2021 at 01:56:13PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Sep 13, 2021 at 02:48:37PM +0200, Paolo Bonzini wrote:
> > > > On 13/09/21 11:35, Daniel P. Berrangé wrote:
> > > > > >    g_autoptr(Error) err = NULL
> > > > > I was mistaken here - Error shouldn't use g_autoptr, just
> > > > > 
> > > > >     Error err = NULL;
> > > > > 
> > > > > > > +    SGXInfo *info = qmp_query_sgx(NULL);
> > > > > > Pass in &err not NULL;
> > > > > > 
> > > > > > Also  declare it with  'g_autoptr(SGXInfo) info = ...'
> > > > > > 
> > > > > > And then
> > > > > > 
> > > > > >     if (err) {
> > > > > >        monitor_printf(mon, "%s\n", error_get_pretty(err);
> > > > > Then use the simpler:
> > > > > 
> > > > >      error_report_err(err);
> > > > 
> > > > Indeed.
> > > > 
> > > > That said, more long term (but this is something Coccinelle could help with)
> > > > perhaps error_report_err should not free the error, and instead we should
> > > > use g_autoptr(Error) in the callers.  I don't like functions that do not
> > > > have free in their name and yet free a pointer...
> > > 
> > > Yes, this error_report_err surprises me every 6 months when I
> > > come to deal with it. So I think using g_autoptr would be a
> > > nice replacement, with no additional burden in terms of lines
> > > of code in callers too.
> > >
> > 
> >   Do we need call qapi_free_SGXInfo(info) here?
> > 
> >   In previous code design, the code like below:
> > 
> >   SGXInfo *info = qmp_query_sgx(&err);
> >   ......
> >   qapi_free_SGXInfo(info);
> 
> I suggested "g_autoptr(SGXInfo) info" for the declaration to avoid
> the need for qapi_free_SGXInfo calls
>

  Daniel, thanks!

  Paolo, i checked the sgx branch of your gitlab, we need add this definition
  "g_autoptr(SGXInfo) info" into hmp_info_sgx() function. thanks!

  Yang
   
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|