* [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-10 10:22 [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX Yang Zhong
@ 2021-09-10 10:22 ` Yang Zhong
2021-09-10 12:39 ` Philippe Mathieu-Daudé
2021-09-10 13:46 ` Daniel P. Berrangé
2021-09-10 10:22 ` [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities() Yang Zhong
` (2 subsequent siblings)
3 siblings, 2 replies; 20+ messages in thread
From: Yang Zhong @ 2021-09-10 10:22 UTC (permalink / raw)
To: qemu-devel; +Cc: yang.zhong, pbonzini, philmd, eblake, seanjc
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;
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-10 10:22 ` [PATCH v2 1/3] monitor: Add HMP and QMP interfaces Yang Zhong
@ 2021-09-10 12:39 ` Philippe Mathieu-Daudé
2021-09-13 10:37 ` Yang Zhong
2021-09-10 13:46 ` Daniel P. Berrangé
1 sibling, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-10 12:39 UTC (permalink / raw)
To: Yang Zhong, qemu-devel
Cc: pbonzini, eblake, Markus Armbruster, Marc-André Lureau, seanjc
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).
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-10 12:39 ` Philippe Mathieu-Daudé
@ 2021-09-13 10:37 ` Yang Zhong
2021-09-13 12:42 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Yang Zhong @ 2021-09-13 10:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: pbonzini, berrange, eblake, qemu-devel, seanjc
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-13 10:37 ` Yang Zhong
@ 2021-09-13 12:42 ` Paolo Bonzini
0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-09-13 12:42 UTC (permalink / raw)
To: Yang Zhong, Philippe Mathieu-Daudé
Cc: eblake, berrange, qemu-devel, seanjc
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-10 10:22 ` [PATCH v2 1/3] monitor: Add HMP and QMP interfaces Yang Zhong
2021-09-10 12:39 ` Philippe Mathieu-Daudé
@ 2021-09-10 13:46 ` Daniel P. Berrangé
2021-09-13 9:35 ` Daniel P. Berrangé
1 sibling, 1 reply; 20+ messages in thread
From: Daniel P. Berrangé @ 2021-09-10 13:46 UTC (permalink / raw)
To: Yang Zhong; +Cc: pbonzini, philmd, qemu-devel, eblake, seanjc
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-10 13:46 ` Daniel P. Berrangé
@ 2021-09-13 9:35 ` Daniel P. Berrangé
2021-09-13 10:46 ` Yang Zhong
2021-09-13 12:48 ` Paolo Bonzini
0 siblings, 2 replies; 20+ messages in thread
From: Daniel P. Berrangé @ 2021-09-13 9:35 UTC (permalink / raw)
To: Yang Zhong, pbonzini, philmd, qemu-devel, eblake, seanjc
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-13 9:35 ` Daniel P. Berrangé
@ 2021-09-13 10:46 ` Yang Zhong
2021-09-13 12:48 ` Paolo Bonzini
1 sibling, 0 replies; 20+ messages in thread
From: Yang Zhong @ 2021-09-13 10:46 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: yang.zhong, eblake, qemu-devel, seanjc, pbonzini, philmd
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-13 9:35 ` Daniel P. Berrangé
2021-09-13 10:46 ` Yang Zhong
@ 2021-09-13 12:48 ` Paolo Bonzini
2021-09-13 12:56 ` Daniel P. Berrangé
1 sibling, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-09-13 12:48 UTC (permalink / raw)
To: Daniel P. Berrangé, Yang Zhong, philmd, qemu-devel, eblake, seanjc
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-13 12:48 ` Paolo Bonzini
@ 2021-09-13 12:56 ` Daniel P. Berrangé
2021-09-13 12:52 ` Yang Zhong
0 siblings, 1 reply; 20+ messages in thread
From: Daniel P. Berrangé @ 2021-09-13 12:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Yang Zhong, eblake, philmd, qemu-devel, seanjc
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-13 12:56 ` Daniel P. Berrangé
@ 2021-09-13 12:52 ` Yang Zhong
2021-09-13 13:24 ` Daniel P. Berrangé
0 siblings, 1 reply; 20+ messages in thread
From: Yang Zhong @ 2021-09-13 12:52 UTC (permalink / raw)
To: Daniel P. Berrangé, pbonzini; +Cc: yang.zhong, seanjc, eblake, qemu-devel
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-13 12:52 ` Yang Zhong
@ 2021-09-13 13:24 ` Daniel P. Berrangé
2021-09-16 6:14 ` Yang Zhong
0 siblings, 1 reply; 20+ messages in thread
From: Daniel P. Berrangé @ 2021-09-13 13:24 UTC (permalink / raw)
To: Yang Zhong; +Cc: pbonzini, eblake, qemu-devel, seanjc
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces
2021-09-13 13:24 ` Daniel P. Berrangé
@ 2021-09-16 6:14 ` Yang Zhong
0 siblings, 0 replies; 20+ messages in thread
From: Yang Zhong @ 2021-09-16 6:14 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: yang.zhong, pbonzini, eblake, qemu-devel, seanjc
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities()
2021-09-10 10:22 [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX Yang Zhong
2021-09-10 10:22 ` [PATCH v2 1/3] monitor: Add HMP and QMP interfaces Yang Zhong
@ 2021-09-10 10:22 ` Yang Zhong
2021-09-10 12:41 ` Philippe Mathieu-Daudé
2021-09-10 10:22 ` [PATCH v2 3/3] pc: Cleanup the SGX definitions Yang Zhong
2021-09-10 14:15 ` [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX Paolo Bonzini
3 siblings, 1 reply; 20+ messages in thread
From: Yang Zhong @ 2021-09-10 10:22 UTC (permalink / raw)
To: qemu-devel; +Cc: yang.zhong, pbonzini, philmd, eblake, seanjc
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;
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities()
2021-09-10 10:22 ` [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities() Yang Zhong
@ 2021-09-10 12:41 ` Philippe Mathieu-Daudé
2021-09-13 10:34 ` Yang Zhong
0 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-10 12:41 UTC (permalink / raw)
To: Yang Zhong, qemu-devel; +Cc: pbonzini, eblake, seanjc
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;
> +}
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities()
2021-09-10 12:41 ` Philippe Mathieu-Daudé
@ 2021-09-13 10:34 ` Yang Zhong
0 siblings, 0 replies; 20+ messages in thread
From: Yang Zhong @ 2021-09-13 10:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: pbonzini, eblake, qemu-devel, seanjc
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;
> > +}
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 3/3] pc: Cleanup the SGX definitions
2021-09-10 10:22 [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX Yang Zhong
2021-09-10 10:22 ` [PATCH v2 1/3] monitor: Add HMP and QMP interfaces Yang Zhong
2021-09-10 10:22 ` [PATCH v2 2/3] qmp: Add the qmp_query_sgx_capabilities() Yang Zhong
@ 2021-09-10 10:22 ` Yang Zhong
2021-09-10 14:15 ` [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX Paolo Bonzini
3 siblings, 0 replies; 20+ messages in thread
From: Yang Zhong @ 2021-09-10 10:22 UTC (permalink / raw)
To: qemu-devel; +Cc: yang.zhong, pbonzini, philmd, eblake, seanjc
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;
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX
2021-09-10 10:22 [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX Yang Zhong
` (2 preceding siblings ...)
2021-09-10 10:22 ` [PATCH v2 3/3] pc: Cleanup the SGX definitions Yang Zhong
@ 2021-09-10 14:15 ` Paolo Bonzini
2021-09-10 14:21 ` Daniel P. Berrangé
3 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2021-09-10 14:15 UTC (permalink / raw)
To: Yang Zhong, qemu-devel; +Cc: eblake, philmd, seanjc
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX
2021-09-10 14:15 ` [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX Paolo Bonzini
@ 2021-09-10 14:21 ` Daniel P. Berrangé
2021-09-10 19:10 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Daniel P. Berrangé @ 2021-09-10 14:21 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Yang Zhong, philmd, eblake, qemu-devel, seanjc
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 :|
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/3] The HMP/QMP interfaces in Qemu SGX
2021-09-10 14:21 ` Daniel P. Berrangé
@ 2021-09-10 19:10 ` Paolo Bonzini
0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2021-09-10 19:10 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Yang Zhong, Philippe Mathieu Daude, Blake, Eric, qemu-devel,
Sean Christopherson
[-- 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread