| Message ID | 20210910102258.46648-2-yang.zhong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | The HMP/QMP interfaces in Qemu SGX | expand |
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 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
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
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 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 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 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 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 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
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
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 :|
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;
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