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;
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).
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
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
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 :|
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 :|
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 :|
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
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 :|
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 :|
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 :|
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 :|
© 2016 - 2026 Red Hat, Inc.