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

[PATCH 0/7] 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 reset are being pulled by Paolo's gitlab:
https://gitlab.com/bonzini/qemu.git tags/for-upstream

This new patchset based on <20210907071349.116329-1-pbonzini@redhat.com>, and
you can check the patches from below link:
https://github.com/intel/qemu-sgx.git monitor

As for the qtest/test-hmp test issue in last PULL, this release has fixed this issue.

Changelog:
=========
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 (7):
  qmp: Add query-sgx command
  hmp: Add 'info sgx' command
  i386: Add sgx_get_info() interface
  bitops: Support 32 and 64 bit mask macro
  qmp: Add the qmp_query_sgx_capabilities()
  monitor: Fix coredump issue in non-x86 platform
  pc: Cleanup the SGX definitions

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

[PATCH 1/7] qmp: Add query-sgx command

[Yang Zhong]

This QMP query command can be used by some userspaces to retrieve
the SGX information when SGX is enabled on Intel platform.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 qapi/misc-target.json      | 43 ++++++++++++++++++++++++++++++++++++++
 target/i386/monitor.c      |  6 ++++++
 tests/qtest/qmp-cmd-test.c |  1 +
 3 files changed, 50 insertions(+)

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..942274c1c2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -763,3 +763,9 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
 {
     return sev_get_attestation_report(mnonce, errp);
 }
+
+SGXInfo *qmp_query_sgx(Error **errp)
+{
+    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
+    return NULL;
+}
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 2/7] hmp: Add 'info sgx' command

[Yang Zhong]

The command can be used to show the SGX information in the monitor
when SGX is enabled on intel platform.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 hmp-commands-info.hx         | 15 +++++++++++++++
 include/monitor/hmp-target.h |  1 +
 target/i386/monitor.c        |  6 ++++++
 3 files changed, 22 insertions(+)

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/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/target/i386/monitor.c b/target/i386/monitor.c
index 942274c1c2..f1861fe6c2 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -769,3 +769,9 @@ SGXInfo *qmp_query_sgx(Error **errp)
     error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
     return NULL;
 }
+
+void hmp_info_sgx(Monitor *mon, const QDict *qdict)
+{
+    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
+    return NULL;
+}

[PATCH 3/7] i386: Add sgx_get_info() interface

[Yang Zhong]

Add the sgx_get_info() interface for hmp and QMP usage, which
will get the SGX info from this API.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 hw/i386/sgx.c         | 21 +++++++++++++++++++++
 include/hw/i386/sgx.h | 11 +++++++++++
 target/i386/monitor.c | 32 ++++++++++++++++++++++++++++----
 3 files changed, 60 insertions(+), 4 deletions(-)
 create mode 100644 include/hw/i386/sgx.h

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 5f988c6368..a3cd671a70 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -17,6 +17,27 @@
 #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;
+    MachineState *ms = MACHINE(qdev_get_machine());
+    X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
+
+    if (x86ms->sgx_epc_list) {
+        PCMachineState *pcms = PC_MACHINE(ms);
+        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/target/i386/monitor.c b/target/i386/monitor.c
index f1861fe6c2..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)
@@ -766,12 +767,35 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
 
 SGXInfo *qmp_query_sgx(Error **errp)
 {
-    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
-    return NULL;
+    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)
 {
-    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
-    return NULL;
+    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);
 }

Re: [PATCH 3/7] i386: Add sgx_get_info() interface

[Philippe Mathieu-Daudé]

On 9/8/21 10:19 AM, Yang Zhong wrote:
> Add the sgx_get_info() interface for hmp and QMP usage, which
> will get the SGX info from this API.
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  hw/i386/sgx.c         | 21 +++++++++++++++++++++
>  include/hw/i386/sgx.h | 11 +++++++++++
>  target/i386/monitor.c | 32 ++++++++++++++++++++++++++++----
>  3 files changed, 60 insertions(+), 4 deletions(-)
>  create mode 100644 include/hw/i386/sgx.h

> @@ -766,12 +767,35 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
>  
>  SGXInfo *qmp_query_sgx(Error **errp)
>  {
> -    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");

>  void hmp_info_sgx(Monitor *mon, const QDict *qdict)
>  {
> -    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
> -    return NULL;

What is the point of patches #1 & #2? Why not squash all here?

Re: [PATCH 3/7] i386: Add sgx_get_info() interface

[Yang Zhong]

On Wed, Sep 08, 2021 at 10:32:27AM +0200, Philippe Mathieu-Daudé wrote:
> On 9/8/21 10:19 AM, Yang Zhong wrote:
> > Add the sgx_get_info() interface for hmp and QMP usage, which
> > will get the SGX info from this API.
> > 
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > ---
> >  hw/i386/sgx.c         | 21 +++++++++++++++++++++
> >  include/hw/i386/sgx.h | 11 +++++++++++
> >  target/i386/monitor.c | 32 ++++++++++++++++++++++++++++----
> >  3 files changed, 60 insertions(+), 4 deletions(-)
> >  create mode 100644 include/hw/i386/sgx.h
> 
> > @@ -766,12 +767,35 @@ qmp_query_sev_attestation_report(const char *mnonce, Error **errp)
> >  
> >  SGXInfo *qmp_query_sgx(Error **errp)
> >  {
> > -    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
> 
> >  void hmp_info_sgx(Monitor *mon, const QDict *qdict)
> >  {
> > -    error_setg(errp, QERR_FEATURE_DISABLED, "query-sgx");
> > -    return NULL;
> 
> What is the point of patches #1 & #2? Why not squash all here?

  Philippe, The different user usage, Monitor and QMP tool to get the some info from VM.
  I am okay to squash those 3 patches into ones, thanks!

  Yang 

Re: [PATCH 3/7] i386: Add sgx_get_info() interface

[Paolo Bonzini]

On 08/09/21 10:19, Yang Zhong wrote:
> +    if (x86ms->sgx_epc_list) {
> +        PCMachineState *pcms = PC_MACHINE(ms);
> +        SGXEPCState *sgx_epc = &pcms->sgx_epc;
> +        info = g_new0(SGXInfo, 1);
> +
> +        info->sgx = true;
> +        info->sgx1 = true;
> +        info->sgx2 = true;
> +        info->flc = true;

Since this is querying the actual machine, it should check the CPUID 
bits of the first CPU, instead of just returning true.

Paolo

Re: [PATCH 3/7] i386: Add sgx_get_info() interface

[Yang Zhong]

On Wed, Sep 08, 2021 at 10:55:13AM +0200, Paolo Bonzini wrote:
> On 08/09/21 10:19, Yang Zhong wrote:
> >+    if (x86ms->sgx_epc_list) {
> >+        PCMachineState *pcms = PC_MACHINE(ms);
> >+        SGXEPCState *sgx_epc = &pcms->sgx_epc;
> >+        info = g_new0(SGXInfo, 1);
> >+
> >+        info->sgx = true;
> >+        info->sgx1 = true;
> >+        info->sgx2 = true;
> >+        info->flc = true;
> 
> Since this is querying the actual machine, it should check the CPUID
> bits of the first CPU, instead of just returning true.
> 

  Paolo, this interface is only for checking SGX info from VM side by
  motinor or QMP tools, the SGXInfo *sgx_get_capabilities(Error **errp)
  in the patch5 check the host cpuid info to get the SGX related CPU bit
  info, like sgx,flc,sgx1,and sgx2 bit info. so here, if x86ms->sgx_epc_list
  is setting, those bits info in the VM side are all ture. thanks!

  Yang
 
> Paolo

[PATCH 4/7] bitops: Support 32 and 64 bit mask macro

[Yang Zhong]

The Qemu should enable bit mask macro like Linux did in the
kernel, the GENMASK(h, l) and GENMASK_ULL(h, l) will set the
bit to 1 from l to h bit in the 32 bit or 64 bit long type.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 include/qemu/bitops.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 03213ce952..04dec60670 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -18,6 +18,7 @@
 
 #define BITS_PER_BYTE           CHAR_BIT
 #define BITS_PER_LONG           (sizeof (unsigned long) * BITS_PER_BYTE)
+#define BITS_PER_LONG_LONG       64
 
 #define BIT(nr)                 (1UL << (nr))
 #define BIT_ULL(nr)             (1ULL << (nr))
@@ -28,6 +29,12 @@
 #define MAKE_64BIT_MASK(shift, length) \
     (((~0ULL) >> (64 - (length))) << (shift))
 
+#define GENMASK(h, l) \
+    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+
+#define GENMASK_ULL(h, l) \
+    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
+
 /**
  * set_bit - Set a bit in memory
  * @nr: the bit to set

Re: [PATCH 4/7] bitops: Support 32 and 64 bit mask macro

[Philippe Mathieu-Daudé]

On 9/8/21 10:19 AM, Yang Zhong wrote:
> The Qemu should enable bit mask macro like Linux did in the
> kernel, the GENMASK(h, l) and GENMASK_ULL(h, l) will set the
> bit to 1 from l to h bit in the 32 bit or 64 bit long type.
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  include/qemu/bitops.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index 03213ce952..04dec60670 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -18,6 +18,7 @@
>  
>  #define BITS_PER_BYTE           CHAR_BIT
>  #define BITS_PER_LONG           (sizeof (unsigned long) * BITS_PER_BYTE)
> +#define BITS_PER_LONG_LONG       64
>  
>  #define BIT(nr)                 (1UL << (nr))
>  #define BIT_ULL(nr)             (1ULL << (nr))
> @@ -28,6 +29,12 @@
>  #define MAKE_64BIT_MASK(shift, length) \
>      (((~0ULL) >> (64 - (length))) << (shift))
>  
> +#define GENMASK(h, l) \
> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> +
> +#define GENMASK_ULL(h, l) \
> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))

What is the difference with MAKE_64BIT_MASK()??

Re: [PATCH 4/7] bitops: Support 32 and 64 bit mask macro

[Yang Zhong]

On Wed, Sep 08, 2021 at 10:34:39AM +0200, Philippe Mathieu-Daudé wrote:
> On 9/8/21 10:19 AM, Yang Zhong wrote:
> > The Qemu should enable bit mask macro like Linux did in the
> > kernel, the GENMASK(h, l) and GENMASK_ULL(h, l) will set the
> > bit to 1 from l to h bit in the 32 bit or 64 bit long type.
> > 
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > ---
> >  include/qemu/bitops.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> > index 03213ce952..04dec60670 100644
> > --- a/include/qemu/bitops.h
> > +++ b/include/qemu/bitops.h
> > @@ -18,6 +18,7 @@
> >  
> >  #define BITS_PER_BYTE           CHAR_BIT
> >  #define BITS_PER_LONG           (sizeof (unsigned long) * BITS_PER_BYTE)
> > +#define BITS_PER_LONG_LONG       64
> >  
> >  #define BIT(nr)                 (1UL << (nr))
> >  #define BIT_ULL(nr)             (1ULL << (nr))
> > @@ -28,6 +29,12 @@
> >  #define MAKE_64BIT_MASK(shift, length) \
> >      (((~0ULL) >> (64 - (length))) << (shift))
> >  
> > +#define GENMASK(h, l) \
> > +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> > +
> > +#define GENMASK_ULL(h, l) \
> > +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
> 
> What is the difference with MAKE_64BIT_MASK()??

  Philippe, thanks for comments, i will use MAKE_64BIT_MASK() to replace
  GENMASK_ULL(), and at the same time, this patch will be dropped, thanks!

  Yang

[PATCH 5/7] qmp: Add the qmp_query_sgx_capabilities()

[Yang Zhong]

Libvirt can use qmp_query_sgx_capabilities() to get the host
sgx capabilities.

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 a3cd671a70..854532fb98 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              GENMASK(3, 0)
+
+static uint64_t sgx_calc_section_metric(uint64_t low, uint64_t high)
+{
+    return (low & GENMASK_ULL(31, 12)) +
+           ((high & GENMASK_ULL(19, 0)) << 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;

Re: [PATCH 5/7] qmp: Add the qmp_query_sgx_capabilities()

[Philippe Mathieu-Daudé]

On 9/8/21 10:19 AM, Yang Zhong wrote:
> Libvirt can use qmp_query_sgx_capabilities() to get the host
> sgx capabilities.
> 
> 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(+)

> +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;
> +    }

Is this Linux specific?

Re: [PATCH 5/7] qmp: Add the qmp_query_sgx_capabilities()

[Yang Zhong]

On Wed, Sep 08, 2021 at 10:38:59AM +0200, Philippe Mathieu-Daudé wrote:
> On 9/8/21 10:19 AM, Yang Zhong wrote:
> > Libvirt can use qmp_query_sgx_capabilities() to get the host
> > sgx capabilities.
> > 
> > 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(+)
> 
> > +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;
> > +    }
> 
> Is this Linux specific?

  Philippe, The /dev/sgx_vepc node is used for KVM side to expose the SGX
  EPC section to guest. Libvirt then use the '-machine none' qemu command 
  to query host SGX capabilities(especially for host SGX EPC section size)
  to decide how many SGX VMs will be started in server. If this node doesn't
  exist, the reason is host can't support SGX or SGX KVM module is not compiled
  in the kernel. thanks!

  Yang

Re: [PATCH 5/7] qmp: Add the qmp_query_sgx_capabilities()

[Philippe Mathieu-Daudé]

On 9/9/21 4:51 AM, Yang Zhong wrote:
> On Wed, Sep 08, 2021 at 10:38:59AM +0200, Philippe Mathieu-Daudé wrote:
>> On 9/8/21 10:19 AM, Yang Zhong wrote:
>>> Libvirt can use qmp_query_sgx_capabilities() to get the host
>>> sgx capabilities.
>>>
>>> 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(+)
>>
>>> +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;
>>> +    }
>>
>> Is this Linux specific?
> 
>   Philippe, The /dev/sgx_vepc node is used for KVM side to expose the SGX
>   EPC section to guest. Libvirt then use the '-machine none' qemu command 
>   to query host SGX capabilities(especially for host SGX EPC section size)
>   to decide how many SGX VMs will be started in server. If this node doesn't
>   exist, the reason is host can't support SGX or SGX KVM module is not compiled
>   in the kernel. thanks!

Sorry but you didn't answer my question in an obvious way... Yes or no?

Re: [PATCH 5/7] qmp: Add the qmp_query_sgx_capabilities()

[Yang Zhong]

On Thu, Sep 09, 2021 at 11:36:58AM +0200, Philippe Mathieu-Daudé wrote:
> On 9/9/21 4:51 AM, Yang Zhong wrote:
> > On Wed, Sep 08, 2021 at 10:38:59AM +0200, Philippe Mathieu-Daudé wrote:
> >> On 9/8/21 10:19 AM, Yang Zhong wrote:
> >>> Libvirt can use qmp_query_sgx_capabilities() to get the host
> >>> sgx capabilities.
> >>>
> >>> 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(+)
> >>
> >>> +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;
> >>> +    }
> >>
> >> Is this Linux specific?
> > 
> >   Philippe, The /dev/sgx_vepc node is used for KVM side to expose the SGX
> >   EPC section to guest. Libvirt then use the '-machine none' qemu command 
> >   to query host SGX capabilities(especially for host SGX EPC section size)
> >   to decide how many SGX VMs will be started in server. If this node doesn't
> >   exist, the reason is host can't support SGX or SGX KVM module is not compiled
> >   in the kernel. thanks!
> 
> Sorry but you didn't answer my question in an obvious way... Yes or no?

  Yes, this is Linux specific. thanks!
  
  Yang

Re: [PATCH 5/7] qmp: Add the qmp_query_sgx_capabilities()

[Philippe Mathieu-Daudé]

On 9/9/21 2:06 PM, Yang Zhong wrote:
> On Thu, Sep 09, 2021 at 11:36:58AM +0200, Philippe Mathieu-Daudé wrote:
>> On 9/9/21 4:51 AM, Yang Zhong wrote:
>>> On Wed, Sep 08, 2021 at 10:38:59AM +0200, Philippe Mathieu-Daudé wrote:
>>>> On 9/8/21 10:19 AM, Yang Zhong wrote:
>>>>> Libvirt can use qmp_query_sgx_capabilities() to get the host
>>>>> sgx capabilities.
>>>>>
>>>>> 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(+)
>>>>
>>>>> +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;
>>>>> +    }
>>>>
>>>> Is this Linux specific?
>>>
>>>   Philippe, The /dev/sgx_vepc node is used for KVM side to expose the SGX
>>>   EPC section to guest. Libvirt then use the '-machine none' qemu command 
>>>   to query host SGX capabilities(especially for host SGX EPC section size)
>>>   to decide how many SGX VMs will be started in server. If this node doesn't
>>>   exist, the reason is host can't support SGX or SGX KVM module is not compiled
>>>   in the kernel. thanks!
>>
>> Sorry but you didn't answer my question in an obvious way... Yes or no?
> 
>   Yes, this is Linux specific. thanks!

You made me have a deeper look at the code. So SGX (Kconfig) requires
KVM, which is only supported on Linux host. Thus this Linux specific
code is only built on Linux host. Thanks.

[PATCH 6/7] monitor: Fix coredump issue in non-x86 platform

[Yang Zhong]

The monitor will coredump if we use 'info sgx' in the '-machine none' or
other non-x86 platform, add the sgx_enabled bool variable to avoid this
coredump issue.

./qemu-system-x86_64 -S -no-user-config -nodefaults -nographic -machine none,accel=kvm -monitor stdio
QEMU 6.1.50 monitor - type 'help' for more information
(qemu) info sgx
/home/vmm/project/upstream/9-8/qemu/include/hw/i386/x86.h:93:X86_MACHINE: Object 0x5615d71a6fd0 is not an instance of type x86-machine
Aborted (core dumped)

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 hw/i386/sgx.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 854532fb98..118126fc70 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -27,6 +27,8 @@
 #define SGX_CPUID_EPC_SECTION           0x1
 #define SGX_CPUID_EPC_MASK              GENMASK(3, 0)
 
+bool sgx_enabled;
+
 static uint64_t sgx_calc_section_metric(uint64_t low, uint64_t high)
 {
     return (low & GENMASK_ULL(31, 12)) +
@@ -88,19 +90,22 @@ SGXInfo *sgx_get_capabilities(Error **errp)
 SGXInfo *sgx_get_info(void)
 {
     SGXInfo *info = NULL;
-    MachineState *ms = MACHINE(qdev_get_machine());
-    X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
-
-    if (x86ms->sgx_epc_list) {
-        PCMachineState *pcms = PC_MACHINE(ms);
-        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;
+
+    if (sgx_enabled) {
+        MachineState *ms = MACHINE(qdev_get_machine());
+        X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
+
+        if (x86ms->sgx_epc_list) {
+            PCMachineState *pcms = PC_MACHINE(ms);
+            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;
 }
@@ -168,4 +173,5 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
     }
 
     memory_region_set_size(&sgx_epc->mr, sgx_epc->size);
+    sgx_enabled = true;
 }

Re: [PATCH 6/7] monitor: Fix coredump issue in non-x86 platform

[Philippe Mathieu-Daudé]

On 9/8/21 10:19 AM, Yang Zhong wrote:
> The monitor will coredump if we use 'info sgx' in the '-machine none' or
> other non-x86 platform, add the sgx_enabled bool variable to avoid this
> coredump issue.

But this code is only built for x86...

> 
> ./qemu-system-x86_64 -S -no-user-config -nodefaults -nographic -machine none,accel=kvm -monitor stdio
> QEMU 6.1.50 monitor - type 'help' for more information
> (qemu) info sgx
> /home/vmm/project/upstream/9-8/qemu/include/hw/i386/x86.h:93:X86_MACHINE: Object 0x5615d71a6fd0 is not an instance of type x86-machine
> Aborted (core dumped)

Paolo suggested on IRC to use object_class_dynamic_cast().

> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  hw/i386/sgx.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)

Re: [PATCH 6/7] monitor: Fix coredump issue in non-x86 platform

[Paolo Bonzini]

On 08/09/21 10:19, Yang Zhong wrote:
> +    if (sgx_enabled) {
> +        MachineState *ms = MACHINE(qdev_get_machine());
> +        X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> +

You can use object_dynamic_cast(ms, TYPE_PC_MACHINE), something like:


PCMachineState *pcms =
     object_dynamic_cast(qdev_get_machine(), TYPE_PC_MACHINE);
X86MachineState *x86ms;

if (!pcms) {
     return NULL;
}
x86ms = X86_MACHINE(pcms);
if (!x86ms->sgx_epc_list) {
     return NULL;
}
info = g_new0(SGXInfo, 1);
...
return info;

Also please squash this patch directly into patch 3 that introduces 
sgx_get_info.

Paolo

Re: [PATCH 6/7] monitor: Fix coredump issue in non-x86 platform

[Yang Zhong]

On Wed, Sep 08, 2021 at 10:54:06AM +0200, Paolo Bonzini wrote:
> On 08/09/21 10:19, Yang Zhong wrote:
> >+    if (sgx_enabled) {
> >+        MachineState *ms = MACHINE(qdev_get_machine());
> >+        X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> >+
> 
> You can use object_dynamic_cast(ms, TYPE_PC_MACHINE), something like:
> 
> 
> PCMachineState *pcms =
>     object_dynamic_cast(qdev_get_machine(), TYPE_PC_MACHINE);
> X86MachineState *x86ms;
> 
> if (!pcms) {
>     return NULL;
> }
> x86ms = X86_MACHINE(pcms);
> if (!x86ms->sgx_epc_list) {
>     return NULL;
> }
> info = g_new0(SGXInfo, 1);
> ...
> return info;
> 
> Also please squash this patch directly into patch 3 that introduces
> sgx_get_info.
>

  Paolo, Good suggestion! This solution can avoid coredump. thanks!

  Yang 

   
> Paolo

[PATCH 7/7] 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;