Re: [PATCH 3/7] i386: Add sgx_get_info() interface
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
Re: [PATCH 3/7] i386: Add sgx_get_info() interface
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
> > ---
> > 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
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
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
> ---
> 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?
[PATCH 3/7] i386: Add sgx_get_info() interface
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
---
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 00..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);
}
