[PATCH 0/4] hw/i386/sgx: Housekeeping around SGX

[PATCH 0/4] hw/i386/sgx: Housekeeping around SGX

[Philippe Mathieu-Daudé]

- Update MAINTAINERS
- Have sgx_epc_get_section() return a boolean
- Move QMP commands from target/i386 to hw/i386 (suggested by Paolo)

Based-on: <20211007161716.453984-1-philmd@redhat.com>

Philippe Mathieu-Daudé (4):
  MAINTAINERS: Cover SGX documentation file with X86/KVM section
  hw/i386/sgx: Have sgx_epc_get_section() return a boolean
  hw/i386/sgx: Move qmp_query_sgx_capabilities() to hw/i386/sgx.c
  hw/i386/sgx: Move qmp_query_sgx() and hmp_info_sgx() to hw/i386/sgx.c

 include/hw/i386/sgx-epc.h |  2 +-
 include/hw/i386/sgx.h     | 12 ------------
 hw/i386/sgx-stub.c        | 16 ++++++++++++----
 hw/i386/sgx.c             | 35 +++++++++++++++++++++++++++++------
 target/i386/monitor.c     | 32 --------------------------------
 MAINTAINERS               |  1 +
 6 files changed, 43 insertions(+), 55 deletions(-)
 delete mode 100644 include/hw/i386/sgx.h

-- 
2.31.1

[PATCH 1/4] MAINTAINERS: Cover SGX documentation file with X86/KVM section

[Philippe Mathieu-Daudé]

Cc: Yang Zhong <yang.zhong@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a49555d94d5..cb8cb48a51b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -418,6 +418,7 @@ M: Marcelo Tosatti <mtosatti@redhat.com>
 L: kvm@vger.kernel.org
 S: Supported
 F: docs/amd-memory-encryption.txt
+F: docs/system/i386/sgx.rst
 F: target/i386/kvm/
 F: target/i386/sev*
 F: scripts/kvm/vmxcap
-- 
2.31.1

[PATCH 2/4] hw/i386/sgx: Have sgx_epc_get_section() return a boolean

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/i386/sgx-epc.h | 2 +-
 hw/i386/sgx-stub.c        | 2 +-
 hw/i386/sgx.c             | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/sgx-epc.h b/include/hw/i386/sgx-epc.h
index 65a68ca753a..a6a65be854f 100644
--- a/include/hw/i386/sgx-epc.h
+++ b/include/hw/i386/sgx-epc.h
@@ -55,7 +55,7 @@ typedef struct SGXEPCState {
     int nr_sections;
 } SGXEPCState;
 
-int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size);
+bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size);
 
 static inline uint64_t sgx_epc_above_4g_end(SGXEPCState *sgx_epc)
 {
diff --git a/hw/i386/sgx-stub.c b/hw/i386/sgx-stub.c
index 3be9f5ca32c..45c473119ef 100644
--- a/hw/i386/sgx-stub.c
+++ b/hw/i386/sgx-stub.c
@@ -20,7 +20,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
     memset(&pcms->sgx_epc, 0, sizeof(SGXEPCState));
 }
 
-int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
+bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
     g_assert_not_reached();
 }
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index e481e9358f1..29724ff8f08 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -115,13 +115,13 @@ SGXInfo *sgx_get_info(Error **errp)
     return info;
 }
 
-int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
+bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     SGXEPCDevice *epc;
 
     if (pcms->sgx_epc.size == 0 || pcms->sgx_epc.nr_sections <= section_nr) {
-        return 1;
+        return true;
     }
 
     epc = pcms->sgx_epc.sections[section_nr];
@@ -129,7 +129,7 @@ int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
     *addr = epc->addr;
     *size = memory_device_get_region_size(MEMORY_DEVICE(epc), &error_fatal);
 
-    return 0;
+    return false;
 }
 
 void pc_machine_init_sgx_epc(PCMachineState *pcms)
-- 
2.31.1

[PATCH 3/4] hw/i386/sgx: Move qmp_query_sgx_capabilities() to hw/i386/sgx.c

[Philippe Mathieu-Daudé]

Move qmp_query_sgx_capabilities() from target/i386/monitor.c to
hw/i386/sgx.c, removing the sgx_get_capabilities() indirection.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/i386/sgx.h | 1 -
 hw/i386/sgx-stub.c    | 4 +++-
 hw/i386/sgx.c         | 3 ++-
 target/i386/monitor.c | 5 -----
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/include/hw/i386/sgx.h b/include/hw/i386/sgx.h
index 16fc25725c4..2bf90b3f4fe 100644
--- a/include/hw/i386/sgx.h
+++ b/include/hw/i386/sgx.h
@@ -7,6 +7,5 @@
 #include "qapi/qapi-types-misc-target.h"
 
 SGXInfo *sgx_get_info(Error **errp);
-SGXInfo *sgx_get_capabilities(Error **errp);
 
 #endif
diff --git a/hw/i386/sgx-stub.c b/hw/i386/sgx-stub.c
index 45c473119ef..3749656db1a 100644
--- a/hw/i386/sgx-stub.c
+++ b/hw/i386/sgx-stub.c
@@ -2,6 +2,8 @@
 #include "hw/i386/pc.h"
 #include "hw/i386/sgx-epc.h"
 #include "hw/i386/sgx.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-misc-target.h"
 
 SGXInfo *sgx_get_info(Error **errp)
 {
@@ -9,7 +11,7 @@ SGXInfo *sgx_get_info(Error **errp)
     return NULL;
 }
 
-SGXInfo *sgx_get_capabilities(Error **errp)
+SGXInfo *qmp_query_sgx_capabilities(Error **errp)
 {
     error_setg(errp, "SGX support is not compiled in");
     return NULL;
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 29724ff8f08..713f1363433 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -16,6 +16,7 @@
 #include "hw/mem/memory-device.h"
 #include "monitor/qdev.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-misc-target.h"
 #include "exec/address-spaces.h"
 #include "hw/i386/sgx.h"
 #include "sysemu/hw_accel.h"
@@ -57,7 +58,7 @@ static uint64_t sgx_calc_host_epc_section_size(void)
     return size;
 }
 
-SGXInfo *sgx_get_capabilities(Error **errp)
+SGXInfo *qmp_query_sgx_capabilities(Error **errp)
 {
     SGXInfo *info = NULL;
     uint32_t eax, ebx, ecx, edx;
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 680d282591c..84fba47f193 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -700,8 +700,3 @@ void hmp_info_sgx(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "size: %" PRIu64 "\n",
                    info->section_size);
 }
-
-SGXInfo *qmp_query_sgx_capabilities(Error **errp)
-{
-    return sgx_get_capabilities(errp);
-}
-- 
2.31.1

[PATCH 4/4] hw/i386/sgx: Move qmp_query_sgx() and hmp_info_sgx() to hw/i386/sgx.c

[Philippe Mathieu-Daudé]

Move qmp_query_sgx() and hmp_info_sgx() from target/i386/monitor.c
to hw/i386/sgx.c, removing the sgx_get_info() indirection and the
"hw/i386/sgx.h" header.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/i386/sgx.h | 11 -----------
 hw/i386/sgx-stub.c    | 10 ++++++++--
 hw/i386/sgx.c         | 26 ++++++++++++++++++++++++--
 target/i386/monitor.c | 27 ---------------------------
 4 files changed, 32 insertions(+), 42 deletions(-)
 delete mode 100644 include/hw/i386/sgx.h

diff --git a/include/hw/i386/sgx.h b/include/hw/i386/sgx.h
deleted file mode 100644
index 2bf90b3f4fe..00000000000
--- a/include/hw/i386/sgx.h
+++ /dev/null
@@ -1,11 +0,0 @@
-#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(Error **errp);
-
-#endif
diff --git a/hw/i386/sgx-stub.c b/hw/i386/sgx-stub.c
index 3749656db1a..c9b379e6651 100644
--- a/hw/i386/sgx-stub.c
+++ b/hw/i386/sgx-stub.c
@@ -1,11 +1,12 @@
 #include "qemu/osdep.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp-target.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/sgx-epc.h"
-#include "hw/i386/sgx.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc-target.h"
 
-SGXInfo *sgx_get_info(Error **errp)
+SGXInfo *qmp_query_sgx(Error **errp)
 {
     error_setg(errp, "SGX support is not compiled in");
     return NULL;
@@ -17,6 +18,11 @@ SGXInfo *qmp_query_sgx_capabilities(Error **errp)
     return NULL;
 }
 
+void hmp_info_sgx(Monitor *mon, const QDict *qdict)
+{
+    monitor_printf(mon, "SGX is not available in this QEMU\n");
+}
+
 void pc_machine_init_sgx_epc(PCMachineState *pcms)
 {
     memset(&pcms->sgx_epc, 0, sizeof(SGXEPCState));
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 713f1363433..11607568b69 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -15,10 +15,11 @@
 #include "hw/i386/sgx-epc.h"
 #include "hw/mem/memory-device.h"
 #include "monitor/qdev.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp-target.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "exec/address-spaces.h"
-#include "hw/i386/sgx.h"
 #include "sysemu/hw_accel.h"
 
 #define SGX_MAX_EPC_SECTIONS            8
@@ -86,7 +87,7 @@ SGXInfo *qmp_query_sgx_capabilities(Error **errp)
     return info;
 }
 
-SGXInfo *sgx_get_info(Error **errp)
+SGXInfo *qmp_query_sgx(Error **errp)
 {
     SGXInfo *info = NULL;
     X86MachineState *x86ms;
@@ -116,6 +117,27 @@ SGXInfo *sgx_get_info(Error **errp)
     return info;
 }
 
+void hmp_info_sgx(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    g_autoptr(SGXInfo) info = qmp_query_sgx(&err);
+
+    if (err) {
+        error_report_err(err);
+        return;
+    }
+    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);
+}
+
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 84fba47f193..8166e17693a 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -34,7 +34,6 @@
 #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)
@@ -674,29 +673,3 @@ void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "This command is obsolete and will be "
                    "removed soon. Please use 'info pic' instead.\n");
 }
-
-SGXInfo *qmp_query_sgx(Error **errp)
-{
-    return sgx_get_info(errp);
-}
-
-void hmp_info_sgx(Monitor *mon, const QDict *qdict)
-{
-    Error *err = NULL;
-    g_autoptr(SGXInfo) info = qmp_query_sgx(&err);
-
-    if (err) {
-        error_report_err(err);
-        return;
-    }
-    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);
-}
-- 
2.31.1

Re: [PATCH 2/4] hw/i386/sgx: Have sgx_epc_get_section() return a boolean

[Yang Zhong]

On Thu, Oct 07, 2021 at 07:56:10PM +0200, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/i386/sgx-epc.h | 2 +-
>  hw/i386/sgx-stub.c        | 2 +-
>  hw/i386/sgx.c             | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/i386/sgx-epc.h b/include/hw/i386/sgx-epc.h
> index 65a68ca753a..a6a65be854f 100644
> --- a/include/hw/i386/sgx-epc.h
> +++ b/include/hw/i386/sgx-epc.h
> @@ -55,7 +55,7 @@ typedef struct SGXEPCState {
>      int nr_sections;
>  } SGXEPCState;
>  
> -int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size);
> +bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size);
>  
>  static inline uint64_t sgx_epc_above_4g_end(SGXEPCState *sgx_epc)
>  {
> diff --git a/hw/i386/sgx-stub.c b/hw/i386/sgx-stub.c
> index 3be9f5ca32c..45c473119ef 100644
> --- a/hw/i386/sgx-stub.c
> +++ b/hw/i386/sgx-stub.c
> @@ -20,7 +20,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>      memset(&pcms->sgx_epc, 0, sizeof(SGXEPCState));
>  }
>  
> -int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
> +bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
>  {
>      g_assert_not_reached();
>  }
> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index e481e9358f1..29724ff8f08 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -115,13 +115,13 @@ SGXInfo *sgx_get_info(Error **errp)
>      return info;
>  }
>  
> -int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
> +bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
>  {
>      PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>      SGXEPCDevice *epc;
>  
>      if (pcms->sgx_epc.size == 0 || pcms->sgx_epc.nr_sections <= section_nr) {
> -        return 1;
> +        return true;


If return boolean, here should be return false, Sean wrote this(return 0 or 1) like Linux kernel did.  


>      }
>  
>      epc = pcms->sgx_epc.sections[section_nr];
> @@ -129,7 +129,7 @@ int sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
>      *addr = epc->addr;
>      *size = memory_device_get_region_size(MEMORY_DEVICE(epc), &error_fatal);
>  
> -    return 0;
> +    return false;

Here should be return true.

Then in the ./target/i386/cpu.c file,

 if (sgx_epc_get_section(count - 2, &epc_addr, &epc_size))

 should be

 if (!sgx_epc_get_section(count - 2, &epc_addr, &epc_size))


Yang


>  }
>  
>  void pc_machine_init_sgx_epc(PCMachineState *pcms)
> -- 
> 2.31.1